Skip to content

CI quarantine list: a test dev's latest push failed is skipped on PRs for 72 hours, never a test the PR touches (#5942) - #5964

Merged
erikdarlingdata merged 2 commits into
devfrom
fix/5942-quarantine-list
Oct 16, 2026
Merged

erikdarlingdata merged 2 commits into
devfrom
fix/5942-quarantine-list

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

Fixes #5942.

Claude posting for Erik Darling.

The smallest useful version from the adversarial review: a quarantine file that sets aside a test dev's own latest push broke, only on a pull request that does not reach that test, for at most 72 hours. The file ships EMPTY, so merging this changes no CI result.

The file: .github/ci-quarantine.txt

One test per line, comments start with #:

<Namespace.Class.Method> | <dev push run id> | <UTC time, ISO 8601> | #<issue>
  • The test is spelled as the runner prints it in a failed job's log (Darling.Tests.X.Y [FAIL]). A theory case's arguments are left off and the entry leaves out every case of the method. It must start with Darling.Tests. or Lite.Tests.. An entry is one method; a whole class is one line per failing method.
  • The run id and the issue are required: a line with no issue number is malformed and ignored. The header of the file spells the format and the rules out.

When an entry is honored (ci-select.py quarantine)

All of these must hold, or the test runs (every doubt fails open):

  1. Valid: the latest COMPLETED dev push run of build.yml (a cancelled or skipped run is passed over) failed this exact test. The check is GET /repos/{repo}/actions/workflows/build.yml/runs?branch=dev&event=push&status=completed, then that run's failed jobs, then each failed job's log, taking the <time>Z Name [FAIL] lines. If that run succeeded, nothing is valid. If any API call fails, the run list or job list is unreadable, or more than 30 jobs failed, the answer is "unknown" and NO entry is honored (fail open: the test runs). The entry's own run id is for humans; it is not checked.
  2. Expiry: the entry is under 72 hours old from its timestamp, whatever else is true. It never renews itself; a future-dated timestamp is ignored too.
  3. Never skip a test the pull request reaches: the selection (map-select's why) is usable (not FULL, not missing) and gives the test's class none of these reasons: own file changed, covers a changed file, text pattern, names a changed symbol, shares a collection with a changed class. A FULL or missing selection honors nothing.
  4. Pull requests only. A push to dev runs everything, which is how dev learns the test is fixed (the next dev push passing also makes the entry invalid on its own).

Decision for the coordinator to check: reads the tree, guard stage, new class and gated on an environment are NOT counted as reach. A census test reads the whole tree, so counting it would make an entry useless for exactly the break (#5845's census) that cost the most. The cost: a PR that itself adds a violation a quarantined census would catch is not stopped by that census until the entry goes; the dev push after it still runs the test.

The decision prints ::notice title=CI quarantine honored/ignored:: annotations, one per entry with the reason, and a line in the job summary.

Where it runs (scope: the Guard job and the two shard runners)

  • test-map-select (pull requests, already has the checkout, the map selection and actions: read) runs ci-select.py quarantine once and uploads quarantine.json inside the existing test-map-selection artifact. The step is continue-on-error, bounded to 150 seconds, and calls the API only when the file has an entry (it ships empty, so there is no API call and no wait today).
  • run-darling-pg-shard.ps1 and run-lite-shard.ps1 read quarantine.json beside their -SelectionFile (no new parameter, no YAML change in the shard jobs) and add -method- <test> to every chunk's runner command. I confirmed on a built Lite.Tests.exe that -class X -method- X.M runs the class minus that method (23 tests became 22). Note: the build job's no-store Darling pass calls the Darling runner script with its selection file too, so it honors the decision as well.
  • The Guard job (guard-tests) gets actions: read and one new step that fetches test-map-selection only when the quarantine file has an entry, retrying up to 90 seconds (it runs in parallel with the Guard build, and a missing decision just means nothing is left out). The test run adds @quarantineArgs after -failSkips. I did NOT add test-map-select to the job's needs: that would change its if (a skipped need skips the job on a push) and the workflow pins around needs: [gate, guard-build].
  • Not in scope and unchanged: the other runners, the nightly, the whole-tree guards job.

.github/REVIEW-TRAPS.md

Added under "Workflows and CI": "Our wrong test is a bug we introduced, never a flake: a test that dev's own push broke ... failed ten unrelated PRs in a week (#5942). It gets an issue and a fix, and a line in .github/ci-quarantine.txt only while that fix lands: <Namespace.Class.Method> | <dev push run id> | <UTC time> | #<issue>, honored for 72 hours, only while the latest completed dev push run failed that exact test, and never on a PR that touches the test's file or one the test map says reaches it." The format is in that line and in the file header.

Tests

New Lite.Tests/CiSelectQuarantineGuardTests.cs (14 cases, behaviour only, no source-text reading), driving ci-select.py on fixture files and a fixed clock:

  • an honored entry skips the test and becomes -method- <test> (and only for its own suite);
  • an entry 73 hours old runs, 71 hours old is honored;
  • an entry the latest dev run did not fail runs;
  • a PR that reaches the test through each of the five reasons runs it; reads the tree and guard stage stay quarantined;
  • an unreadable dev result, a malformed line, no selection, an unreadable selection and a FULL selection all run the test;
  • an empty or missing file honors nothing and does not fail;
  • the real map-select chained into quarantine: Lite/B.cs (unrelated) honors the entry, Lite/A.cs (covered) and the test's own file run it, so a rename of a selection reason cannot turn the reach check off unseen;
  • the log reader takes the failed tests (and drops theory arguments) from the runner's [FAIL] format, checked also by hand against the real log of dev run 38903116863 (job log API).

Ran (Debug build, 0 warnings): Lite.Tests classes *CiSelect* *Census* *Ratchet* *TestMap* *GuardStage* *Workflow*: Total 130, Failed 0; CiSelectQuarantineGuardTests alone: Total 14, Failed 0. Darling.Tests classes *GuardStage* *Census* *Ratchet* *Workflow*: Total 526, Failed 0, Skipped 1. Workflow YAML parses (yaml.safe_load); both runner scripts parse with the PowerShell parser. A live call of dev_failures() against the repo returned the latest completed dev push run (success, no failures).

Not run: the workflow itself in Actions (the file is empty, so the first real use is the first entry), the full suite (CI decides), a nested-class test name through -method- (an entry copies the log's spelling; I did not verify the runner accepts Outer+Inner there).

Rebase

Dev had not moved when I last fetched; the siblings' edits (#5944, #5955/#5946, #5947) are not in. My edits are a new function block and two subcommands in ci-select.py, one new step plus one artifact path in test-map-select, one new step and one run-line edit in guard-tests, and one block plus @quarantineArgs in each runner script.

CHANGELOG entry

SECTION: Added
ENTRY:

@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 16, 2026 19:02
@erikdarlingdata
erikdarlingdata merged commit 7404409 into dev Oct 16, 2026
60 of 87 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/5942-quarantine-list branch October 16, 2026 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant