parametrize part of parser test cases with pytest #727
Conversation
dateutil/test/test_parser.py
Outdated
| @@ -27,6 +27,132 @@ | |||
| except ValueError: | |||
| PLATFORM_HAS_DASH_D = False | |||
|
|
|||
| parser_test_cases = [ | |||
There was a problem hiding this comment.
For these sorts of globals I have tended to use ALL_CAPS
There was a problem hiding this comment.
ok, i'm not sure how i fee about the variable name anyway. All of these tests are "parser test cases"
There was a problem hiding this comment.
They don't have to go into a variable at all if you don't want.
Most of the time I just pass them directly into the parametrize decorator, like so:
@pytest.mark.parametrize('arg1, arg2', [
(1, 2),
(3, 4),
(5, 6),
])
def test_func(arg1, arg2):
assert func(arg1, arg2)There was a problem hiding this comment.
I saw this style of parameterization in some of the other test files. I chose against it because a lot of the important code, like: @pytest.mark.parametrize('arg1, arg2', [, is not close to the function being decorated.
I didn't like that it did not fit on one screen for both how the args are being used and what the args are. With them separated, you have to the data to be injected separate form how it is injected. I liked this because I felt the "how" should live near the function being decorated.
There was a problem hiding this comment.
Yeah, it's a reasonable choice. I would probably go with ALL_CAPS if you are going with global scope, though. See dateutil/tests/test_isoparser.py for how I did it.
dateutil/test/test_parser.py
Outdated
|
|
||
|
|
||
| @pytest.fixture | ||
| def default(): |
There was a problem hiding this comment.
What is the advantage of using a fixture for this? I will admit I don't love fixtures in general because they feel too magical (being being injected by name into the arguments is... very weird).
Since all it does is provide a datetime literal, maybe just pass that to default instead.
There was a problem hiding this comment.
pytest likes dependency injection. test_parser_default is not testing a specific default value, it is instead testing that given some text and a default that you create an expected datetime object. The test is currently written to assert that given some text an a default a particular result is created. The alternative would given some text with an exact default a particular result is created.
There was a problem hiding this comment.
But if we ever change default from returning this specific datetime literal, all the parametrized tests would immediately break, right? So really the test is whether that specific result is created given this specific default value. What specific things could we do with this fixture that a datetime literal don't accomplish?
There was a problem hiding this comment.
For this test case and your example, not much.
Things like this would be possible: https://docs.pytest.org/en/latest/fixture.html#parametrizing-fixtures
However, since that datetime object needs to be in a particular state, I don't see parametrizing fixtures useful.
dateutil/test/test_parser.py
Outdated
| @@ -536,9 +536,6 @@ def testMicrosecondPrecisionErrorReturns(self): | |||
| dt = datetime(2008, 2, 27, 21, 26, 1, ms) | |||
| self.assertEqual(parse(dt.isoformat()), dt) | |||
|
|
|||
| # def testHighPrecisionSeconds(self): | |||
There was a problem hiding this comment.
Hm... Not sure why this is commented out. If it fails it should be converted to an xfail.
There was a problem hiding this comment.
It should be deleted in the last commit. I missed it when I removed all the commented out tests. its here as a specific commit because I cherry picked two of the commits from the previously closed pr. see: 42f62c2
There was a problem hiding this comment.
Oh you commented them out as you moved them over, I see.
There was a problem hiding this comment.
yeah, very scientific.
Tried to preserver comments in assertion messages. Lots of the test case names did not have much meaning, this would be a good time to fix that.
Summary of changes
@pganssle suggested that it would be good to migrate the tests to parameterized pytests.
This is a possible partial conversion.
I've tried to keep the grouping and meaning of test cases the same.
I've converted the old test function signatures to assertion messages. Where appropriate, I've integrated the comments from the test cases into the error messages as well. I've noticed that a large number of the test cases weretestSomeThing1, testSomeThing2, testSomeThing3,... where the trailing number has appeared to be indexing the i'th test case I have dropped it.
Closes
Pull Request Checklist