GeoIP2-python
GeoIP2-python copied to clipboard
Modernize tests
This PR (sibling of #178) modernizes the tests to be more Pytest and less Unittest.
Please see each commit's message for details.
Thank you for your work on porting our tests from unittest to pytest. While I appreciate the effort and recognize the benefits pytest can offer, I have some reservations about making this change at this stage of our project.
unittest is part of the Python standard library, ensuring long-term stability and compatibility. Given our project's maturity, the testing suite is relatively stable and not expected to undergo significant changes. The benefits of pytest's additional features may not outweigh the costs of migration at this point.
By sticking with unittest, we minimize external dependencies, simplify our setup, and reduce potential conflicts from API changes. It also avoids introducing a new syntax and concepts that the team would need to learn and maintain.
While pytest has its merits, especially for new projects, maintaining our current unittest setup seems the most pragmatic approach for us at this time.
@oschwald The main branch already requires pytest.
Regarding
Given our project's maturity, the testing suite is relatively stable and not expected to undergo significant changes.
the last PR to significantly change the test suite was https://github.com/maxmind/GeoIP2-python/pull/173, from last month.
Our existing use of pytest is quite minimal, mostly as a test runner. pytest is an actively developed project and is much more likely to introduce breaking changes. The PR you referenced from last month was a rare instance, and the whole point of it was to replace an unreliable dependency with many breaking changes (due to how the mocking worked) with one that is less likely to need regular maintenance.
Our existing use of
pytestis quite minimal, mostly as a test runner.
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 with pytest.raises() you're concerned about, it's practically the same thing as with self.assertRaises().
pytestis an actively developed project and is much more likely to introduce breaking changes.
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 unittest's API will be almost identical in 5 or even 10 years, minimizing unnecessary churn to the tests.
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.
The last breaking change listed in the pytest change log was in 8.2.0 in April.
Which, coincidentally, was for certain edge cases of UnitTest suites 😄 They also apologize for the inconvenience of this change happening in a minor release.
8.0.0 introduced a number of breaking changes at the start of the year.
Yes, I meant "they're careful not to break things in a non-semver way". Sorry for the imprecision.
However, it is very likely that unittest's API will be almost identical in 5 or even 10 years, minimizing unnecessary churn to the tests.
I would disagree, see e.g. https://github.com/python/cpython/pull/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 :)
Name Stmts Miss Cover Missing
--------------------------------------------------------
geoip2\__init__.py 5 0 100%
geoip2\database.py 61 0 100%
geoip2\errors.py 25 1 96% 59
geoip2\mixins.py 7 1 86% 15
geoip2\models.py 105 2 98% 347, 352
geoip2\records.py 164 1 99% 919
geoip2\types.py 3 0 100%
geoip2\webservice.py 141 2 99% 128, 443
tests\__init__.py 0 0 100%
tests\database_test.py 166 2 99% 13-14
tests\models_test.py 117 0 100%
tests\webservice_test.py 166 1 99% 266
--------------------------------------------------------
TOTAL 960 10 99%