Refactor tests in test_check.py to use pytest.mark.parametrize for better test isolation #1172
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Made 2 key changes:
pytest.mark.parameterize
annotation to pull the inputs out of similar unit test functionAs explained above, I used the
pytest.mark.parameterize
annotation. This in turn meant that I could delete 3 unit tests, and had to modify 2 others. For the main test functiontest_check_behaviour()
, I used the cartesian product feature to test all inputs on both a default config, and a special config.However, this turned out to be highly inefficient; it took Pytest around 1min50s to run
test_check.py
on my computer. After further digging, I realized that the reasons behind this were:With that in mind, for the second key change, I simply optimized the test inputs by:
This resulted in Pytest taking around 50s to run
test_check.py
on my computer. For reference, the master branch takes around 45s to run on my computer. This was the best I could do, since any further change affected the coverage.Final thoughts:
pytest.mark.parameterize
definitely makes the file more clean, and removes redundant code. However even after optimization, Pytest takes longer (albeit around 5 more seconds) to runtest_check.py
.Type of Change
(Write an
X
or a brief description next to the type or types that best describe your changes.)Checklist
Before opening your pull request:
If this is my first contribution, I have added myself to the list of contributors.After opening your pull request:
Questions and Comments
1)
I believe that the optimization was directly related to the refactoring I was doing. I may be mistaken though...
Should I have created a new PR for this issue?
2)
Also, this piece of code (although ugly) saves around 30s when running the test file:
Is this problematic? Should I make a separate variable for
_TEST_FILE_INPUTS + ["examples/nodes",]
?3)
I tried isolating different files from
examples/nodes
but no matter what I did, there was always a coverage problem.4)
Finally, as usual, any tips/criticism is highly appreciated and welcome. I'm always looking to improve!