merge queue: checking #89 on main (08156aa) - #118
Closed
mergify[bot] wants to merge 4 commits into
Closed
Conversation
…ection Test retry needs everything flaky detection already knows how to do -- carve a per-test deadline out of a budget, honour the execution cap and the `pytest-timeout` margin, suspend higher-scoped finalizers across a rerun and restore them on the last one -- and differs only in which tests it targets, which budget it spends and when it stops. Copying ~200 lines into a second class would have made the two mechanisms drift apart exactly where they must not; folding retry into `FlakyDetector` as a third mode would have made one budget's exhaustion able to switch off the other's behaviour, which is the thing the design most needs to prevent. So the lifecycle moves to `RerunLoop` in its own module and each mechanism subclasses it, holding its own metrics, its own budget and its own targets. No behaviour change: `FlakyDetector` keeps its mode, its target selection and its report, and the extracted methods move verbatim. `_flaky_detection_disabled` becomes `mergify_marker_disables(item, option)` so a second mechanism can read its own `@pytest.mark.mergify` keyword -- opting out of being learned from is not the same statement as refusing to have a failure retried. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> References: MRGFY-8872
The engine now serves `budget_ratio_for_test_retries`, `flaky_test_names` and `broken_test_names` on `/flaky-detection-context` (Mergifyio/monorepo#39141): the eligible set a client may retry a failed attempt on, and the share of the suite's runtime it may spend doing so. Nothing reads them yet; this is the wire layer they arrive through. All three default rather than being required, in both the Rust model and the Python dataclass. A client is released on its own cadence and runs against whatever engine a customer's CI reaches, so a build of this client will meet an engine that predates these fields. Defaulting makes that an engine with no budget and nothing to retry -- which is exactly what it is -- instead of a deserialisation error that costs the customer their flaky detection too. `broken_test_names` is carried but will stay unread: retry acts on `flaky` and nothing else, because a deterministic failure is not something a rerun can rescue. It ships so the payload states what the server knows rather than only what today's client consumes. The napi binding maps its own struct field by field, so the TypeScript clients are unaffected until they implement retry (MRGFY-8874). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> References: MRGFY-8872
…is flaky A flaky test reds a build that has nothing wrong with it, and today the only answer Mergify offers is auto-quarantine: the failure still happens, it just stops counting. Retry replaces that with a real answer -- run the test again, and let the run produce a genuine pass or fail. The change is smaller than it sounds, and it is not a change to what gets logged. `FlakyDetector` already reruns a targeted set within a budget; what made a flaky failure fatal was that the *initial* attempt was always reported as it happened. For a test the server classifies as flaky, that attempt is now held back until the reruns have answered for it. Everything else follows. A rescue is reported as a pass carrying a mark of its own, which is how pytest reports an xfail. `pytest_report_teststatus` reads the mark back, so the terminal still shows `R`/RESCUED and the tail still counts it separately (`1 rescued, 128 passed`) -- the case most worth seeing, and the one auto-quarantine hid best. The outcome itself has to stay one of the three pytest core knows: an invented `"rescued"` string leaves `report.passed`, `.failed` and `.skipped` all false, and junitxml then wrote `tests="0"` beside the testcase it had just emitted, while `--last-failed` re-selected the test forever. Where the two mechanisms overlap, retry buys nothing. Every flaky test is also in `unhealthy_test_names`, so on a push run flaky detection is already rerunning it; those attempts answer retry's question on detection's budget. Retry still owns the verdict there, but runs its own loop only for tests nobody else is rerunning. Two owners of one finalizer stack is not a state this code could be correct in, and double-spending two budgets on one test is not one worth being correct in. In `new` mode it goes further and stands aside entirely: there a rerun failure *is* the merge gate that stops a newly flaky test from landing. The server can call one test both new and flaky at once, when its baseline lags a rename or a re-parametrization, and absorbing that failure would switch the gate off without anyone asking. The stricter of the two answers wins. What the wire carries is the *initial* attempt, always: `test.case.result.status` stays `failed` on a rescue, plus `cicd.test.auto_retry` to say which mechanism asked for the rerun. Reporting the pass would drop the test out of the flaky set that made it eligible, and retry would switch itself off on exactly what it rescues. There is a test whose whole job is to kill that idea if it returns. Also here, because retry cannot be honest without them: - A failure retry could not afford to answer is stated as such. It otherwise looks identical to one retry answered and lost, and only one of those is a verdict on the test. Nothing in the block claims an attempt that never happened. - Metrics are read from `pytest_runtest_makereport` rather than `pytest_runtest_logreport`, which a held-back attempt never reaches. That hook fires for every phase of every attempt whether or not it is reported, so deadlines and finalizer bookkeeping stay correct while retry is still deciding what the session will say. - The context fetch is split from building the mechanisms. It was skipped outright in `new` mode with an empty baseline -- and `new` is every pull request -- so a repository that opted into retry alone, which is served no baseline, would have dropped the eligible set on every PR. The same repository no longer gets a "Flaky detection" block on push runs for a mechanism it never enabled. - Retry needs room for one more attempt, not `min_test_execution_count` of them; inheriting detection's gate made it refuse retries it could afford by that factor and then blame the budget. - Per-test opt-out is `@pytest.mark.mergify(auto_retry=False)`, its own keyword: declining to have a failure retried is not the same statement as declining to have the test learned from. `broken_test_names` stays unread and `skipped` never enters the matrix: a test that fails every time is not something a rerun can rescue, and a test that never ran has nothing to answer for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Fixes: MRGFY-8872
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.
🎉 This pull request has been checked successfully and will be merged soon. 🎉
#89 is queued for merge on branch main (08156aa).
This pull request has been created by Mergify to check the mergeability of #89.
You don't need to do anything. Mergify will close this pull request automatically when it is complete.
Required conditions of queue rule
defaultfor merge:depends-on = Mergifyio/monorepo#39141[⛓️ Mergifyio/monorepo#39141]Enforce conventional commit]:title ~= ^(fix|feat|internal|docs|style|refactor|perf|test|build|ci|chore|revert|ui)(?:\(.+\))?!?:👀 Review Requirements]:#approved-reviews-by>=2author = dependabot[bot]author = mergify-ci-botauthor = renovate[bot]📕 PR description]:body ~= (?ms:.{48,})🔎 Reviews]:#changes-requested-reviews-by = 0#review-requested = 0#review-threads-unresolved = 0🤖 Continuous Integration]:check-success=ci-gateRequired conditions to stay in the queue:
base=maindepends-on = Mergifyio/monorepo#39141[⛓️ Mergifyio/monorepo#39141]label!=manual mergeEnforce conventional commit]:title ~= ^(fix|feat|internal|docs|style|refactor|perf|test|build|ci|chore|revert|ui)(?:\(.+\))?!?:👀 Review Requirements]:#approved-reviews-by>=2author = dependabot[bot]author = mergify-ci-botauthor = renovate[bot]📕 PR description]:body ~= (?ms:.{48,})🔎 Reviews]:#changes-requested-reviews-by = 0#review-requested = 0#review-threads-unresolved = 0🤖 Continuous Integration]:check-success=ci-gate