Deprecate calling fixture functions directly #3705

nicoddemus · 2018-07-22T14:53:49Z

The 3 first commits are small refactorings that I felt doing while implementing the actual task, so I recommend reading the commits separately.

I had to use a "wrapper" around the original fixture function to issue a warning, and one difficulty I had was to make this work reliably in Python 2 because @itertools.wraps doesn't preserve the signature. I believe this is fine though, because we probably will drop Python 2 by the time we release 4.0.

Surprisingly, we had a few tests in our test suite which did call fixture functions directly, I just fixed those while keeping the same semantics.

Fix #3661

coveralls · 2018-07-22T15:24:19Z

Coverage increased (+0.05%) to 92.542% when pulling 57b0c60 on nicoddemus:deprecate-call-fixture-func into f8749ee on pytest-dev:features.

asottile

looks good, I wonder if we want to split out some of the earlier commits so that we can tackle #3704 sooner

asottile · 2018-07-22T16:09:33Z

src/_pytest/fixtures.py

+    """Wrap the given fixture function so we can issue warnings about it being called directly, instead of
+    used as an argument in a test function.
+
+    The warning is emited only in Python 2, because I did'nt find a reliable way to make the wrapper function


"emitted", "Python 3" "didn't" I believe :)

Fixed, thanks!

asottile · 2018-07-22T16:11:33Z

src/_pytest/fixtures.py

@@ -964,6 +1011,8 @@ def __call__(self, function):
                "fixture is being applied more than once to the same function"
            )

+        function = wrap_function_to_warning_if_called_directly(function, self)


(just double checked, good that this is going to features):

changing a decorator from returning the original function to returning a wrapper always seems to break things -- I'm hoping nobody was depending on decorator ordering for @fixture before :)

asottile · 2018-07-22T16:13:53Z

testing/test_conftest.py

-    from _pytest.tmpdir import tmpdir
-
-    tmpdir = tmpdir(request, tmpdir_factory)
+    tmpdir = tmpdir_factory.mktemp("basedir", numbered=True)


could this also just take the tmpdir fixture as a positional?

No because this basedir is module-scoped, and tmpdir is function-scoped. 😉

oh right of course!

nicoddemus · 2018-07-22T20:04:39Z

looks good, I wonder if we want to split out some of the earlier commits so that we can tackle sooner

I can tackle #3704 soon after we merge this, I don't think the merging will take long. If it does for some reason, I will split it to a separate PR.

nicoddemus · 2018-07-22T20:24:49Z

Marked test_idval_hypothesis as flaky on Windows, as I've seen it fail randomly before (#3707).

RonnyPfannschmidt · 2018-07-22T20:46:33Z

src/_pytest/fixtures.py

+
+    if is_yield_function:
+
+        @functools.wraps(function)


functools.wraps is broken on python2 and needs to set __wrapped__ or so to get the correct metadata

Ahh thanks, will try that!

RonnyPfannschmidt · 2018-07-22T20:48:03Z

src/_pytest/fixtures.py

+        @functools.wraps(function)
+        def result(*args, **kwargs):
+            __tracebackhide__ = True
+            __being_called_by_pytest = kwargs.pop("__being_called_by_pytest", False)


instead of introducing this kwarg, why not just unpack the real function from the definition object

That is a great idea, thanks! I wished I have thought about it myself. 😁

I've tried and it works well, except when we have test functions which are staticmethods.

Here's an illustrative example:

import pytest class Test(object): @staticmethod def test_something(): pass @pytest.fixture def fix(self): return 1 @staticmethod def test_fix(fix): assert fix == 1

test_something passes, but test_fix fails because we can't create the fixture because request.instance is None at that point.

The place where I did the unwrapping was in resolve_fixture_function:

def resolve_fixture_function(fixturedef, request): """Gets the actual callable that can be called to obtain the fixture value, dealing with unittest-specific instances and bound methods. """ fixturefunc = get_real_func(fixturedef.func) # <- unwrapping! if fixturedef.unittest: if request.instance is not None: # bind the unbound method to the TestCase instance fixturefunc = fixturefunc.__get__(request.instance) else: # the fixture function needs to be bound to the actual # request.instance so that code working with "fixturedef" behaves # as expected. if request.instance is not None: #is_method = getimfunc(fixturedef.func) != fixturedef.func is_method = inspect.ismethod(fixturedef.func) if is_method: fixturefunc = fixturefunc.__get__(request.instance) return fixturefunc

This issue affects 6 of our tests, on py27 and py36.

Any suggestions on how to proceed? The unwrapping before calling is much cleaner, but I'm not sure how to deal with staticmethods in that case.

this seems to me like we should open a follow-up issue and go for the mechanism you currently have as enabling the cleaner way requires a re-factoring/bugfixing in fixture lookup/requesting

Just to make sure I follow, keep using the keyword argument approach, and warning only in Python 3? I can try to make it work with Python 2 if you think setting __wrapped__ will work.

please try setting wrapped - wrt the keyword argument approach - i dont like it but as far as i understood we need to sort out a structural thing to get real extraction working

so we keep kwargs, and we try to set up the warnings for py2 as well

Follow up in #3720

RonnyPfannschmidt · 2018-07-22T20:51:10Z

src/_pytest/pytester.py

@@ -669,6 +669,11 @@ def copy_example(self, name=None):
        else:
            raise LookupError("example is not found as a file or directory")

+    def run_example(self, name=None, *pytest_args):


required this one seems problematic due to leaving state behind, as is this api is only oblivious/sensible to core dev's

i'm pretty sure a plugin author would misuse it running it twice

for now i propose using the markers to declare what example to copy to keep apart the folder structure prep and the test running

Good point, I can remove it.

This will now issue a RemovedInPytest4Warning when the user calls a fixture function directly, instead of requesting it from test functions as is expected Fix pytest-dev#3661

nicoddemus · 2018-07-26T23:08:36Z

Pushed the changes that trigger the warning on Python 2, thanks @RonnyPfannschmidt for the tip.

asottile reviewed Jul 22, 2018

View reviewed changes

nicoddemus force-pushed the deprecate-call-fixture-func branch from 00edd81 to 571ef67 Compare July 22, 2018 20:31

RonnyPfannschmidt requested changes Jul 22, 2018

View reviewed changes

This was referenced Jul 22, 2018

Small refactorings #3708

Merged

Improve lookup/request to allow access to class instances from staticfunctions #3720

Closed

nicoddemus added 2 commits July 26, 2018 19:58

Deprecate calling fixture functions directly

011f88f

This will now issue a RemovedInPytest4Warning when the user calls a fixture function directly, instead of requesting it from test functions as is expected Fix pytest-dev#3661

Mark test_idval_hypothesis as flaky on Windows (pytest-dev#3707)

6e57d12

nicoddemus force-pushed the deprecate-call-fixture-func branch from 571ef67 to 6e57d12 Compare July 26, 2018 23:03

Remove Testdir.run_example as recommended

57b0c60

RonnyPfannschmidt approved these changes Jul 27, 2018

View reviewed changes

nicoddemus merged commit fe16f81 into pytest-dev:features Jul 27, 2018

nicoddemus deleted the deprecate-call-fixture-func branch July 27, 2018 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate calling fixture functions directly #3705

Deprecate calling fixture functions directly #3705

nicoddemus commented Jul 22, 2018

coveralls commented Jul 22, 2018 •

edited

Loading

asottile left a comment

asottile Jul 22, 2018

nicoddemus Jul 22, 2018

asottile Jul 22, 2018

nicoddemus Jul 22, 2018

asottile Jul 22, 2018

nicoddemus Jul 22, 2018 •

edited

Loading

asottile Jul 23, 2018

nicoddemus commented Jul 22, 2018

nicoddemus commented Jul 22, 2018

RonnyPfannschmidt Jul 22, 2018

nicoddemus Jul 22, 2018

RonnyPfannschmidt Jul 22, 2018

nicoddemus Jul 22, 2018

RonnyPfannschmidt Jul 23, 2018

nicoddemus Jul 23, 2018

RonnyPfannschmidt Jul 23, 2018

RonnyPfannschmidt Jul 23, 2018

nicoddemus Jul 26, 2018

RonnyPfannschmidt Jul 22, 2018 •

edited

Loading

nicoddemus Jul 22, 2018

nicoddemus commented Jul 26, 2018

Deprecate calling fixture functions directly #3705

Deprecate calling fixture functions directly #3705

Conversation

nicoddemus commented Jul 22, 2018

coveralls commented Jul 22, 2018 • edited Loading

asottile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus Jul 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented Jul 22, 2018

nicoddemus commented Jul 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonnyPfannschmidt Jul 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented Jul 26, 2018

coveralls commented Jul 22, 2018 •

edited

Loading

nicoddemus Jul 22, 2018 •

edited

Loading

RonnyPfannschmidt Jul 22, 2018 •

edited

Loading