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

Pytest aborts when fixture errors during teardown and --maxfail=1 #11706

Closed
4 tasks done
bbrown1867 opened this issue Dec 14, 2023 · 9 comments · Fixed by #11721 or #12279
Closed
4 tasks done

Pytest aborts when fixture errors during teardown and --maxfail=1 #11706

bbrown1867 opened this issue Dec 14, 2023 · 9 comments · Fixed by #11721 or #12279
Labels
type: bug problem that needs to be addressed

Comments

@bbrown1867
Copy link
Contributor

bbrown1867 commented Dec 14, 2023

  • a detailed description of the bug or problem you are having
  • output of pip list from the virtual environment you are using
  • pytest and operating system versions
  • minimal example if possible

Description

Pytest aborts if a session-scoped fixture errors during teardown when (a) one or more test cases fail and (b) --maxfail=1 is used.

When this happens, test-report.xml does not contain the teardown fixture error. This is problematic as users may want to also record teardown fixture errors when their test cases fail.

Minimal Example

Code

import pytest


def some_condition() -> bool:
    return True


@pytest.fixture(scope="session", autouse=True)
def my_end_of_session_check():
    yield

    print("Running end of session check!")
    if some_condition():
        pytest.fail("Oh noes")


def test_case_1():
    pytest.fail("This is a failing test")


def test_case_2():
     pytest.fail("This is also a failing test")

Args

pytest example.py --maxfail=1 --junitxml=test-report.xml

Output

Console:

============================================================================== test session starts ===============================================================================
platform darwin -- Python 3.8.13, pytest-7.4.3, pluggy-1.3.0
rootdir: /Users/bbrown/Downloads
collected 2 items

example.py FRunning end of session check!
Traceback (most recent call last):
  File "/Users/bbrown/Downloads/.venv/bin/pytest", line 8, in <module>
    sys.exit(console_main())
  <clipped>
  File "/Users/bbrown/Downloads/.venv/lib/python3.8/site-packages/_pytest/fixtures.py", line 911, in _teardown_yield_fixture
    next(it)
  File "/Users/bbrown/Downloads/example.py", line 14, in my_end_of_session_check
    pytest.fail("Oh noes")
  File "/Users/bbrown/Downloads/.venv/lib/python3.8/site-packages/_pytest/outcomes.py", line 198, in fail
    raise Failed(msg=reason, pytrace=pytrace)
Failed: Oh noes

test-report.xml:

<?xml version="1.0" encoding="utf-8"?><testsuites><testsuite name="pytest" errors="0" failures="1" skipped="0" tests="1" time="0.035" timestamp="2023-12-15T08:56:19.980572" hostname="macbookpro"><testcase classname="example" name="test_case_1" time="0.001"><failure message="Failed: This is a failing test">def test_case_1():
&gt;       pytest.fail("This is a failing test")
E       Failed: This is a failing test

example.py:18: Failed</failure></testcase></testsuite></testsuites>

Expected Output

I expect pytest to not abort, and report "1 failed, 1 error, 1 skipped" in both the console and test-report.xml when this happens.

Environment

OS:

macOS Sonoma 14.1.2

Virtual environment:

(.venv) % pip list
Package        Version
-------------- -------
exceptiongroup 1.2.0
iniconfig      2.0.0
packaging      23.2
pip            23.3.1
pluggy         1.3.0
pytest         7.4.3
setuptools     56.0.0
tomli          2.0.1
@bbrown1867 bbrown1867 changed the title Test case failure + session-scoped fixture teardown error with --maxfail=1 leads to pytest aborting Pytest aborts when session-scoped fixture errors during teardown and --maxfail=1 Dec 15, 2023
@bbrown1867
Copy link
Contributor Author

Note that this also happens when the fixture is module scoped, but does not happen if the fixture is function scoped. Will update the issue name to indicate this.

@bbrown1867 bbrown1867 changed the title Pytest aborts when session-scoped fixture errors during teardown and --maxfail=1 Pytest aborts when fixture errors during teardown and --maxfail=1 Dec 15, 2023
@bluetech bluetech added the type: bug problem that needs to be addressed label Dec 16, 2023
@bluetech
Copy link
Member

This is an interesting edge case you've stumbled upon.

Background

pytest's setups/teardowns are managed by the SetupState class.

You can see the docstring for how it works, but the important part here is that we're tearing down some item (test), we look at the next item (nextitem), see which fixtures the nextitem won't be needing, and tear them down in the context of item. In particular, the last item gets nextitem = None which tears down everything.

When --maxfail=N is used (or -x which is alias for --maxfail=1), if the N failures are reached, the runner sets session.shouldfail = ..., then the runner loop checks it and exits the loop.

Problem

Let's call the Nth failing test "test N" and assume there is at least one test after it, "test N + 1", which shares some higher-scoped fixtures.

"test N" is torn down with nextitem = test N + 1, then shouldfail is set and the loop exits. But, all of the remaining fixtures weren't torn down because nothing told them to.

To fix this, pytest in a last ditch effort runs the remaining teardowns in a pytest_sessionfinish hook.

But this pytest_sessionfinish hook currently is not written to handle exceptions, except for pytest.exit, even though it executes user code. So if one of the remaining teardowns raises, the exception just propagates.

Possible solution 1

Catch the exceptions from the pytest_sessionfinish and issue some warning.

This would work, however then the teardown failures aren't reported in the context of any test. In particular they won't show in any xml reports and such.

Possible solution 2

Detect when an item had set shouldfail = True, then instead of tearing it down with nextitem = item N+1, tear it down with nextitem = None.

This would work and run the teardowns in the context of the test, but is a bit dangerous - doesn't handle the case where plugins reset shouldfail. Also, I think there are other cases where we bail early, those would still need pytest_sessionfinish and the problem would still be possible there.

@bbrown1867
Copy link
Contributor Author

Thanks for the detailed reply @bluetech!

The real pain point here is the lack of reporting - my team's observability tools consume test-report.xml and want all errors/failures to be reported such that we can triage them. Because of that, I'll try to investigate Possible solution 2 you've outlined above.

@bbrown1867
Copy link
Contributor Author

bbrown1867 commented Dec 18, 2023

Detect when an item had set shouldfail = True, then instead of tearing it down with nextitem = item N+1, tear it down with nextitem = None.

This was pretty easy to implement, basically a one-line change in pytest_runtest_teardown:

def pytest_runtest_teardown(item: Item, nextitem: Optional[Item]) -> None:
    _update_current_test_var(item, "teardown")
    nextitem = None if item.session.shouldfail else nextitem
    item.session._setupstate.teardown_exact(nextitem)
    _update_current_test_var(item, None)

I also added a unittest in test_runner.py mimicking the sample code above. I can open a PR with this change, however I'm not sure what to do about these comments:

This would work and run the teardowns in the context of the test, but is a bit dangerous - doesn't handle the case where plugins reset shouldfail.

A plugin resetting shouldfail feels like even more of an edge-case, not sure if we should be that concerned about it?

Also, I think there are other cases where we bail early, those would still need pytest_sessionfinish and the problem would still be possible there.

I'm not really following this comment, likely just due to my lack of context about the codebase.

bluetech added a commit to bluetech/pytest that referenced this issue Feb 23, 2024
…1721)"

Fix pytest-dev#12021.
Reopens pytest-dev#11706.

This reverts commit 12b9bd5.

This change caused a bad regression in pytest-xdist:
pytest-dev/pytest-xdist#1024

pytest-xdist necessarily has special handling of `--maxfail` and session
fixture teardown get executed multiple times with the change.

Since I'm not sure how to adapt pytest-xdist myself, revert for now.

I kept the sticky `shouldstop`/`shouldfail` changes as they are good
ideas regardless I think.
@bluetech
Copy link
Member

Reopening since the fix was reverted.

@bbrown1867
Copy link
Contributor Author

@bluetech do you think we can "revert the revert" (93cd7ba) now that we have a fix in pytest-xdist? Or will we need to wait for a release of pytest-xdist?

@bluetech
Copy link
Member

bluetech commented Apr 6, 2024

Let's wait for a pytest-xdist release as there is no rush on the pytest side (there's probably some time until we release 8.2).

I think we will do a pytest-xdist release soon (once we tie up so some typing changes there). I will notify you when we can reapply the fix here (unless I forget...).

I do foresee we'll get some complaints from users who will be using new pytest with old pytest-xdist, since we can't use dependency constraints to prevent this scenario, but that's OK.

flying-sheep pushed a commit to flying-sheep/pytest that referenced this issue Apr 9, 2024
…1721)"

Fix pytest-dev#12021.
Reopens pytest-dev#11706.

This reverts commit 12b9bd5.

This change caused a bad regression in pytest-xdist:
pytest-dev/pytest-xdist#1024

pytest-xdist necessarily has special handling of `--maxfail` and session
fixture teardown get executed multiple times with the change.

Since I'm not sure how to adapt pytest-xdist myself, revert for now.

I kept the sticky `shouldstop`/`shouldfail` changes as they are good
ideas regardless I think.
@bluetech
Copy link
Member

I think we will do a pytest-xdist release soon (once we tie up so some typing changes there). I will notify you when we can reapply the fix here (unless I forget...).

@bbrown1867 We has some issues with the 3.6.0 xdist release which had to be yanked, but I released 3.6.1 now and hopefully it'd be fine. So we can proceed on the pytest side.

@bbrown1867
Copy link
Contributor Author

Okay sounds good, I'll open a PR to revert 4cc87ad

bluetech pushed a commit that referenced this issue May 2, 2024
…2279)

Closes #11706.

Originally fixed in #11721, but then reverted in #12022 due to a regression in pytest-xdist.

The regression was fixed on the pytest-xdist side in pytest-dev/pytest-xdist#1026.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug problem that needs to be addressed
Projects
None yet
2 participants