retain-on-failure discards trace/video if teardown/setup fails in fixtures

[dmeecs]

From what I can see, if a fixture fails in setup/teardown, the pytest --output .out --tracing retain-on-failure discards the trace. And at least sometimes the user will want the trace to be kept.

The code only looks at whether the rep_call has failed. https://github.com/microsoft/playwright-pytest/blob/main/pytest_playwright/pytest_playwright.py#L224-L226

But that ignores issues in the setup/teardown, e.g. pytests the example code

# content of conftest.py

import pytest


@pytest.hookimpl(tryfirst=True, hookwrapper=True)
def pytest_runtest_makereport(item, call):
    # execute all other hooks to obtain the report object
    outcome = yield
    rep = outcome.get_result()

    # set a report attribute for each phase of a call, which can
    # be "setup", "call", "teardown"

    setattr(item, "rep_" + rep.when, rep)


@pytest.fixture
def something(request):
    yield
    # request.node is an "item" because we use the default
    # "function" scope
    if request.node.rep_setup.failed:
        print("setting up a test failed!", request.node.nodeid)
    elif request.node.rep_setup.passed:
        if request.node.rep_call.failed:
            print("executing test failed", request.node.nodeid)

So if a user writes a fixture, that does something with a page in it's setup or teardown, and that setup or teardown code fails, the trace/video will be discarded. That's because at this point pytest will set rep_setup.failed (or rep_teardown) to be true, rather than rep_call.failed, I believe. Docs for the runtest_protocol

@pytest.fixture()
def logged_in(page: Page) -> Page:
    """Log in"""
    log_in_flow(page)
    yield page
    log_out_flow(page)

[mxschmitt]

Thank you for your bug report!

Do you have a solution in mind how to solve it? Because currently we create e.g. tracing after the yield of the context fixture. So the teardown error of the "problematic" fixture happens after that.

[dmeecs]

Do I have a solution? Not really. My initial thoughts were altering the line to include rep_setup.failed/rep_teardown.failed:

https://github.com/microsoft/playwright-pytest/blob/main/pytest_playwright/pytest_playwright.py#L226

So this

    # If requst.node is missing rep_call, then some error happened during execution
    # that prevented teardown, but should still be counted as a failure
    failed = request.node.rep_call.failed if hasattr(request.node, "rep_call") else True

would change to something like this:

    # If requst.node is missing rep_call, then some error happened during execution
    # that prevented teardown, but should still be counted as a failure
    failed = any(
        [request.node.rep_call.failed if hasattr(request.node, "rep_call") else True],
        [request.node.rep_setup.failed if hasattr(request.node, "rep_setup") else True],
        [request.node.rep_teardown.failed if hasattr(request.node, "rep_teardown") else True],
    )

Sorry, just a quick guess at where I might alter the code. I'm not familiar with exactly how pytest alter's the request object. But I was hoping maybe just expanding the retain-on-failure to include errors during setup/teardown.

[mxschmitt]

I tried that, but it fails as of today after the context was closed. So it won't work. We'd need a different point where we hook in and close the context and manipulate fixture teardown order.

[storenth]

@mxschmitt Looks like this is more general problem: any test artifacts relay on failed = request.node.rep_call.failed if hasattr(request.node, "rep_call") else True logic under the hood. So we miss rep_setup/rep_teardown that's it.

[mxschmitt]

Hi Thanks! Are you interested in contributing a fix with a test?

[storenth]

yes, I can give a shot)

[storenth]

@mxschmitt I have tried a lot of things using TDD. Also I found related bug on pytest pytest-dev/pytest#9909. For now setup phase fixed, but teardown doesn't even know about its state (failed or passed) because of context implementation. I create draft see #207 to show off some stuff (look at test_artifacts_on_teardown). So, I suggest one of the next solution vector:

1. save artifacts, then check the final test state under pytest_runtest_makereport and if teardown was passed, remove the output (overhead)
2. get n parse the trace (I got the needed info under teardown_exact phase see attached pic, but don't know how to get one, ideas welcome Max) to figure out is there exception or not to set the special flag (failed_xteardown via config.teardown) for context to process or not screenshots, etc.

[storenth]

@mxschmitt I solved all the problems above, can you review?