Repository navigation
Keep extras appended during a fixture's own teardown - #1073
Closed
afonsojanu wants to merge 1 commit into
Closed
afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
The extras/extra fixtures only ever fed their stashed list into report.extras during the "call" phase, and cleared the stash as soon as their own teardown ran. That's a problem for any fixture that depends on extras and appends to it after its own yield: that append happens during the overall test's teardown phase, by which point the "call" report has already been built and the stash has already been wiped by the extras fixture's own cleanup. The appended extra never makes it into the HTML report at all, silently. pytest_runtest_makereport now also looks at the stash during the "teardown" phase, taking only whatever was added after "call" already took its snapshot so nothing gets duplicated, and clears the stash itself once it's done with it. The extras/extra fixtures no longer clear the stash in their own teardown, since something depending on them may still need to write to it after they've already yielded. Added a regression test that appends an extra from a dependent fixture's teardown and checks it shows up in the report's embedded JSON data (the legacy suite's older tests assert on literal <a>/<img> tags, which the current JS-based renderer doesn't emit directly into the HTML anymore, so this one reads the data-jsonblob attribute instead). Confirmed it fails on unmodified master and passes with the fix.
There was a problem hiding this comment.
🟡 Changes recommended
The teardown slicing logic can drop extras when _html_report_extras_seen is stale across reruns (e.g., setup-skip/fail rerun attempts), so it should be hardened before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a long-standing limitation where extras appended to the extras/extra fixture during fixture teardown (after yield) were never included in the HTML report, by extending report generation to also pick up newly-added extras during the test’s teardown phase.
Changes:
- Update
pytest_runtest_makereportto also read fixture-stashed extras during theteardownphase, attaching only the extras added after thecallsnapshot and clearing the stash afterward. - Stop clearing the extras stash in the
extra/extrasfixtures’ own teardown so dependent fixtures can append during their teardown. - Add a regression test that appends an extra from a dependent fixture’s teardown and asserts it appears in the report’s embedded JSON blob.
File summaries
| File | Description |
|---|---|
testing/legacy_test_pytest_html.py |
Adds a regression test ensuring teardown-added extras appear in the generated report data. |
src/pytest_html/plugin.py |
Extends extras collection into the teardown phase and moves stash clearing into the reporting hook. |
src/pytest_html/fixtures.py |
Prevents fixture teardown from clearing the stash so dependent fixtures can append extras later. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+146
to
+148
| fixture_extras = item.config.stash.get(extras_stash_key, []) | ||
| already_seen = getattr(item, "_html_report_extras_seen", 0) | ||
| new_extras = fixture_extras[already_seen:] |
Author
|
Part of a general cleanup on my end, closing this along with the rest of what I had open. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #820.
extras/extraonly ever fed their stashed list intoreport.extrasduring the "call" phase, and cleared the stash the moment their own teardown ran. That breaks any fixture that depends onextrasand appends to it after its ownyield(for example to attach a file generated during cleanup): that append happens during the overall test's teardown phase, by which point the "call" report has already been built and the stash has already been wiped byextras's own cleanup. The appended item never shows up in the HTML report, with nothing to indicate why.pytest_runtest_makereportnow also reads the stash during the "teardown" phase, taking only what was added after "call" already took its snapshot (so nothing gets attached twice), and clears the stash itself once it's done with it.extras/extrano longer clear the stash themselves, since something depending on them may still need to write to it after they've already yielded.Added a regression test that appends an extra from a dependent fixture's teardown and checks it shows up in the report's embedded JSON data. I went with that instead of the
<a>/<img>assertions the older tests in that file use, since the current JS-based renderer doesn't emit that markup directly into the HTML anymore and those assertions were already failing for unrelated reasons on this branch. Confirmed the new test fails on unmodifiedmaster(assert 0 == 1) and passes with the fix. Fulltesting/test_unit.pysuite passes; the rest oftesting/legacy_test_pytest_html.pyhas a number of pre-existing unrelated failures in my environment (missing Selenium/webdriver, filesystem permission quirks) that are identical before and after this change, so this diff doesn't add any new ones.No CLA/DCO step as far as I could find in this repo.