-
Notifications
You must be signed in to change notification settings - Fork 142
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
Modernize tests #180
base: main
Are you sure you want to change the base?
Modernize tests #180
Conversation
Via `ast-grep run --pattern 'type($A) == $B' --rewrite 'isinstance($A, $B)' -iU tests`
Thank you for your work on porting our tests from
By sticking with While |
Our existing use of |
The point stands – as-is, the tests can't be run without Pytest. The upside of using e.g. plain asserts is a more familiar syntax, as well as better diagnostics for failing assertions. If it's
It's a mature project that is downloaded over 6 million times a day. In my experience, they are very careful not to introduce breaking changes. If you've bumped into such breaking changes, I'd be interested to know! If you are worried about breaking changes in Pytest, then the version of Pytest used by this project should be pinned to a given version range. I can do that in this PR as well. |
The last breaking change listed in the pytest change log was in 8.2.0 in April. 8.0.0 introduced a number of breaking changes at the start of the year. I am in no way minimizing the work of the pytest team or saying that these changes should not have been made. However, it is very likely that Yes, pinning the version is a short-term solution, but eventually we would likely need to upgrade pytest, e.g., to support a newer Python version, and our internal policies would eventually require it as well. Although we currently use pytest, it would be easy to replace and our current use seems unlikely to break due to API changes. Again, I think this is just a matter of values and the life cycle of the project. This project is relatively mature and we value avoiding unnecessary maintenance over making the test suite more modern. |
Which, coincidentally, was for certain edge cases of UnitTest suites 😄 They also apologize for the inconvenience of this change happening in a minor release.
Yes, I meant "they're careful not to break things in a non-semver way". Sorry for the imprecision.
I would disagree, see e.g. python/cpython#28268 and the PRs referencing it. Anyway, I took the liberty of pinning pytest as discussed above, adding pytest-cov for coverage reporting and adding test coverage in a couple of places that were missing :)
|
This PR (sibling of #178) modernizes the tests to be more Pytest and less Unittest.
Please see each commit's message for details.