From f38bf3dbd30a4204e3a5b96fab9575a296b69501 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Wed, 7 Oct 2026 15:42:53 -0700 Subject: [PATCH 1/2] fix: keep artifacts when a fixture fails during teardown With --tracing/--video retain-on-failure or --screenshot only-on-failure, artifacts were discarded when a user fixture (e.g. one that uses `page`) failed during teardown: the recorder decided in its own fixture teardown, which runs before such failures are reported. Videos are now copied out of Playwright in the fixture teardown, and when the test has not failed yet, keeping or discarding the artifacts is deferred to pytest_runtest_makereport for the teardown report. The session temp folder is now cleaned up at the end of the run, since the last test's teardown report comes after session fixtures are torn down. Fixes #117 --- .../pytest_playwright.py | 102 ++++++++++++------ .../pytest_playwright/pytest_playwright.py | 100 +++++++++++------ tests/test_asyncio.py | 57 ++++++++++ tests/test_sync.py | 53 +++++++++ 4 files changed, 249 insertions(+), 63 deletions(-) diff --git a/pytest-playwright-asyncio/pytest_playwright_asyncio/pytest_playwright.py b/pytest-playwright-asyncio/pytest_playwright_asyncio/pytest_playwright.py index 7e41a37..0df068b 100644 --- a/pytest-playwright-asyncio/pytest_playwright_asyncio/pytest_playwright.py +++ b/pytest-playwright-asyncio/pytest_playwright_asyncio/pytest_playwright.py @@ -33,6 +33,7 @@ Optional, Protocol, Sequence, + Tuple, Union, Pattern, cast, @@ -66,18 +67,27 @@ import pytest_asyncio from slugify import slugify import tempfile +import traceback @pytest.fixture(scope="session") -def _pw_artifacts_folder() -> Generator[tempfile.TemporaryDirectory, None, None]: +def _pw_artifacts_folder( + pytestconfig: Any, +) -> Generator[tempfile.TemporaryDirectory, None, None]: artifacts_folder = tempfile.TemporaryDirectory(prefix="playwright-pytest-") yield artifacts_folder - try: - # On Windows, files can be still in use. - # https://github.com/microsoft/playwright-pytest/issues/163 - artifacts_folder.cleanup() - except (PermissionError, NotADirectoryError): - pass + + def cleanup() -> None: + try: + # On Windows, files can be still in use. + # https://github.com/microsoft/playwright-pytest/issues/163 + artifacts_folder.cleanup() + except (PermissionError, NotADirectoryError): + pass + + # Session fixtures are torn down during the last test's teardown, before its + # artifacts are moved out of this folder in pytest_runtest_makereport. + pytestconfig.add_cleanup(cleanup) @pytest.fixture(scope="session", autouse=True) @@ -169,6 +179,11 @@ def pytest_configure(config: Any) -> None: ) +# Set on a test item when keeping or discarding its artifacts has to wait for +# the teardown report, see _artifacts_recorder. +_PENDING_ARTIFACTS_RECORDER_ATTR = "_playwright_pending_artifacts_recorder" + + # Making test result information available in fixtures # https://docs.pytest.org/en/latest/example/simple.html#making-test-result-information-available-in-fixtures @pytest.hookimpl(tryfirst=True, hookwrapper=True) @@ -182,6 +197,18 @@ def pytest_runtest_makereport(item: Any) -> Generator[None, Any, None]: setattr(item, "rep_" + rep.when, rep) + if rep.when == "teardown": + artifacts_recorder = getattr(item, _PENDING_ARTIFACTS_RECORDER_ATTR, None) + if artifacts_recorder is not None: + delattr(item, _PENDING_ARTIFACTS_RECORDER_ATTR) + try: + artifacts_recorder.did_finish_test(rep.failed) + except Exception: + # Report it as a teardown error of this test instead of letting it + # escape the hook as an INTERNALERROR that aborts the session. + rep.outcome = "failed" + rep.longrepr = traceback.format_exc() + def _get_skiplist(item: Any, values: List[str], value_name: str) -> List[str]: skipped_values: List[str] = [] @@ -316,10 +343,17 @@ async def _artifacts_recorder( pytestconfig, request, output_path, playwright, _pw_artifacts_folder ) yield artifacts_recorder + await artifacts_recorder.collect_videos() # If request.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 - await artifacts_recorder.did_finish_test(failed) + if failed: + artifacts_recorder.did_finish_test(failed) + else: + # Fixtures torn down after this one (e.g. a user fixture that uses `page`) + # can still fail the test, so decide in pytest_runtest_makereport once + # the teardown report exists. + setattr(request.node, _PENDING_ARTIFACTS_RECORDER_ATTR, artifacts_recorder) @pytest_asyncio.fixture(scope="session") @@ -650,6 +684,8 @@ def __init__( self._all_pages: List[Page] = [] self._screenshots: List[str] = [] self._traces: List[str] = [] + # (temporary path, file name in the output folder) + self._videos: List[Tuple[str, str]] = [] self._tracing_option = pytestconfig.getoption("--tracing") self._capture_trace = self._tracing_option in ["on", "retain-on-failure"] @@ -659,7 +695,7 @@ def _build_artifact_test_folder(self, folder_or_file_name: str) -> str: _truncate_file_name(folder_or_file_name), ) - async def did_finish_test(self, failed: bool) -> None: + def did_finish_test(self, failed: bool) -> None: screenshot_option = self._pytestconfig.getoption("--screenshot") capture_screenshot = screenshot_option == "on" or ( failed and screenshot_option == "only-on-failure" @@ -694,32 +730,34 @@ async def did_finish_test(self, failed: bool) -> None: preserve_video = video_option == "on" or ( failed and video_option == "retain-on-failure" ) - if preserve_video: - for index, page in enumerate(self._all_pages): + for video, video_file_name in self._videos: + if preserve_video: + video_path = self._build_artifact_test_folder(video_file_name) + os.makedirs(os.path.dirname(video_path), exist_ok=True) + shutil.move(video, video_path) + else: + os.remove(video) + + async def collect_videos(self) -> None: + # Videos are only complete once their context is closed. Copy them out of + # Playwright now, so that keeping or discarding them later is file work only. + if self._pytestconfig.getoption("--video") not in ["on", "retain-on-failure"]: + return + for index, page in enumerate(self._all_pages): + try: video = page.video if not video: continue - try: - video_file_name = ( - "video.webm" - if len(self._all_pages) == 1 - else f"video-{index + 1}.webm" - ) - await video.save_as( - path=self._build_artifact_test_folder(video_file_name) - ) - except Error: - # Silent catch empty videos. - pass - else: - for page in self._all_pages: - # Can be changed to "if page.video" without try/except once https://github.com/microsoft/playwright-python/pull/2410 is released and widely adopted. - if video_option in ["on", "retain-on-failure"]: - try: - if page.video: - await page.video.delete() - except Error: - pass + video_path = Path(self._pw_artifacts_folder.name) / _create_guid() + await video.save_as(path=video_path) + await video.delete() + except Error: + # Silent catch empty videos. + continue + video_file_name = ( + "video.webm" if len(self._all_pages) == 1 else f"video-{index + 1}.webm" + ) + self._videos.append((str(video_path), video_file_name)) async def on_did_create_browser_context(self, context: BrowserContext) -> None: context.on("page", lambda page: self._all_pages.append(page)) diff --git a/pytest-playwright/pytest_playwright/pytest_playwright.py b/pytest-playwright/pytest_playwright/pytest_playwright.py index 141f1e6..3f0d208 100644 --- a/pytest-playwright/pytest_playwright/pytest_playwright.py +++ b/pytest-playwright/pytest_playwright/pytest_playwright.py @@ -31,6 +31,7 @@ Optional, Protocol, Sequence, + Tuple, Union, Pattern, cast, @@ -63,18 +64,27 @@ ) from slugify import slugify import tempfile +import traceback @pytest.fixture(scope="session") -def _pw_artifacts_folder() -> Generator[tempfile.TemporaryDirectory, None, None]: +def _pw_artifacts_folder( + pytestconfig: Any, +) -> Generator[tempfile.TemporaryDirectory, None, None]: artifacts_folder = tempfile.TemporaryDirectory(prefix="playwright-pytest-") yield artifacts_folder - try: - # On Windows, files can be still in use. - # https://github.com/microsoft/playwright-pytest/issues/163 - artifacts_folder.cleanup() - except (PermissionError, NotADirectoryError): - pass + + def cleanup() -> None: + try: + # On Windows, files can be still in use. + # https://github.com/microsoft/playwright-pytest/issues/163 + artifacts_folder.cleanup() + except (PermissionError, NotADirectoryError): + pass + + # Session fixtures are torn down during the last test's teardown, before its + # artifacts are moved out of this folder in pytest_runtest_makereport. + pytestconfig.add_cleanup(cleanup) @pytest.fixture(scope="session", autouse=True) @@ -166,6 +176,11 @@ def pytest_configure(config: Any) -> None: ) +# Set on a test item when keeping or discarding its artifacts has to wait for +# the teardown report, see _artifacts_recorder. +_PENDING_ARTIFACTS_RECORDER_ATTR = "_playwright_pending_artifacts_recorder" + + # Making test result information available in fixtures # https://docs.pytest.org/en/latest/example/simple.html#making-test-result-information-available-in-fixtures @pytest.hookimpl(tryfirst=True, hookwrapper=True) @@ -179,6 +194,18 @@ def pytest_runtest_makereport(item: Any) -> Generator[None, Any, None]: setattr(item, "rep_" + rep.when, rep) + if rep.when == "teardown": + artifacts_recorder = getattr(item, _PENDING_ARTIFACTS_RECORDER_ATTR, None) + if artifacts_recorder is not None: + delattr(item, _PENDING_ARTIFACTS_RECORDER_ATTR) + try: + artifacts_recorder.did_finish_test(rep.failed) + except Exception: + # Report it as a teardown error of this test instead of letting it + # escape the hook as an INTERNALERROR that aborts the session. + rep.outcome = "failed" + rep.longrepr = traceback.format_exc() + def _get_skiplist(item: Any, values: List[str], value_name: str) -> List[str]: skipped_values: List[str] = [] @@ -313,10 +340,17 @@ def _artifacts_recorder( pytestconfig, request, output_path, playwright, _pw_artifacts_folder ) yield artifacts_recorder + artifacts_recorder.collect_videos() # If request.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 - artifacts_recorder.did_finish_test(failed) + if failed: + artifacts_recorder.did_finish_test(failed) + else: + # Fixtures torn down after this one (e.g. a user fixture that uses `page`) + # can still fail the test, so decide in pytest_runtest_makereport once + # the teardown report exists. + setattr(request.node, _PENDING_ARTIFACTS_RECORDER_ATTR, artifacts_recorder) @pytest.fixture(scope="session") @@ -647,6 +681,8 @@ def __init__( self._all_pages: List[Page] = [] self._screenshots: List[str] = [] self._traces: List[str] = [] + # (temporary path, file name in the output folder) + self._videos: List[Tuple[str, str]] = [] self._tracing_option = pytestconfig.getoption("--tracing") self._capture_trace = self._tracing_option in ["on", "retain-on-failure"] @@ -691,32 +727,34 @@ def did_finish_test(self, failed: bool) -> None: preserve_video = video_option == "on" or ( failed and video_option == "retain-on-failure" ) - if preserve_video: - for index, page in enumerate(self._all_pages): + for video, video_file_name in self._videos: + if preserve_video: + video_path = self._build_artifact_test_folder(video_file_name) + os.makedirs(os.path.dirname(video_path), exist_ok=True) + shutil.move(video, video_path) + else: + os.remove(video) + + def collect_videos(self) -> None: + # Videos are only complete once their context is closed. Copy them out of + # Playwright now, so that keeping or discarding them later is file work only. + if self._pytestconfig.getoption("--video") not in ["on", "retain-on-failure"]: + return + for index, page in enumerate(self._all_pages): + try: video = page.video if not video: continue - try: - video_file_name = ( - "video.webm" - if len(self._all_pages) == 1 - else f"video-{index + 1}.webm" - ) - video.save_as( - path=self._build_artifact_test_folder(video_file_name) - ) - except Error: - # Silent catch empty videos. - pass - else: - for page in self._all_pages: - # Can be changed to "if page.video" without try/except once https://github.com/microsoft/playwright-python/pull/2410 is released and widely adopted. - if video_option in ["on", "retain-on-failure"]: - try: - if page.video: - page.video.delete() - except Error: - pass + video_path = Path(self._pw_artifacts_folder.name) / _create_guid() + video.save_as(path=video_path) + video.delete() + except Error: + # Silent catch empty videos. + continue + video_file_name = ( + "video.webm" if len(self._all_pages) == 1 else f"video-{index + 1}.webm" + ) + self._videos.append((str(video_path), video_file_name)) def on_did_create_browser_context(self, context: BrowserContext) -> None: context.on("page", lambda page: self._all_pages.append(page)) diff --git a/tests/test_asyncio.py b/tests/test_asyncio.py index 3af3913..8cb3abc 100644 --- a/tests/test_asyncio.py +++ b/tests/test_asyncio.py @@ -938,6 +938,63 @@ async def test_failing(page): ) +def test_artifacts_retain_on_failure_when_fixture_fails( + testdir: pytest.Testdir, +) -> None: + # https://github.com/microsoft/playwright-pytest/issues/117 + testdir.makepyfile( + """ + import pytest + import pytest_asyncio + + @pytest_asyncio.fixture + async def failing_setup(page): + await page.goto("data:text/html,
setup
") + raise Exception("setup failed") + + @pytest_asyncio.fixture + async def failing_teardown(page): + yield page + raise Exception("teardown failed") + + @pytest.mark.asyncio + async def test_passing(page): + assert 2 == await page.evaluate("1 + 1") + + @pytest.mark.asyncio + async def test_setup_fails(failing_setup): + pass + + @pytest.mark.asyncio + async def test_teardown_fails(failing_teardown): + await failing_teardown.goto("data:text/html,
teardown
") + """ + ) + result = testdir.runpytest( + "--screenshot", + "only-on-failure", + "--video", + "retain-on-failure", + "--tracing", + "retain-on-failure", + ) + result.assert_outcomes(passed=2, errors=2) + test_results_dir = os.path.join(testdir.tmpdir, "test-results") + _assert_folder_structure( + test_results_dir, + """ +- test-artifacts-retain-on-failure-when-fixture-fails-py-test-setup-fails-chromium: + - test-failed-1.png + - trace.zip + - video.webm +- test-artifacts-retain-on-failure-when-fixture-fails-py-test-teardown-fails-chromium: + - test-failed-1.png + - trace.zip + - video.webm +""", + ) + + def test_should_work_with_test_names_which_exceeds_256_characters( testdir: pytest.Testdir, ) -> None: diff --git a/tests/test_sync.py b/tests/test_sync.py index 0ecaa70..7e6fea0 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -933,6 +933,59 @@ def test_failing(page): ) +def test_artifacts_retain_on_failure_when_fixture_fails( + testdir: pytest.Testdir, +) -> None: + # https://github.com/microsoft/playwright-pytest/issues/117 + testdir.makepyfile( + """ + import pytest + + @pytest.fixture + def failing_setup(page): + page.goto("data:text/html,
setup
") + raise Exception("setup failed") + + @pytest.fixture + def failing_teardown(page): + yield page + raise Exception("teardown failed") + + def test_passing(page): + assert 2 == page.evaluate("1 + 1") + + def test_setup_fails(failing_setup): + pass + + def test_teardown_fails(failing_teardown): + failing_teardown.goto("data:text/html,
teardown
") + """ + ) + result = testdir.runpytest( + "--screenshot", + "only-on-failure", + "--video", + "retain-on-failure", + "--tracing", + "retain-on-failure", + ) + result.assert_outcomes(passed=2, errors=2) + test_results_dir = os.path.join(testdir.tmpdir, "test-results") + _assert_folder_structure( + test_results_dir, + """ +- test-artifacts-retain-on-failure-when-fixture-fails-py-test-setup-fails-chromium: + - test-failed-1.png + - trace.zip + - video.webm +- test-artifacts-retain-on-failure-when-fixture-fails-py-test-teardown-fails-chromium: + - test-failed-1.png + - trace.zip + - video.webm +""", + ) + + def test_should_work_with_test_names_which_exceeds_256_characters( testdir: pytest.Testdir, ) -> None: From 7cc6e8f01d0fd912f54f76e18af794d26f7063d8 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Wed, 7 Oct 2026 17:52:33 -0700 Subject: [PATCH 2/2] review: run the makereport wrapper trylast, don't keep artifacts of tests skipped from a fixture, keep the fixture's own teardown error, test the artifact error path --- .../pytest_playwright.py | 50 ++++++++++----- .../pytest_playwright/pytest_playwright.py | 50 ++++++++++----- tests/test_asyncio.py | 61 ++++++++++++++++++- tests/test_sync.py | 57 ++++++++++++++++- 4 files changed, 188 insertions(+), 30 deletions(-) diff --git a/pytest-playwright-asyncio/pytest_playwright_asyncio/pytest_playwright.py b/pytest-playwright-asyncio/pytest_playwright_asyncio/pytest_playwright.py index 0df068b..eeffb1f 100644 --- a/pytest-playwright-asyncio/pytest_playwright_asyncio/pytest_playwright.py +++ b/pytest-playwright-asyncio/pytest_playwright_asyncio/pytest_playwright.py @@ -181,12 +181,17 @@ def pytest_configure(config: Any) -> None: # Set on a test item when keeping or discarding its artifacts has to wait for # the teardown report, see _artifacts_recorder. -_PENDING_ARTIFACTS_RECORDER_ATTR = "_playwright_pending_artifacts_recorder" +_ARTIFACTS_RECORDER_ATTR = "_playwright_artifacts_recorder" # Making test result information available in fixtures # https://docs.pytest.org/en/latest/example/simple.html#making-test-result-information-available-in-fixtures -@pytest.hookimpl(tryfirst=True, hookwrapper=True) +# +# trylast: the code after `yield` of a trylast wrapper runs before that of the +# other wrappers, so pytest_runtest_makereport hooks in conftest.py and in other +# plugins see the teardown outcome set below. (A tryfirst wrapper elsewhere that +# changes rep.outcome after this point is not taken into account.) +@pytest.hookimpl(trylast=True, hookwrapper=True) def pytest_runtest_makereport(item: Any) -> Generator[None, Any, None]: # execute all other hooks to obtain the report object outcome = yield @@ -198,16 +203,32 @@ def pytest_runtest_makereport(item: Any) -> Generator[None, Any, None]: setattr(item, "rep_" + rep.when, rep) if rep.when == "teardown": - artifacts_recorder = getattr(item, _PENDING_ARTIFACTS_RECORDER_ATTR, None) + artifacts_recorder = getattr(item, _ARTIFACTS_RECORDER_ATTR, None) if artifacts_recorder is not None: - delattr(item, _PENDING_ARTIFACTS_RECORDER_ATTR) + delattr(item, _ARTIFACTS_RECORDER_ATTR) try: artifacts_recorder.did_finish_test(rep.failed) except Exception: # Report it as a teardown error of this test instead of letting it - # escape the hook as an INTERNALERROR that aborts the session. - rep.outcome = "failed" - rep.longrepr = traceback.format_exc() + # escape the hook as an INTERNALERROR that aborts the session. A + # fixture's own teardown error stays the main one. + error = traceback.format_exc() + if rep.failed: + rep.sections.append(("Playwright artifacts error", error)) + else: + rep.outcome = "failed" + rep.longrepr = error + + +def _test_failed_before_teardown(item: Any) -> bool: + rep_call = getattr(item, "rep_call", None) + if rep_call is not None: + return rep_call.failed + # No call report: the setup failed, the test was skipped from a fixture, or + # the run was interrupted (then there is no teardown report either, so the + # artifacts have to be kept right away). + rep_setup = getattr(item, "rep_setup", None) + return rep_setup is None or not rep_setup.skipped def _get_skiplist(item: Any, values: List[str], value_name: str) -> List[str]: @@ -344,16 +365,13 @@ async def _artifacts_recorder( ) yield artifacts_recorder await artifacts_recorder.collect_videos() - # If request.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 - if failed: - artifacts_recorder.did_finish_test(failed) + if _test_failed_before_teardown(request.node): + artifacts_recorder.did_finish_test(failed=True) else: # Fixtures torn down after this one (e.g. a user fixture that uses `page`) # can still fail the test, so decide in pytest_runtest_makereport once # the teardown report exists. - setattr(request.node, _PENDING_ARTIFACTS_RECORDER_ATTR, artifacts_recorder) + setattr(request.node, _ARTIFACTS_RECORDER_ATTR, artifacts_recorder) @pytest_asyncio.fixture(scope="session") @@ -750,7 +768,6 @@ async def collect_videos(self) -> None: continue video_path = Path(self._pw_artifacts_folder.name) / _create_guid() await video.save_as(path=video_path) - await video.delete() except Error: # Silent catch empty videos. continue @@ -758,6 +775,11 @@ async def collect_videos(self) -> None: "video.webm" if len(self._all_pages) == 1 else f"video-{index + 1}.webm" ) self._videos.append((str(video_path), video_file_name)) + try: + await video.delete() + except Error: + # The copy is what gets kept; removing Playwright's file is best effort. + pass async def on_did_create_browser_context(self, context: BrowserContext) -> None: context.on("page", lambda page: self._all_pages.append(page)) diff --git a/pytest-playwright/pytest_playwright/pytest_playwright.py b/pytest-playwright/pytest_playwright/pytest_playwright.py index 3f0d208..05b7d01 100644 --- a/pytest-playwright/pytest_playwright/pytest_playwright.py +++ b/pytest-playwright/pytest_playwright/pytest_playwright.py @@ -178,12 +178,17 @@ def pytest_configure(config: Any) -> None: # Set on a test item when keeping or discarding its artifacts has to wait for # the teardown report, see _artifacts_recorder. -_PENDING_ARTIFACTS_RECORDER_ATTR = "_playwright_pending_artifacts_recorder" +_ARTIFACTS_RECORDER_ATTR = "_playwright_artifacts_recorder" # Making test result information available in fixtures # https://docs.pytest.org/en/latest/example/simple.html#making-test-result-information-available-in-fixtures -@pytest.hookimpl(tryfirst=True, hookwrapper=True) +# +# trylast: the code after `yield` of a trylast wrapper runs before that of the +# other wrappers, so pytest_runtest_makereport hooks in conftest.py and in other +# plugins see the teardown outcome set below. (A tryfirst wrapper elsewhere that +# changes rep.outcome after this point is not taken into account.) +@pytest.hookimpl(trylast=True, hookwrapper=True) def pytest_runtest_makereport(item: Any) -> Generator[None, Any, None]: # execute all other hooks to obtain the report object outcome = yield @@ -195,16 +200,32 @@ def pytest_runtest_makereport(item: Any) -> Generator[None, Any, None]: setattr(item, "rep_" + rep.when, rep) if rep.when == "teardown": - artifacts_recorder = getattr(item, _PENDING_ARTIFACTS_RECORDER_ATTR, None) + artifacts_recorder = getattr(item, _ARTIFACTS_RECORDER_ATTR, None) if artifacts_recorder is not None: - delattr(item, _PENDING_ARTIFACTS_RECORDER_ATTR) + delattr(item, _ARTIFACTS_RECORDER_ATTR) try: artifacts_recorder.did_finish_test(rep.failed) except Exception: # Report it as a teardown error of this test instead of letting it - # escape the hook as an INTERNALERROR that aborts the session. - rep.outcome = "failed" - rep.longrepr = traceback.format_exc() + # escape the hook as an INTERNALERROR that aborts the session. A + # fixture's own teardown error stays the main one. + error = traceback.format_exc() + if rep.failed: + rep.sections.append(("Playwright artifacts error", error)) + else: + rep.outcome = "failed" + rep.longrepr = error + + +def _test_failed_before_teardown(item: Any) -> bool: + rep_call = getattr(item, "rep_call", None) + if rep_call is not None: + return rep_call.failed + # No call report: the setup failed, the test was skipped from a fixture, or + # the run was interrupted (then there is no teardown report either, so the + # artifacts have to be kept right away). + rep_setup = getattr(item, "rep_setup", None) + return rep_setup is None or not rep_setup.skipped def _get_skiplist(item: Any, values: List[str], value_name: str) -> List[str]: @@ -341,16 +362,13 @@ def _artifacts_recorder( ) yield artifacts_recorder artifacts_recorder.collect_videos() - # If request.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 - if failed: - artifacts_recorder.did_finish_test(failed) + if _test_failed_before_teardown(request.node): + artifacts_recorder.did_finish_test(failed=True) else: # Fixtures torn down after this one (e.g. a user fixture that uses `page`) # can still fail the test, so decide in pytest_runtest_makereport once # the teardown report exists. - setattr(request.node, _PENDING_ARTIFACTS_RECORDER_ATTR, artifacts_recorder) + setattr(request.node, _ARTIFACTS_RECORDER_ATTR, artifacts_recorder) @pytest.fixture(scope="session") @@ -747,7 +765,6 @@ def collect_videos(self) -> None: continue video_path = Path(self._pw_artifacts_folder.name) / _create_guid() video.save_as(path=video_path) - video.delete() except Error: # Silent catch empty videos. continue @@ -755,6 +772,11 @@ def collect_videos(self) -> None: "video.webm" if len(self._all_pages) == 1 else f"video-{index + 1}.webm" ) self._videos.append((str(video_path), video_file_name)) + try: + video.delete() + except Error: + # The copy is what gets kept; removing Playwright's file is best effort. + pass def on_did_create_browser_context(self, context: BrowserContext) -> None: context.on("page", lambda page: self._all_pages.append(page)) diff --git a/tests/test_asyncio.py b/tests/test_asyncio.py index 8cb3abc..1cf328a 100644 --- a/tests/test_asyncio.py +++ b/tests/test_asyncio.py @@ -957,6 +957,11 @@ async def failing_teardown(page): yield page raise Exception("teardown failed") + @pytest_asyncio.fixture + async def skipping_setup(page): + await page.goto("data:text/html,
skip
") + pytest.skip("skipped from a fixture") + @pytest.mark.asyncio async def test_passing(page): assert 2 == await page.evaluate("1 + 1") @@ -968,6 +973,10 @@ async def test_setup_fails(failing_setup): @pytest.mark.asyncio async def test_teardown_fails(failing_teardown): await failing_teardown.goto("data:text/html,
teardown
") + + @pytest.mark.asyncio + async def test_setup_skips(skipping_setup): + pass """ ) result = testdir.runpytest( @@ -978,8 +987,9 @@ async def test_teardown_fails(failing_teardown): "--tracing", "retain-on-failure", ) - result.assert_outcomes(passed=2, errors=2) + result.assert_outcomes(passed=2, errors=2, skipped=1) test_results_dir = os.path.join(testdir.tmpdir, "test-results") + # Nothing is kept for the skipped test. _assert_folder_structure( test_results_dir, """ @@ -995,6 +1005,55 @@ async def test_teardown_fails(failing_teardown): ) +def test_artifact_errors_are_reported_as_teardown_errors( + testdir: pytest.Testdir, +) -> None: + # Artifacts are moved to the output folder in pytest_runtest_makereport; an + # error there is reported for the test instead of aborting the session. + testdir.makeconftest( + """ + import shutil + + def _move(*args, **kwargs): + raise OSError("disk full") + + shutil.move = _move + """ + ) + testdir.makepyfile( + """ + import pytest + import pytest_asyncio + + @pytest_asyncio.fixture + async def failing_teardown(page): + yield page + raise Exception("teardown failed") + + @pytest.mark.asyncio + async def test_passing(page): + pass + + @pytest.mark.asyncio + async def test_teardown_fails(failing_teardown): + pass + """ + ) + result = testdir.runpytest("--tracing", "on") + result.assert_outcomes(passed=2, errors=2) + result.stdout.fnmatch_lines( + [ + "*ERROR at teardown of test_passing*", + "*OSError: disk full*", + # The fixture's error remains the main one. + "*ERROR at teardown of test_teardown_fails*", + "*Exception: teardown failed*", + "*Playwright artifacts error*", + "*OSError: disk full*", + ] + ) + + def test_should_work_with_test_names_which_exceeds_256_characters( testdir: pytest.Testdir, ) -> None: diff --git a/tests/test_sync.py b/tests/test_sync.py index 7e6fea0..3f85532 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -951,6 +951,11 @@ def failing_teardown(page): yield page raise Exception("teardown failed") + @pytest.fixture + def skipping_setup(page): + page.goto("data:text/html,
skip
") + pytest.skip("skipped from a fixture") + def test_passing(page): assert 2 == page.evaluate("1 + 1") @@ -959,6 +964,9 @@ def test_setup_fails(failing_setup): def test_teardown_fails(failing_teardown): failing_teardown.goto("data:text/html,
teardown
") + + def test_setup_skips(skipping_setup): + pass """ ) result = testdir.runpytest( @@ -969,8 +977,9 @@ def test_teardown_fails(failing_teardown): "--tracing", "retain-on-failure", ) - result.assert_outcomes(passed=2, errors=2) + result.assert_outcomes(passed=2, errors=2, skipped=1) test_results_dir = os.path.join(testdir.tmpdir, "test-results") + # Nothing is kept for the skipped test. _assert_folder_structure( test_results_dir, """ @@ -986,6 +995,52 @@ def test_teardown_fails(failing_teardown): ) +def test_artifact_errors_are_reported_as_teardown_errors( + testdir: pytest.Testdir, +) -> None: + # Artifacts are moved to the output folder in pytest_runtest_makereport; an + # error there is reported for the test instead of aborting the session. + testdir.makeconftest( + """ + import shutil + + def _move(*args, **kwargs): + raise OSError("disk full") + + shutil.move = _move + """ + ) + testdir.makepyfile( + """ + import pytest + + @pytest.fixture + def failing_teardown(page): + yield page + raise Exception("teardown failed") + + def test_passing(page): + pass + + def test_teardown_fails(failing_teardown): + pass + """ + ) + result = testdir.runpytest("--tracing", "on") + result.assert_outcomes(passed=2, errors=2) + result.stdout.fnmatch_lines( + [ + "*ERROR at teardown of test_passing*", + "*OSError: disk full*", + # The fixture's error remains the main one. + "*ERROR at teardown of test_teardown_fails*", + "*Exception: teardown failed*", + "*Playwright artifacts error*", + "*OSError: disk full*", + ] + ) + + def test_should_work_with_test_names_which_exceeds_256_characters( testdir: pytest.Testdir, ) -> None: