-
Notifications
You must be signed in to change notification settings - Fork 203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Pytest for test execution #1043
Conversation
Adds a dependency for pytest and configurates it to search for files following the pytest_* scheme. After the transition to pytest is completed and the nose-based testsuite can be safely removed, it is probably a good idea to rename the pytest-based testsuite back to the current test_* scheme. While having both testsuites at the same time causes temporary code duplication, it makes the transition process easier and allows quick three-way diffs between the main branch and the, on this branch, unmodified tests (via git diff) and the changes specific to pytest (via regular diff on this branch).
The same already applies to the existing nose-based testsuite - using wildcard imports shortens test code significantly
Saves the coverage of both testsuites, to allow us to compare them (and make sure the nose-based testsuite hasn't had a regression for some reason)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if the final diff of the PR (before merging) is somewhat useful and readable (to allow reviewing). I know that there will inevitably be quite some changes due to the test structure and the assertions, but can you maybe aim at making these changes as localized as possible? Such that for example comparing pytest_foo.py
against test_foo.py
is doable.
We also want to compare the old tests against the new tests, in particular with regards to the question how many tests are executed, in order to find any overlooked things.
For this I wonder what the best thing to do is?
- Copy the test suite in a branch (as you do) such that we can always execute both? And only at the end of the project delete the old test suite and rename the new one?
- Simply replace the test suite in a branch and then compare it with an execution of the old test suite on
main
? This allows to continually look at the diff between the test suites using GitHub. - Replace the test suite incrementally, module by module, after checking that the module is rewritten correctly. Makes everything smaller and easier to follow, but with the disadvantage of having two independent test suites in CI for a while.
I'm not sure if the localization of the changes is the main problem here, i.e. a
Do you have a preference? I'll do what's easiest to review for you.
Another way to do this, in hindsight a better way, would be duplicating and renaming the current testsuite, and changing the existing files to pytest. As a downside, this would again cause commits with "brand-new" files (the existing, nose-based testsuite) during development, however, as those would be deleted with the last commit before merging, they shouldn't need to be reviewed. In this case, this may be fine though, as the only reason for duplicating the testsuite in the branch would be to make sure we don't miss any changes to the testsuite on main in the meantime (e.g. added tests which may not cause a merge conflict) and to make comparing the results between the two testsuites (especially coverage) more comfortable, right? Without copying the testsuite (or simply not checking it into git), this would be equivalent to option 2. |
I would expect that almost all changes to the test suite on main in the meantime would cause a merge conflict. And to double check it is easy to get a list of all commits touching test files.
Then we can simply do option 2, if you like that most. |
…assignment, test_pqos to pytest
Yes, this is exactly right. Pytest supports unittest-based testsuites and even "mixtures" where some tests are based on unittest, some are based on native assert statements. |
Lists are attached (tests_pytest.txt, tests_nose.txt) - they are indeed identical (even though formatting is a bit different). Note that we have one test more than on main, as I added a test to make sure any (existing) changes to the rounding mode are not silently overwritten (but will cause an assertion error) - thinking about it now, this test becomes redundant after #991 is fixed, and should be flipped around: External interference should not cause an assertion error, as we use a local context. (Commands to verify the lists independently are: sed -e 's|<testcase|\
|g' -e 's|</testcase>|\
|g' nosetests.xml | grep classname | awk -F '"' '{ print $2'.'$4 }' | sort -u for nose) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for the work and comparing the test results!
Just some minor cleanup now.
now requires no assertion to be violated when the rounding mode is modified for the thread, which should always be the case as long as local context is used (which sets the rounding mode independently of the rounding mode of the actual, global decimal context of the thread
… extend localcontext to additonal methods
…mode, formerly set for the whole thread
Since "setup.py test" is no longer possible, we need to tell people.
This fastens up CI jobs by not having to install it on every job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! I am happy that this is implemented and one of our oldest open issues is resolved.
This branch changes the test runner to pytest. Fixes #162.