pytest-dev / pytest

The pytest framework makes it easy to write small tests, yet scales to support complex functional testing
https://pytest.org
MIT License
12.17k stars 2.7k forks source link

Confusing `AssertionError` when using `request.getfixturevalue` with new fixture during teardown #12882

Open The-Compiler opened 1 month ago

The-Compiler commented 1 month ago

With 8.3.3 (as well as the current main, f373974707f57a0b28d12563e4d03c7cd54c70d9), something like

import pytest

def test_stuff(fixt):
    pass

@pytest.fixture
def fixt(request):
    yield
    # e.g. store screenshots on failures for GUI tests
    request.getfixturevalue("tmp_path")

results in a rather confusing internal AssertionError from pytest:

============================= test session starts ==============================
collected 1 item

test_cleanup.py .E                                                       [100%]

==================================== ERRORS ====================================
_______________________ ERROR at teardown of test_stuff ________________________

request = <SubRequest 'fixt' for <Function test_stuff>>

    @pytest.fixture
    def fixt(request):
        yield
        # e.g. store screenshots on failures for GUI tests
>       request.getfixturevalue("tmp_path")

test_cleanup.py:12: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/usr/lib/python3.12/site-packages/_pytest/fixtures.py:532: in getfixturevalue
    fixturedef = self._get_active_fixturedef(argname)
/usr/lib/python3.12/site-packages/_pytest/fixtures.py:617: in _get_active_fixturedef
    fixturedef.execute(request=subrequest)
/usr/lib/python3.12/site-packages/_pytest/fixtures.py:1094: in execute
    request.node.addfinalizer(finalizer)
/usr/lib/python3.12/site-packages/_pytest/nodes.py:391: in addfinalizer
    self.session._setupstate.addfinalizer(fin, self)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <_pytest.runner.SetupState object at 0x7457a8320470>
finalizer = functools.partial(<bound method FixtureDef.finish of <FixtureDef argname='tmp_path' scope='function' baseid=''>>, request=<SubRequest 'tmp_path' for <Function test_stuff>>)
node = <Function test_stuff>

    def addfinalizer(self, finalizer: Callable[[], object], node: Node) -> None:
        """Attach a finalizer to the given node.

        The node must be currently active in the stack.
        """
        assert node and not isinstance(node, tuple)
        assert callable(finalizer)
>       assert node in self.stack, (node, self.stack)
E       AssertionError: (<Function test_stuff>, {})

/usr/lib/python3.12/site-packages/_pytest/runner.py:526: AssertionError
=========================== short test summary info ============================
ERROR test_cleanup.py::test_stuff - AssertionError: (<Function test_stuff>, {})
========================== 1 passed, 1 error in 0.04s ==========================

which is here:

https://github.com/pytest-dev/pytest/blob/d0f136fe64f9374f18a04562305b178fb380d1ec/src/_pytest/runner.py#L519-L527

This seems similar to what has been reported (and improved) by @petebman here:

and indeed, also requesting tmp_path from the test (after fixt, so it's torn down first) leads to a much better error message:

AssertionError: The fixture value for "tmp_path" is not available. This can happen when the fixture has already been torn down.

From what I can gather, what happens here instead is:


In the actual context we saw this in qutebrowser, this was made even more difficult to debug:

>    fixture = self.request.getfixturevalue('take_x11_screenshot')
E    AssertionError: [...]

without a full traceback.

AssertionError: (<Function test_sandboxing[disable-seccomp-bpf-True-False-True-You are NOT adequately sandboxed.]>, {<Session  exitstatus=<ExitCode.OK: 0> testsfailed=3 testscollected=3>: ([<bound method Node.teardown of <Session  exitstatus=<ExitCode.OK: 0> testsfailed=3 testscollected=3>>, functools.partial(<bound method FixtureDef.finish of <FixtureDef argname='check_display' scope='session' baseid='tests'>>, request=<SubRequest 'check_display' for <Function test_sandboxing[enable-all-True-True-True-You are adequately sandboxed.]>>), functools.partial(<bound method FixtureDef.finish of <FixtureDef argname='check_yaml_c_exts' scope='session' baseid='tests'>>, request=<SubRequest 'check_yaml_c_exts' for <Function test_sandboxing[enable-all-True-True-True-You are adequately sandboxed.]>>), functools.partial(<bound method FixtureDef.finish of <FixtureDef argname='fail_on_logging' scope='session' baseid='tests'>>, request=<SubRequest 'fail_on_logging' for <Function test_sandboxing[enable-all-True-True-True-You are adequately sandboxed.]>>), functools.partial(<bound method FixtureDef.finish of <FixtureDef argname='qapp_args' scope='session' baseid='tests'>>, request=<SubRequest 'qapp_args' for <Function test_sandboxing[enable-all-True-True-True-You are adequately sandboxed.]>>), functools.partial(<bound method FixtureDef.finish of <FixtureDef argname='qapp_cls' scope='session' baseid=''>>, request=<SubRequest 'qapp_cls' for <Function test_sandboxing[enable-all-True-True-True-You are adequately sandboxed.]>>), functools.partial(<bound method FixtureDef.finish of <FixtureDef argname='pytestconfig' scope='session' baseid=''>>, request=<SubRequest 'pytestconfig' for <Function test_sandboxing[enable-all-True-True-True-You are adequately sandboxed.]>>), functools.partial(<bound method FixtureDef.finish of <FixtureDef argname='qapp' scope='session' baseid=''>>, request=<SubRequest 'qapp' for <Function test_sandboxing[enable-all-True-True-True-You are adequately sandboxed.]>>), functools.partial(<bound method FixtureDef.finish of <FixtureDef argname='qapp' scope='session' baseid='tests'>>, request=<SubRequest 'qapp' for <Function test_sandboxing[enable-all-True-True-True-You are adequately sandboxed.]>>), functools.partial(<bound method FixtureDef.finish of <FixtureDef argname='server' scope='session' baseid='tests/end2end'>>, request=<SubRequest 'server' for <Function test_sandboxing[enable-all-True-True-True-You are adequately sandboxed.]>>), functools.partial(<bound method FixtureDef.finish of <FixtureDef argname='tmp_path_factory' scope='session' baseid=''>>, request=<SubRequest 'tmp_path_factory' for <Function test_sandboxing[enable-all-True-True-True-You are adequately sandboxed.]>>)], None), <Dir work>: ([<bound method Node.teardown of <Dir work>>], None), <Dir tests>: ([<bound method Node.teardown of <Dir tests>>], None), <Dir end2end>: ([<bound method Node.teardown of <Dir end2end>>], None), <Module test_invocations.py>: ([<bound method Node.teardown of <Module test_invocations.py>>], None)})
RonnyPfannschmidt commented 1 month ago

Let's forbid getfixturevalue to trigger setup after the fixture was yielded

The-Compiler commented 1 month ago

As in, make it error out explicitly if we're in the teardown phase and request a fixture that has not already been requested before? Makes sense I'd say.

The-Compiler commented 1 month ago

Oh, though I'm not sure how much existing usage it'd break - evidently there are cases where this kind of thing still seems to work out currently? Maybe we should deprecate it first or something (plus improve the error message if it happens to hard-fail)?

RonnyPfannschmidt commented 1 month ago

We can start with deprecating and replace it with a exception later