-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
(fixtures): Replace fixture represenation with a class #12473
base: main
Are you sure you want to change the base?
Conversation
assert src == " @pytest.fixture\n def deco_fixture():\n assert False\n" | ||
# currently Source does not unwrap decorators, testing the | ||
# existing behavior here for explicitness, but perhaps we should revisit/change this |
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.
This test was introduced in https://github.com/pytest-dev/pytest/pull/6840/files and I updated it to work with the new code. Please let me know what you think.
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 wonder if it is a fair hack to set the __wrapped__
attribute to a property that triggers a deprecationwarning
bf90fca
to
d25a8d9
Compare
@@ -52,8 +51,8 @@ def __getattr__(self, attr): | |||
with pytest.raises( | |||
ValueError, | |||
match=( | |||
"could not find real function of <Evil left=800>\n" | |||
"stopped at <Evil left=800>" | |||
"could not find real function of <Evil left=900>\n" |
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.
explanation: previously we were doing 2 get attributes but it's now replaced by isinstance check. So left is 100 more.
3d2ec33
to
84cc5b7
Compare
Hey @The-Compiler @RonnyPfannschmidt I rebased the PR could you please take a look? |
src/_pytest/compat.py
Outdated
def get_real_func(obj): | ||
"""Get the real function object of the (possibly) wrapped object by | ||
functools.wraps or functools.partial.""" | ||
functools.wraps or functools.partial or pytest.fixture""" | ||
from _pytest.fixtures import FixtureFunctionDefinition |
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.
Is it possible to have this import at the top?
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.
This is cyclic if I remember correctly. I will try to move it to a another file and try to resolve it
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 tried moving it to fixture.py file so we can get rid of the import(otherwise it's cylcic) but that would require a lot more refactoring. I prefer to leave the import here or introduce a new file and put this function or FixtureFunctionDefinition there. What do you think?
testing/code/test_source.py
Outdated
@@ -478,12 +478,13 @@ def deco_mark(): | |||
def deco_fixture(): | |||
assert False | |||
|
|||
src = inspect.getsource(deco_fixture) | |||
# Since deco_fixture is now an instance of FixtureFunctionDef the getsource function will not work on it. | |||
with pytest.raises(Exception): |
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.
Could you assert against a more specific exception? As I understand, this is not testing that something went wrong but it's not clear what exactly. Also, it's usually a good idea to use the match=
regex with this helper.
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.
Yes will apply. This tries to document the behavior more than testing anything. Before introducing the FixtureFunctionDefinition it was possible to run inspect.getsource on fixtures but now it's not. This is just describing the behavior. I'm open to removing it as well.
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 changed it to a more specific error. Do you think we can remove the test? This just tests that you cannot use inspect on fixture functions.
8e5c28a
to
f0324a5
Compare
This is an internal class users don't need to know about it.
for more information, see https://pre-commit.ci
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
f0324a5
to
9d2a916
Compare
Closes #11525
During the sprint we discussed fixing the above issue and thought maybe it's a better idea to add a better represenation to fixtures. To do this without patching the object more, this PR refactors fixtures to have a class with attributes.
The main rational behind that is:
Example
Previously we had:
where fixt is a pytest fixture function that is not replaced by it's value(directly used)
Now we print: