python: fix instance handling in static and class method tests #12096
+105
−19
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
No longer does unittest stuff. Also the rest of the sentence is not really necessary for a docstring.
No need to compute the property multiple times.
and also fixes a regression in pytest 8.0.0 where `setup_method` crashes if the class has static or class method tests. It is allowed to have a test class with static/class methods which request non-static/class method fixtures (including `setup_method` xunit-fixture). I take it as a given that we need to support this somewhat odd scenario (stdlib unittest also supports it). This raises a question -- when a staticmethod test requests a bound fixture, what is that fixture's `self`? stdlib unittest says - a fresh instance for the test. Previously, pytest said - some instance that is shared by all static/class methods. This is definitely broken since it breaks test isolation. Change pytest to behave like stdlib unittest here. In practice, this means stopping to rely on `self.obj.__self__` to get to the instance from the test function's binding. This doesn't work because staticmethods are not bound to anything. Instead, keep the instance explicitly and use that. BTW, I think this will allow us to change `Class`'s fixture collection (`parsefactories`) to happen on the class itself instead of a class instance, allowing us to avoid one class instantiation. But needs more work. Fixes pytest-dev#12065.
nicoddemus
approved these changes
Mar 9, 2024
| assert isinstance(items[1], Function) | ||
| assert items[1].name == "test_method" | ||
| assert items[1].instance is not None | ||
| assert items[1].instance.__class__.__name__ == "TestIt" | ||
|
|
||
| # Even class and static methods get an instance! |
There was a problem hiding this comment.
I guess it is possible for this to break some plugin which is using instance to check if the method is a static/classmethod, but I suppose this is far fetched.
There was a problem hiding this comment.
You prompted me to check how plugins use instance. Overall it's almost not used, the only prominent case I found is pytest-asyncio, whose usage is not problematic if I'm reading correctly and according to its tests.
pytest-asyncio however was broken by yesterday's removal of FixtureDef.unittest (#12089), for which I will send them a fix.
This was referenced Apr 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
and also fixes a regression in pytest 8.0.0 where
setup_methodcrashes if the class has static or class method tests.It is allowed to have a test class with static/class methods which request non-static/class method fixtures (including
setup_methodxunit-fixture). I take it as a given that we need to support this somewhat odd scenario (stdlib unittest also supports it).This raises a question -- when a staticmethod test requests a bound fixture, what is that fixture's
self?stdlib unittest says - a fresh instance for the test.
Previously, pytest said - some instance that is shared by all static/class methods. This is definitely broken since it breaks test isolation.
Change pytest to behave like stdlib unittest here.
In practice, this means stopping to rely on
self.obj.__self__to get to the instance from the test function's binding. This doesn't work because staticmethods are not bound to anything.Instead, keep the instance explicitly and use that.
BTW, I think this will allow us to change
Class's fixture collection (parsefactories) to happen on the class itself instead of a class instance, allowing us to avoid one class instantiation. But needs more work.Fixes #12065.