Split up unit test files and rename tests directory to test #351
Conversation
Refactoring Revert and refactor unit tests Finish moving tests to separate modules Remove test feature files Change paths for tests Change makefile Finish moving tests to separate modules Remove test feature files
algochoi
changed the title
test_unit.py
Outdated
| [m.get_signature() for m in c.methods], | ||
| ["add(uint64,uint64)void", "multiply(uint64,uint64)void"], | ||
| ) | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
I see we are keeping this file to act as a "driver" that determines which tests to actually run. I think this is a good opportunity to move away from this approach. For example, last week I almost took a victory lap on my encoder unit test before I realized that it wasn't getting run at all (I had it add it on the former line 4160 of this file). I don't see a compelling reason to have tests in the codebase that aren't being run. I believe that with your new refactoring, all tests will be run if we change line 28 of config.yml to the following (but haven't actually tried it) assuming you want to:
- run these unit tests in an asynchronous fashion and have added
pytest-xdist==2.5.0torequirements.txt - print the 10 slowest tests at the end of each run
pytest -n auto --durations=10 -sv tests/unit_tests
There was a problem hiding this comment.
For those with more Python experience: If we move away from requiring a driver to run the tests, is it more idiomatic to co-locate non-cucumber unit test files with source files (named as <test_name>_test.py rather than the separate dir?
From my scan of https://docs.python.org/3/library/unittest.html#test-discovery, it seems _test.py is an accepted convention.
There was a problem hiding this comment.
I personally prefer a separate tests directory, but we seem to be mostly favoring the approach you mentioned. pytest is smart enough to figure it out if you give it the root directory.
There was a problem hiding this comment.
Got it - alright, disregard my co-location question. Removing the driver seems prudent.
There was a problem hiding this comment.
That being said, I must confess again to not actually trying it... I commit to trying this by standup if no one else has.
There was a problem hiding this comment.
So is everyone in favor of removing the driver file and using something like: pytest tests/unit_tests?
On my side, yes. It feels less error prone because tests won't require registration.
I can also change the test file names to *_tests.py to follow the convention.
- I don't feel strongly here. I also don't know what's considered common convention. I originally raised it when considering co-location with source files.
- If it's considered Pythonic and you're up for the change, then I'm all for it.
There was a problem hiding this comment.
So is everyone in favor of removing the driver file and using something like:
pytest tests/unit_tests?
+1 to getting rid of test_unit.py. I think because our unit tests are written using unittest, not pytest, you may need to use an approach like this: https://stackoverflow.com/a/15630454, but regardless, auto discovery of all tests should be preferred over a driver file.
I can also change the test file names to
*_tests.pyto follow the convention.
I think it only makes sense to add a suffix like this when tests are co-located with other source files.
There was a problem hiding this comment.
Sounds good, it looks like both of these commands will work:
$ python -m unittest discover tests/unit_tests
with pytest (better formatting):
$ pytest tests/unit_tests
Pytest seems to work with the version that's currently in the requirements.txt and outputs better formatting so I'd be inclined to use that.
There was a problem hiding this comment.
Ok great, I agree pytest should be preferred
Co-authored-by: Zeph Grunschlag <[email protected]>
Co-authored-by: Zeph Grunschlag <[email protected]>
Co-authored-by: Zeph Grunschlag <[email protected]>
There was a problem hiding this comment.
@algochoi Thanks for splitting out the tests. ☕
I don't consider myself an authoritative reviewer. Appreciate if you get at least 1 more approval prior to merging.
This PR separates the different unit test classes into different files for improved readability.
This PR also proposes to rename the
testdirectory totestsdirectory to prevent any conflicts with the existing Pythontestpackage.