ci: quarantine flaky tests against a ticket instead of retrying past them - #777
Draft
rkennke wants to merge 1 commit into
Draft
ci: quarantine flaky tests against a ticket instead of retrying past them#777rkennke wants to merge 1 commit into
rkennke wants to merge 1 commit into
Conversation
…them CI could not tell a flaky test from a broken one, and its one retry erased the evidence either way. - The retry existed only for ASan. A test that failed on attempt 1 and passed on attempt 2 left a green job and a ::warning:: in a log, naming nothing. - Test reports upload only `if: failure()`, so the run that recovered on a retry -- the one worth studying -- produced no artifact at all. - generate-test-summary.sh already downloaded `(test-reports)*` and grepped TEST-*.xml for failed test names, but prepare_reports.sh copies build/reports/tests (the HTML) and never build/test-results (the XML), so it searched artifacts containing no XML and every failed job rendered "No detailed failure information available". A `**` glob with no `shopt -s globstar` would have stopped it recursing even had the files been there. Retrying until green would only have made the tolerance official. Instead the retry now buys a label and nothing else, and an explicit list decides what may fail: flaky failed one attempt, passed another broken failed every attempt gating not on the quarantine list -- red, whichever of the above it is So a flake fails the build until somebody quarantines it against a PROF ticket. ddprof-test/quarantine.txt is a plain "|"-separated table, one entry per line, chosen over JSON/YAML because it is edited by hand far more than by machine: real comments, one-line diffs, clean git blame, and no parser beyond str.split (it must also load inside the Alpine containers, where PyYAML is not a given). Every entry carries a ticket and a review_by date, and validate-quarantine fails CI once that date passes -- otherwise the list only grows and quarantine becomes a permanent mute rather than tracked debt. Quarantined tests still run and still report; only the gating is suspended, so the pass rate keeps saying whether the test is recovering or has quietly become permanently broken. To keep the honest path the cheap one, the PR comment prints a filled-in entry to paste, with a `cells` glob narrowed to the axis that actually failed. The ticket and the judgement stay with a person; the typing does not. Reporting is grouped by test rather than by cell -- one flaky test reddens a dozen cells and so do a dozen unrelated breakages -- and per-cell outcomes now upload whether the cell passed or failed, since a cell that failed only on its first attempt produces no failure artifact. Failing to classify is itself a failure: if flake_report.py cannot run, the job goes red rather than inheriting a pass nothing examined. An earlier draft had `|| true` there and turned a real flake green in testing. test_quarantine.sh covers the gating decisions against fixtures, including that an un-quarantined flake stays red, a quarantined one does not, a build error is never excused by the list, and an unreadable list cannot yield green. It runs in the validate-quarantine job. The retry path only executes once something has failed, so CI would otherwise never exercise it. Deferred: auto-filing PROF tickets (needs dedupe and an Atlassian credential for CI) and the GitLab dd-trace integration matrix, which still gets one shot per config.
Contributor
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
Contributor
CI Test ResultsRun: #33669575587 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-09-02 19:06:55 UTC |
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
1 participant
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.
What does this PR do?:
Makes flaky tests a tracked, expiring debt instead of something CI quietly retries past.
The retry now buys a label, not a pass:
A flake fails the build until someone quarantines it against a PROF ticket in
ddprof-test/quarantine.txt. Every entry carries areview_bydate and CI fails once it passes, so the list expires rather than accumulating. Quarantined tests still run and report — only the gating is suspended, so the pass rate keeps saying whether the test is recovering or has quietly become permanently broken.To keep the honest path the cheap one, the PR comment prints a ready-to-paste entry for each flaky test, with
cellspre-narrowed to the axis that actually failed.Motivation:
CI could not tell a flaky test from a broken one, and its one retry erased the evidence either way.
::warning::in a log, naming nothing.if: failure()— so the run that recovered on a retry, the one worth studying, produced no artifact at all.generate-test-summary.shalready downloaded(test-reports)*and greppedTEST-*.xmlfor failed test names, butprepare_reports.shcopiesbuild/reports/tests(the HTML) and neverbuild/test-results(the XML). It searched artifacts containing no XML, so every failed job rendered "No detailed failure information available". A**glob with noshopt -s globstarwould have stopped it recursing even had the files been there.Answering "is this flaky?" therefore meant hand-comparing PRs by eye. Retrying until green would only have made the tolerance official.
Additional Notes:
Why a text table and not JSON/YAML. The list is edited by hand far more often than by machine, so real comments, one-line diffs and clean
git blamematter more than a schema. It also has to parse inside the Alpine test containers, where PyYAML cannot be assumed — this needs nothing butstr.split.Failing to classify is itself a failure. If
flake_report.pycannot run, the job goes red rather than inheriting a pass nothing examined. An earlier draft had|| truethere and turned a real flake green during testing; there's a regression test for it now.Behaviour changes worth knowing: retry is enabled for all four platform jobs (previously ASan only), so a flake-shaped failure costs that cell a second run; slow suites are capped at one attempt to stay clear of the 180-minute timeout; ASan keeps its retry-on-abort via
RETRY_ON_NO_TEST_FAILURES, since it names no test when it dies at init.Deferred: auto-filing PROF tickets (needs dedupe and an Atlassian credential for CI), and the GitLab dd-trace integration matrix, which still gets one shot per config.
Not exercised on real CI: the retry and gating paths only run once something has failed, which is why they are covered by fixtures instead.
How to test the change?:
.github/scripts/tests/test_quarantine.sh— 15 assertions, run by the newvalidate-quarantinejob:review_by, duplicate entry, malformed line, and that the committed list is itself valid*arm64*matches arm64 cells and not amd64), class wildcards (a.B.*)For Datadog employees: