-
-
Notifications
You must be signed in to change notification settings - Fork 733
Upgrade pytest to 9.0.0, remove pytest-subtests, and migrate to new TOML config #3439
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
base: main
Are you sure you want to change the base?
Conversation
|
@copilot - pytest 9.0 supports subtests natively now, and these should be used (and the pytest-subtests removed from the pyproject and project), they are documented here: https://docs.pytest.org/en/stable/how-to/subtests.html#subtests
|
Removed pytest-subtests plugin and migrated to the new TOML configuration format in commit e6029ef. Changes:
All 442 tests passing with the new configuration. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Prevents DumpError when test parameters are sent between test executors but cannot be serialised (primarily a problem for out mock objects).
e6029ef to
e4ac857
Compare
wookie184
left a comment
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.
not reviewed fully, this seems pretty meh.
| [tool.pytest] | ||
| # We don't use nose style tests so disable them in pytest. | ||
| # This stops pytest from running functions named `setup` in test files. | ||
| # See https://github.com/python-discord/bot/pull/2229#issuecomment-1204436420 | ||
| addopts = "-p no:nose" | ||
| addopts = ["-p", "no: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.
Nose support was dropped in pytest 8.0.0 so this should no longer be necessary https://docs.pytest.org/en/stable/deprecations.html#support-for-tests-written-for-nose
| for i, ((duration, message, was_silenced), target) in enumerate(itertools.product(test_cases, targets)): | ||
| with ( | ||
| mock.patch.object(self.cog, "_set_silence_overwrites", return_value=was_silenced), | ||
| self.subTest(was_silenced=was_silenced, target=target, message=message), | ||
| self.subTest(test_case=i, was_silenced=was_silenced, has_target=target is not None), | ||
| mock.patch.object(self.cog, "send_message") as send_message | ||
| ): |
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.
Adding a test case ID seems silly here, as it's coming from an itertools.product and it would be a pain to work out the inputs that it's referring to.
Also the has_targets is not a replacement for the target because there are multiple not none targets.
I'd accept that the message can be removed though, since it's an expected output.
| for i, channel in enumerate((MockVoiceChannel(), MockTextChannel())): | ||
| with self.subTest(test_case=i): |
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.
I dunno about just wrapping everything in enumerate in general, I feel like e.g. str(type(channel)) would be a more descriptive name for a test case.
| for i, overwrite_json in enumerate(('{"send_messages": true, "add_reactions": null}', None)): | ||
| with self.subTest(test_case=i, has_overwrite=overwrite_json is not None): |
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.
surely this change just isn't necessary?
|
Will go through and have a proper look case-by-case, I agree there are probably ways in a lot of them where we can better serialize instead of using enumerate. |
Pytest 9.0.0 introduced native support for unittest's
self.subTest()and stricter serialization requirements for subTest parameters when using pytest-xdist parallel execution. Non-serializable objects (mocks, datetime instances, exceptions) passed tosubTest()now causeDumpErrorfailures.Changes
pyproject.tomlpytest-subtests==0.14.1from dev dependencies (pytest 9.0 has native support)[tool.pytest.ini_options]to[tool.pytest]with list-basedaddoptsPattern Applied
Before:
After:
Configuration Migration
Before:
After:
Additional Fixes
HTTPExceptioninitialization intest_utils.py(requiresmessageargument)All 442 tests pass with xdist parallel execution.
Original prompt