Skip to content
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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Modernize tests #180

wants to merge 11 commits into from

Conversation

akx
Copy link

@akx akx commented Aug 28, 2024

This PR (sibling of #178) modernizes the tests to be more Pytest and less Unittest.

Please see each commit's message for details.

@oschwald
Copy link
Member

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.

@akx
Copy link
Author

akx commented Aug 28, 2024

@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 #173, from last month.

@oschwald
Copy link
Member

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.

@akx
Copy link
Author

akx commented Aug 28, 2024

Our existing use of pytest is 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().

pytest is 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.

@oschwald
Copy link
Member

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.

@akx
Copy link
Author

akx commented Aug 29, 2024

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. 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 :)

  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%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants