Repository navigation
Conversation
icemac
left a comment
icemac
left a comment
There was a problem hiding this comment.
Code review (high effort): 8 findings, see inline comments.
— Comment created by Claude
| condition | ||
| and not _reruns_condition_matches_phase(item, report.when) | ||
| and not _rerun_matches_phase(item, report.when) | ||
| and _rerun_matches_later_phase(item, report.when) |
There was a problem hiding this comment.
The last attempt drops the real call failure. This continue branch does not check whether reruns are left. On the last attempt (execution_count > reruns), the call report is still suppressed because a later phase "matched", but _should_not_rerun then logs the teardown report as a final error, so nothing ever reports the call failure.
Repro: @pytest.mark.flaky(reruns=1, only_rerun=["ValueError"]), where the test always raises AssertionError and its fixture teardown always raises ValueError.
- master:
1 failed, 1 error - this PR:
1 error, 1 rerun. TheAssertionErroris never reported, and the test is not counted as failed.
This branch should only apply when a rerun will actually happen (reruns left, suite cap not reached, and no veto from _should_not_rerun for the later phase).
— Comment created by Claude
| phases = ("setup", "call", "teardown") | ||
| phase_index = phases.index(phase) | ||
| return any( | ||
| _rerun_matches_phase(item, failed_phase) |
There was a problem hiding this comment.
Terminal setup errors get swallowed. _rerun_matches_later_phase ignores the terminal errors of earlier phases, but _should_not_rerun (teardown branch) still vetoes on _terminal_errors["setup"]. If setup fails with a non-matching error and teardown raises a matching one, the setup report hits continue here, and then the teardown report is logged as final with no rerun.
Repro: only_rerun=["ValueError"], where one fixture raises AssertionError in setup and another raises ValueError in teardown.
- master:
2 errors(the setup AssertionError is shown) - this PR:
1 error. Only the teardownValueErroris shown, and the setup failure disappears.
— Comment created by Claude
| def _should_not_rerun(item, report, reruns, condition): | ||
| xfail = hasattr(report, "wasxfail") | ||
| is_terminal_error = any(item._terminal_errors.values()) | ||
| if report.when == "teardown": |
There was a problem hiding this comment.
This bypasses the rerun_except veto. For teardown, a terminal error in the call phase is now ignored. With rerun_except, a terminal call error means "this exception must never be rerun". With this change, any unrelated teardown failure overrides that and reruns the test.
Repro: @pytest.mark.flaky(reruns=1, rerun_except=["AssertionError"]). The first attempt fails with assert False, "real bug", and its fixture teardown raises RuntimeError.
- master:
1 failed, 1 error - this PR:
1 passed, 1 rerun. The excludedAssertionErroris rerun and hidden.
The override probably only makes sense when the call error is terminal because it misses only_rerun, not when it matches rerun_except.
— Comment created by Claude
| if report.when == "teardown": | ||
| is_terminal_error = item._terminal_errors.get( | ||
| "setup", False | ||
| ) or item._terminal_errors.get("teardown", False) |
There was a problem hiding this comment.
Higher-scoped fixtures are torn down and set up again before the new rerun. pytest_runtest_teardown (around L1229) still refuses to suspend finalizers when any(item._terminal_errors.values()). So when a terminal call error is followed by a matching teardown error, which is the new rerun path, module, class and session fixtures are not held back. They get torn down and then set up again for the rerun.
Repro: a module-scoped fixture that prints on setup and teardown, used by the only test in the module. The new test scenario prints module setup/module teardown twice. This is the double teardown that test_..._module teardown count == 1 (just above the new test) guards against. That guard and the matching check in pytest_runtest_makereport need to follow the new rule too.
— Comment created by Claude
| elif ( | ||
| condition | ||
| and not _reruns_condition_matches_phase(item, report.when) | ||
| and not _rerun_matches_phase(item, report.when) |
There was a problem hiding this comment.
Altitude: each phase report now decides on its own (continue / _should_not_rerun with a teardown special case / _rerun_matches_later_phase), and the three checks have to stay in sync by hand. The three bugs above come from them drifting apart. A simpler approach: after runtestprotocol returns, decide once whether this attempt reruns, based on all failed phases, terminal errors, condition, remaining reruns and the suite cap. Then either mark every failed report as rerun or log them all normally. That also keeps the call failure's traceback in the rerun report instead of silently dropping it.
— Comment created by Claude
| and not _reruns_condition_matches_phase(item, report.when) | ||
| and not _rerun_matches_phase(item, report.when) | ||
| and _rerun_matches_later_phase(item, report.when) | ||
| and ( |
There was a problem hiding this comment.
Efficiency: _rerun_matches_later_phase comes before the cheap report.failed check in this and chain. So it runs for every passing report, and each time it rebuilds _get_reruns_condition_failures(item), which was already computed for condition a few lines above. Moving the report.failed/subtests check first, and passing the already computed failures list in, avoids this.
— Comment created by Claude
|
|
||
| def _rerun_matches_later_phase(item, phase): | ||
| """Return whether a later failed phase can trigger a rerun.""" | ||
| phases = ("setup", "call", "teardown") |
There was a problem hiding this comment.
Reuse: this phase-order tuple duplicates _phase_order() (and the tuple in _get_reruns_condition_failures). Comparing with _phase_order(failed_phase) > _phase_order(phase) keeps a single definition of the ordering.
— Comment created by Claude
|
|
||
| result = testdir.runpytest() | ||
|
|
||
| assert_outcomes(result, passed=1, rerun=1) |
There was a problem hiding this comment.
Test gap: the test only covers the case where the rerun succeeds. A case with no reruns left (both attempts fail in call and teardown) would have caught that the call failure is dropped on the last attempt. Cases for rerun_except and a module-scoped fixture would cover the other regressions noted above.
— Comment created by Claude
|
Pushed dc6cfa1 with the requested fix. Rerun decisions are now made once from all failed phases, terminal rerun-except errors, xfail state, remaining reruns, and the suite cap; failed setup/call/teardown reports are kept together instead of being dropped when a later phase matches. Added regression coverage for last-attempt call/setup failures and rerun-except veto, plus a changelog entry. Validation: 7 targeted cases passed; 261 tests in tests/test_pytest_rerunfailures.py passed (1 pre-existing local pytest 9 summary assertion deselected). |
2 participants
Fixes #261
When a test call fails with an exception excluded by only_rerun, a fixture teardown can still raise an exception that matches the rerun filter. The plugin should rerun the test without retaining the earlier call failure in the final result.
This change:
Checks: