This PR aims to fix Issue #233 #292

EricLiclair · 2021-03-24T07:09:52Z

What:

Fix for issue #233

Added Constructor level input type checks for various Matchers.
Added missing tests for testAnyFloat, testNotEmptyList and testEmptyList

Why:

How:

For point 1
Checked for the passed arguments to be of the specified types. If not, raised a ValueError with appropriate error message.

Risks:

N/A. At least not intentionally.

Checklist:

Added tests, if you've added code that should be tested
Updated the documentation, if you've changed APIs
Ensured the test suite passes
Made sure your code lints
Completed the Contributor License Agreement ("CLA")

…Used TestArguments

deathowl · 2021-03-25T12:48:17Z

@EricLiclair build seems to break for Python3.9

EricLiclair · 2021-03-25T13:39:23Z

@deathowl I did notice the failed build test, but i am not sure why is that happening. I checked the test log and found that tests/mock_async_callable_testslide.py is where the test fails but I have not changed anything in that file. Can you suggest me something to get this fixed?

Also, the make instruction successfully executes in my system. I wonder where am I going wrong.

I also forked the master repo from my second account just to be sure I was not missing anything, added my changes and dropped a PR. Seems the build failed for that PR as well. I am kind of stuck. I'll be looking into this, but your comments would be appreciated.

coveralls · 2021-03-28T20:04:33Z

Pull Request Test Coverage Report for Build 695782107

36 of 36 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 94.344%

Totals
Change from base Build 687383861:	0.2%
Covered Lines:	2602
Relevant Lines:	2758

💛 - Coveralls

tests/matchers_unittest.py

EricLiclair · 2021-03-28T20:06:06Z

tests/matchers_unittest.py

+        self.assertEqual(testslide.matchers.FloatBetween(2.1, 666.0), 4.2)
+        self.assertEqual(testslide.matchers.FloatBetween(2.1, 666.0), 2.1)
+        self.assertEqual(testslide.matchers.FloatBetween(21.0, 66.6), 66.6)


This is where the tests were updated as the argument used 666 was not of type float.

I need to go through all the PR to fully review it.

a suggestion that I would provide, by checking this test, I'd that it would consider acceptable to have integers in the bounds of FloatBeetween, after all the intention is to get numbers (floats) within 2 ranges.

The same comment will apply to greater and lesser matchers as well as for IntBetween

tests/matchers_unittest.py

EricLiclair · 2021-03-28T20:23:33Z

testslide/matchers.py

@@ -265,7 +313,7 @@ def __init__(self) -> None:

 class RegexMatches(Matcher):
    """
-    Compares true if other mathes given regex.
+    Compares true if other matches given regex.
    """

    def __init__(self, pattern: str, flags: int = 0) -> None:


What I would like to mention is that, adding a type check here was the reason the build was failing. I wonder why was it so. And hence I couldn't add a type check here. Any comments on this will be appreciated.

I'm not sure I'm following you ... what failure specifically are you referring to?

Checking the history of your commits I see that the code was something like

def __init__(self, pattern: str, flags: int = 0) -> None: if not isinstance(pattern, str) or not isinstance(flags, int): raise ValueError( f"RegexMatches(...) expects 'str' and 'int' as arguments while '{type(pattern).__name__}' and '{type(flags).__name__}' were provided" ) ...

how is this causing random test failures?

Side (and personal) note: to make the code-review easier I would suggest to limit merging master branch in your branch.
This because it makes harder to trace changes and makes the history more confusing. Clearly, this does not mean that you should not be doing it but do it parsimoniously.
I do tend to use git rebase on the PR branch if the PR takes a very long time to be reviewed and merged in order to gather all the updates.
If a PR is short-living it would be unlikely to have failures caused by other changes and anyway when github actions runs tests they are run on top of your branch merged with master, so tests will detect if your changes would cause test failures caused by new code.

I wanted to point the PR checks.
as mentioned by deathowl here

I was curious why the build was failing so i checked for the build logs as i mentioned here

This is the failure i was mentioning about.

And, I'll remember the note.

In fact all of the commits were failing build test, until the last one.

EricLiclair · 2021-03-28T20:25:34Z

@deathowl hey, sorry that it took a lot of time figuring out what went wrong.

Also, @macisamuele I believe this should work. ☺
Review awaited.

macisamuele

@EricLiclair thanks a lot for your PR.
I do like the general approach used, I would just relax a little bit the constraints here in order to not have a huge toll on developers using it.

I would still be curious to understand what was causing the test failures. Is this something that we can address in this PR such that we can re-add the ValueError throwing into RegexMatches as well?

macisamuele · 2021-03-28T21:43:43Z

testslide/matchers.py

        super().__init__(ne=ne)


 class IntBetween(_IntComparison):
    def __init__(self, lower: int, upper: int) -> None:
+        if not isinstance(lower, int) or not isinstance(upper, int):


I would personally allow float values as well.
I mean what if I'm looking for any integer between 2.8 and 33.9 ?

Think about a case like

def test_dummy(self) -> None: lower = some_calculation() # This returns a number, maybe an integer value but probably stored in float upper = lower * 2 IntBetween(lower, upper) # This would fail, but from a conceptual point of view it might be valid

This comment is valid for all the Int* matchers

macisamuele · 2021-03-28T21:46:39Z

testslide/matchers.py

        super().__init__(ne=ne)


 class FloatBetween(_FloatComparison):
    def __init__(self, lower: float, upper: float) -> None:
+        if not isinstance(lower, float) or not isinstance(upper, float):


I would allow integers values as well.
The same reasoning of IntBetween holds in this case.

Let's say that I want to match any number that is between 0 and 100 ... why should I be forced to represent them as floats?

macisamuele · 2021-03-28T21:49:32Z

tests/matchers_unittest.py

+        with self.assertRaises(ValueError):
+            testslide.matchers.IntGreaterThan(42.42)


I would not make it fail in this case ...

The "concept" here is to have all the integer bigger than 42.42 so numbers like 43, 44, etc. I would say that constraining floats away from here would require users of the matchers to make more gymnastics with floor/ceil/int which might end up with tests that would be less obvious to be read

Insightful, I really missed out the point that both float and integer values can be valid range extremes. I'll try to update accordingly as per the comments you mentioned. Thanks for your review. ☺

macisamuele · 2021-03-28T21:57:19Z

@deathowl : do you have strong opinion against ensuring that we still run full tests on all the python versions even if a single version failed?
I do find this information useful to triage failures which might affect one python version but not another.
This should be rather a simple update on the workflow, just wondering if you have adverse opinions.

deathowl · 2021-03-29T09:31:55Z

@macisamuele i'll resurrect Fabios Tox diff, and make it work for running all our tests . I think running against all supported py versions is strong signal

… or int ranges

…led (#295) Summary: **What:** Ensure that we do run all the tests (platforms/python-versions) and that we don't immediately stop if one has failed. This is a follow-up from #292 **Why:** In case of test failures is extremely interesting to have information about "is this failure a genuine bug or is an issue with a single python version?". **How:** Ensure that github workflow is correctly defined. Doc in [here](https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idstrategyfail-fast) **Risks:** n/a **Checklist**: - [x] Added tests, if you've added code that should be tested - [x] Updated the documentation, if you've changed APIs - [x] Ensured the test suite passes - [x] Made sure your code lints - [x] Completed the Contributor License Agreement ("CLA") Pull Request resolved: #295 Reviewed By: deathowl Differential Revision: D27395399 Pulled By: macisamuele fbshipit-source-id: e474f46614b5422c798b0fcfbd73edab58a9e007

deathowl · 2021-04-08T12:11:40Z

Hey @EricLiclair build still seems broken to me

EricLiclair · 2021-05-21T10:03:56Z

@deathowl @macisamuele Sorry for being unavailable for the past few months. Hard times.
I added the fix to this in #303 .
This PR can now be closed. Thank you.

deathowl · 2021-05-25T22:35:17Z

Closing because resubmitted in #303

EricLiclair added 7 commits March 22, 2021 23:52

Raise Exception from AnyInstanceOf Constructor

0f6aa8f

Update tests/matchers_test.py sample_module.py

c274151

inlining tests

58e90c3

update : single test for anyInstanceOf

0adcc9b

Merge remote-tracking branch 'upstream/master'

d757d3d

Adding Constructor Level RaiseError + MissingTests + Updated Wrongly …

1ce5127

…Used TestArguments

Updating with Remote Branch

600af99

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Mar 24, 2021

EricLiclair added 3 commits March 26, 2021 09:15

Merge from remote repo

ffde464

clearing tests + fixed a typo

209ddff

reverting tests

24c9855

EricLiclair commented Mar 28, 2021

View reviewed changes

tests/matchers_unittest.py Show resolved Hide resolved

EricLiclair commented Mar 28, 2021

View reviewed changes

tests/matchers_unittest.py Show resolved Hide resolved

EricLiclair commented Mar 28, 2021

View reviewed changes

macisamuele reviewed Mar 28, 2021

View reviewed changes

macisamuele mentioned this pull request Mar 29, 2021

Enable running all tests to completion even if one python version failed #295

Closed

5 tasks

making type checks flexible for numerical ranges, rather strict float…

96a73dc

… or int ranges

deathowl closed this May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

This PR aims to fix Issue #233 #292

This PR aims to fix Issue #233 #292

EricLiclair commented Mar 24, 2021 •

edited

Loading

deathowl commented Mar 25, 2021

EricLiclair commented Mar 25, 2021

coveralls commented Mar 28, 2021

EricLiclair Mar 28, 2021

macisamuele Mar 28, 2021 •

edited

Loading

EricLiclair Mar 28, 2021

macisamuele Mar 28, 2021

EricLiclair Mar 28, 2021

EricLiclair Mar 28, 2021

EricLiclair commented Mar 28, 2021

macisamuele left a comment

macisamuele Mar 28, 2021

macisamuele Mar 28, 2021

macisamuele Mar 28, 2021

EricLiclair Mar 28, 2021

macisamuele commented Mar 28, 2021

deathowl commented Mar 29, 2021

deathowl commented Apr 8, 2021

EricLiclair commented May 21, 2021

deathowl commented May 25, 2021

		with self.assertRaises(ValueError):
		testslide.matchers.IntGreaterThan(42.42)

This PR aims to fix Issue #233 #292

This PR aims to fix Issue #233 #292

Conversation

EricLiclair commented Mar 24, 2021 • edited Loading

deathowl commented Mar 25, 2021

EricLiclair commented Mar 25, 2021

coveralls commented Mar 28, 2021

Pull Request Test Coverage Report for Build 695782107

💛 - Coveralls

Choose a reason for hiding this comment

macisamuele Mar 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricLiclair commented Mar 28, 2021

macisamuele left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macisamuele commented Mar 28, 2021

deathowl commented Mar 29, 2021

deathowl commented Apr 8, 2021

EricLiclair commented May 21, 2021

deathowl commented May 25, 2021

EricLiclair commented Mar 24, 2021 •

edited

Loading

macisamuele Mar 28, 2021 •

edited

Loading