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

[WIP] classmethod and class-scoped fixtures (#3778) #3781

Closed

Conversation

nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Aug 4, 2018

Unfortunately I could not come up with a fix here.

The issue is that the fixture is declared as:

@classmethod
@pytest.fixture(...)
def setup(cls): ...

When we "unwrap" the fixture to get to the underlying setup function, we lose the classmethod decoration; we would have to get the original setup function and somehow reapply the @classmethod decorator in order to get the class object as cls when we call the function later. I don't see any way to do this in a general manner.

After thinking about this issue I'm afraid the conclusion is that we have to pull back the warning about calling fixture functions directly: its intention was good but this has caused many more problems than we expected to be worth the trouble at this point.

xref: #3778

@@ -667,7 +667,9 @@ def copy_example(self, name=None):
example_path.copy(result)
return result
else:
raise LookupError("example is not found as a file or directory")
raise LookupError(
"example {} is not found as a file or directory".format(example_path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should use !r



class Test(object):
@classmethod
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the order in-correctness i was talking about in #3780 - we structurally have to loose something as long as we wrap

@nicoddemus
Copy link
Member Author

I gave this another go and I can't get it to work: classmethod objects are not really callable or have the attributes we expect functions to have (for example __name__ and __module__).

Even when I work around this by obtaining the __func__ object, I still cannot call @functools.wraps on it. Even when I work around that with a hack, it still blows on other parts of pytest because they expect callable objects later.

I'm nearly giving up here.

I see two solutions:

  1. Revert the warning code about calling fixture functions directly: because we are no longer be able to use fixtures decorated with @classmethod this seems like a breaking change to me, although it was never documented and there were not tests for it.
  2. Bite the bullet and document the fact that it was never supported and worked mostly by accident, suggesting a workaround instead: simply remove the @classmethod decorator and use cls = type(self) as one of the first lines in the fixture function instead.

What do you think @RonnyPfannschmidt? Because it worked mostly by accident and there's an easy workaround, I'm leaning towards 2).

@RonnyPfannschmidt
Copy link
Member

class methods can be stroked via the descriptor protocol to return the actual function

>>> def func(cls):
...   pass
... 
>>> 
KeyboardInterrupt
>>> cm = classmethod(func)
>>> class Class(object): pass
... 
>>> cm
<classmethod object at 0x7f66468502b8>
>>> vars(cm)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: vars() argument must have __dict__ attribute
>>> cm.__get__(None, Class)
<bound method type.func of <class '__main__.Class'>>
>>> cm.__get__(None, Class).__func__
<function func at 0x7f664684a848>

@RonnyPfannschmidt
Copy link
Member

so I'm leaning towards 3 - using the descriptor protocol (and encapsulating this bugger into something)

@nicoddemus
Copy link
Member Author

Thanks for the code. I did obtain the underlying function with my testing earlier but ran in other errors in other parts, but I will give this another go then!

@nicoddemus nicoddemus changed the title Add failing example for issue #3778 WIP classmethod/class-scoped fixtures (#3778) Aug 22, 2018
@nicoddemus nicoddemus changed the title WIP classmethod/class-scoped fixtures (#3778) [WIP] classmethod/class-scoped fixtures (#3778) Aug 22, 2018
@nicoddemus nicoddemus changed the title [WIP] classmethod/class-scoped fixtures (#3778) [WIP] classmethod and class-scoped fixtures (#3778) Aug 22, 2018
@nicoddemus
Copy link
Member Author

Hi @RonnyPfannschmidt,

I gave this another go but still no success. 😞

In this example:

class Test:

    @classmethod
    @pytest.fixture(scope='class', autouse=True)
    def setup(cls):
        ...

What happens is:

  1. @pytest.fixture is applied, and we wrap setup successfully with our object. At this point setup is a normal method, and @pytest.fixture has no way of knowing that @classmethod will be applied next.

  2. @classmethod gets applied and bound to the class.

When we get to the point of creating the FixtureDef object during parsefactories, we try to "unwrap" the underlying function.

The function is obtained using getattr(Test, "setup") in parsefactories, so at this point we obtain a bound method to the Test class (not aclassmethod object) and even don't know about that fact anymore.

At this point we have these function layers:

function (returned by classmethod's .__get__, bound to the class)
|
|__ wrapped function (emits warning)
   |
   |__ setup

And here lies the problem as I see it: as we unwrap function to get to setup (in order to call it without emitting a warning), we lose the fact that function was applied to a classmethod in the first place, so setup ends up being called as a normal function.

Would you mind giving a go at this branch yourself? I'm nearly giving up, I'm afraid.

@RonnyPfannschmidt
Copy link
Member

at earliest i can try next week

@nicoddemus
Copy link
Member Author

Sure, sounds good, thanks!

@nicoddemus
Copy link
Member Author

@RonnyPfannschmidt still want to try to fix this? I think this has very little use as we haven't seen more reports about this problem other than in #3778. I'm inclined to just close this and keep the existing behavior.

@nicoddemus
Copy link
Member Author

Closing for the reasons stated in #3778 (comment).

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

Successfully merging this pull request may close these issues.

2 participants