Skip to content

ci: quarantine flaky tests against a ticket instead of retrying past them - #777

Open
rkennke wants to merge 9 commits into
mainfrom
ci/flake-visibility
Open

rkennke wants to merge 9 commits into
mainfrom
ci/flake-visibility

Conversation

@rkennke

@rkennke rkennke commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?:

Makes flaky tests a tracked, expiring debt instead of something CI quietly retries past.

A retry now buys a label, not a pass:

flaky failed one attempt, passed another
broken failed every attempt
unclassified failed, and the suite was not retried — so which of the two was never measured
gating not on the quarantine list — red, whichever of the above it is

A flake fails the build until it is quarantined against a PROF ticket in ddprof-test/quarantine.txt. Entries carry a review_by date and expire: once it passes, the entry stops excusing anything and CI goes red again. Quarantined tests still run and report — only the gating is suspended, so the pass rate still says whether a test is recovering or has quietly become permanently broken. The PR comment prints a paste-ready entry for each flake, with cells narrowed to the axes that actually failed.

Only the final attempt's own named failures are weighed against the list, and flaky requires observing a pass rather than mere absence from a later attempt. The list cannot excuse:

  • a failure in any task other than the test task
  • a final attempt that exited non-zero and shows evidence of being cut short — Gradle reporting a dead test JVM, or the attempt having reached fewer tests than another attempt managed. A quarantined test that fails makes Gradle exit non-zero by itself, so the exit code alone cannot separate the two
  • results that could not be read — a snapshot missing root-owned files, or one merged with an earlier attempt's, is indistinguishable from an attempt whose missing tests all passed
  • its own machinery failing: if the classifier cannot run, the job goes red rather than inheriting a pass nothing examined

Conversely, suspect evidence with no failure recorded holds no opinion: it is a reason to distrust an excuse, not a test failure, so a suite that passed stays green.

Motivation:

CI could not tell a flaky test from a broken one, and its single retry erased the evidence either way:

  • the retry existed only for ASan, and a run that recovered on attempt 2 left a green job and a ::warning:: naming nothing
  • reports upload only if: failure(), so the recovered run — the one worth studying — produced no artifact
  • generate-test-summary.sh grepped TEST-*.xml out of artifacts that only ever held build/reports/tests (HTML), never build/test-results (XML), so every failed job rendered "No detailed failure information available"
  • musl produced no JUnit XML at all, so nothing there could be classified, retried or gated

Answering "is this flaky?" therefore meant hand-comparing PRs by eye. Retrying until green would only have made the tolerance official.

Additional Notes:

musl coverage. ProfilerTestRunner runs musl's tests through the JUnit launcher directly and registered no XML listener. It now attaches a LegacyXmlReportGeneratingListener when -Dtest.reportsDir is set, and ProfilerTestPlugin points that at build/test-results/<taskName> — the layout Gradle's own Test task uses — so the TEST-*.xml scan finds musl's results the same way it finds every other cell's. It fails loudly if reports were requested and cannot be written, since producing none silently is indistinguishable from a cell with nothing to report.

Patterns match the ids JUnit writes. Gradle records a @Test method as name="method()", so ids arrive as Class.method() while entries are written Class.method; covers() normalises that. @ParameterizedTest/@RetryingTest invocations appear as [1], [2] with no method name at all, so they can only be quarantined class-wide. A trailing .* now covers one class's methods, as the format has always documented, rather than being an unbounded prefix — so one validate-clean line can no longer suspend the merge gate repo-wide. validate rejects all three unmatchable shapes, and a proposal widened to the class says so with a WIDENED line.

Expiry is enforced where matching happens, not only in the validate subcommand: validate-quarantine runs from ci.yml alone, while find_entry is reached from every workflow reusing run_tests_with_retry.sh. An unreadable review_by counts as expired rather than as never expiring.

Proposals narrow on every axis. cells_glob reads the shared axes positionally out of the <libc>-<jdk>-<config>-<arch>[-slow] grammar rather than matching a token list, so the JDK and the regular/slow axes cannot be silently dropped — a flake seen on one cell no longer proposes muting it across 13 JDKs and the slow suite.

Behaviour changes. Retry now covers all four platform jobs, not just ASan, 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 retry-on-abort via RETRY_ON_NO_TEST_FAILURES, naming no test when it dies at init.

Status check names change. The four matrix jobs gain an explicit name:, so their checks go from test-linux-glibc-amd64 (17, debug) to test-linux-glibc-amd64 (17, debug, regular). The suffix is required: this workflow is called twice in one run (nightly, release-validated) with overlapping configs, and without it two different jobs collapse onto the same cell. No ruleset on this repo requires status checks by name, so nothing should block on the old strings — but anything outside GitHub that matches on them would need the new form.

How to test the change?:

Three hermetic suites, all run by the new validate-quarantine job: test_quarantine.sh (56 assertions), test_generate_test_summary.sh (4), test_prepare_reports.sh (2). Between them they cover every validation rejection, glob matching in both directions (a proposal matches the cells it came from and excludes those differing only in JDK or in the slow suffix), each thing the list cannot excuse, the classification edges (absent-from-retry, single-attempt, unnamed testcase), an empty command list, an artifact still shipping XML when a snapshot came out empty, and the rendered output. The summary suite also checks the job-name regex against test_workflow.yml's own name: lines, so a rename fails here rather than rendering a summary that reports zero test jobs.

Every guard was mutation-checked: reverting it individually turns its own assertion red.

The retry and gating paths only run once something has failed, so nothing in CI has exercised them end-to-end — they are covered by fixtures instead. Worth knowing when reading the risk of this PR.

Known gaps, deliberately not in this PR:

  • No test runs the real producers. Every XML fixture is hand-written, and nothing runs Gradle or ProfilerTestRunner to confirm the shape matches what flake_report.py parses. A mismatch there makes the whole list a no-op while every assertion stays green — which is exactly what the Class.method() bug above was. The highest-value follow-up.
  • The retry policy is duplicated across the four platform jobs in two divergent encodings, so which value applies to which (config, suite) pair has no single home.
  • The outcome JSON has a second reader: the shell re-parses it through an inline python3 -c and a two-line stdout protocol, rather than flake_report.py communicating its verdict directly.
  • The cell-name grammar has no owner — six places build or parse it, one of them in a different shape.
  • Auto-filing PROF tickets (needs dedupe and an Atlassian credential), and the GitLab dd-trace integration matrix, which reads validation logs rather than JUnit XML and so needs this policy against a different signal.

For Datadog employees:

  • This PR doesn't touch any of that.
  • JIRA: PROF-15902

…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.
@dd-octo-sts

dd-octo-sts Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Scan-Build Report

User:runner@runnervmlun5p
Working Directory:/home/runner/work/java-profiler/java-profiler/ddprof-lib/src/test/make
Command Line:make -j4 all
Clang Version:Ubuntu clang version 18.1.3 (1ubuntu1)
Date:Tue Sep 15 11:56:12 2026

Bug Summary

Bug TypeQuantityDisplay?
All Bugs1
Logic error
Dereference of null pointer1

Reports

Bug Group Bug Type ▾ File Function/Method Line Path Length
Logic errorDereference of null pointerfaultInjection.cppcrashNow242

@dd-octo-sts

dd-octo-sts Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

CI Test Results

Run: #34965861903 | Commit: 2b3dae3 | Duration: 16m 7s (longest job)

All 32 test jobs passed

Status Overview

JDK glibc-aarch64/debug glibc-amd64/debug musl-aarch64/debug musl-amd64/debug
8 - - -
8-ibm - - -
8-j9 - -
8-librca - -
8-orcl - - -
11 - - -
11-j9 - -
11-librca - -
17 - -
17-graal - -
17-j9 - -
17-librca - -
21 - -
21-graal - -
21-librca - -
25 - -
25-graal - -
25-librca - -

Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled

Summary: Total: 32 | Passed: 32 | Failed: 0


Updated: 2026-09-15 12:13:19 UTC

Two defects that would have mattered:

Quarantine excused too much. When every named test failure was on the
list, the runner forced a green exit -- including when the same
invocation had also failed a native or verification task, which the
list has no business excusing. The runner now scans the attempt's log
for `Execution failed for task` naming anything outside the test task
and refuses to zero the exit code.

The documented cell glob could never match. Cells are named
<libc>-<jdk>-<config>-<arch> with arch amd64 or aarch64, so the
`*arm64*` in quarantine.txt's example and in flake_summary.py's axis
list matched nothing: the narrowing they advertised silently
quarantined everywhere. Both use aarch64 now, and `validate` rejects a
glob naming an architecture CI never builds.

Also:

- flaky now means failed once and observed passing on another attempt,
  not merely absent from it. An attempt that aborted early no longer
  turns every earlier failure into a flake with a paste-ready entry.
- the runner clears its own evidence directory, so a reused workspace
  cannot contribute a previous run's attempts to this run's gating.
- the counter and the gating read are checked rather than defaulted to
  zero, matching the fail-loud policy already applied to the classifier.
- Docker-written results are made readable before snapshotting, and the
  snapshot warns instead of discarding errors; musl-aarch64 was losing
  flake classification silently.
- pipes in failure messages are escaped, flaky tests appear in the
  per-job details, and an unparseable outcome report is visible rather
  than rendering as a clean non-test failure.
- validating a missing list fails instead of reporting zero problems;
  duplicate detection keys on the cell globs, so narrowing by cell is
  actually usable.
- one first-match helper shared by both selection paths.

The regression test for the classifier guard did not exercise it -- a
malformed line is skipped, not fatal, so the flake was the reason for
the red. It now points --list at a directory to make the classifier
genuinely fail. Each new guard was mutation-checked: reverting it turns
the corresponding test red.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dd-octo-sts

dd-octo-sts Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

All 40 integration tests passed

📊 Dashboard · 👷 Pipeline · 📦 3786d2e6

rkennke and others added 2 commits September 3, 2026 17:38
The exit code the quarantine list overrides comes from the final attempt,
but the failures it was weighed against were aggregated across all of them.
A final attempt that failed without naming a test -- a docker failure in
the musl-aarch64 job, a Gradle configuration error, an OOM-killed daemon,
the ASan init abort this retry exists for -- was excused as soon as one
entry matched a failure from an earlier attempt. flake_report.py now
reports the final attempt's own standing and the runner refuses to zero
the exit code unless that attempt produced results with every one of its
own named failures quarantined; the non-test-task grep stays as a second
line of defence rather than the only one.

Alongside it:

- validate rejects overlapping cell globs, not just byte-identical ones,
  and reports an out-of-range review_by as an annotated problem instead of
  an uncaught ValueError that loses every other annotation in the file
- the dead `propose` subcommand goes; flake_summary.py already renders the
  paste-ready entry CI actually uses
- test ids and failure messages are sanitised before they reach the PR
  comment, so a test's own output cannot break out of the fenced
  quarantine proposal a reviewer is invited to copy
- a summary with no readable outcome reports says so rather than looking
  like a clean run
- the cell label carries the slow/regular axis, so nightly's two invocations
  of the same config stop colliding in ci-outcome/<cell>.json
- testcase elements with no name are skipped instead of being counted and
  proposed for quarantine as "."
- make_results_readable probes per-file ownership rather than the top of the
  tree, and covers the parent so the pre-attempt rm -rf can unlink it
- flake-evidence/ and ci-outcome/ are gitignored

Tests: the clean-pass path and the final-attempt-named-no-test regression
are now covered (27 assertions), and a new test_generate_test_summary.sh
pins generate-test-summary.sh's jq failure branch -- verified by removing
the `!` and watching it go red.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A final attempt that crashes part-way through still writes JUnit XML for
the tests it reached, and those passed -- so it names no failure of its
own while every aggregated failure is quarantined. Gradle attributes the
abort to the test task itself, so the non-test-task check cannot see it
either. The final attempt's own exit code is the only thing that tells
this apart from an ordinary flaky-then-passed run, so the runner now
hands it to the classifier, which gates a non-zero exit that named
nothing.

Two evidence-integrity holes alongside it:

- snapshot() takes read access again before copying. The XML is written
  by the command that just ran, after the loop-top call, and under
  Docker it lands root-owned; without this the copy fails and the cell
  loses flake classification silently.
- make_results_readable() reports failure instead of warning and
  returning success, and its callers turn that into EVIDENCE_SUSPECT. A
  snapshot missing root-owned files is indistinguishable from an attempt
  whose missing tests all passed, which is exactly what the quarantine
  list must not be allowed to excuse.

Three assertions cover these; each was mutation-checked individually.
One of them asserts the ordinary quarantined-flake-recovers case stays
green, so the new gate cannot be satisfied by reddening everything.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@rkennke rkennke added the sphinx:critical Sphinx: critical — human review required label Sep 4, 2026
@datadog-official

datadog-official Bot commented Sep 4, 2026

Copy link
Copy Markdown

Pipelines

⚠️ Warnings

Your PR has failed checks. Please review the issues below and take necessary action before merging.

🚦 4 Pipeline jobs failed

DataDog/java-profiler | gtest-asan-amd64

View more details · View in GitLab

DataDog/java-profiler | gtest-asan-arm64

View more details · View in GitLab

DataDog/java-profiler | gtest-tsan-amd64

View more details · View in GitLab

View all 4 failed jobs.

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 5f01c39 | Docs | View more details | Give us feedback!

rkennke and others added 5 commits September 15, 2026 11:31
musl produced no JUnit XML, so every mechanism here read nothing for it: the
retry never fired, no failure could be classified, and a passing attempt was
indistinguishable from one that never ran. ProfilerTestRunner now registers a
LegacyXmlReportGeneratingListener when -Dtest.reportsDir is set, and
ProfilerTestPlugin points it at build/test-results/<taskName> -- the same
layout Gradle's own Test task uses, so the TEST-*.xml scan finds musl's
results exactly as it finds every other cell's. Adds junit-platform-reporting
to the testing bundle.

Four gating holes:

- A non-zero final attempt gates regardless of how many of its own failures
  were named and quarantined. A JVM can abort part-way through *after* naming
  a real quarantined failure, leaving the tests it never reached absent from
  the XML rather than passing; requiring zero named failures let one
  quarantined name paper over the crash.
- quarantine.py enforces review_by at match time, not only in the validate
  subcommand. validate-quarantine runs from ci.yml alone, while find_entry is
  reached from every workflow that reuses run_tests_with_retry.sh, so an
  expired entry could keep excusing failures in nightly and release-validated.
- cells_glob listed its axes out of cell-name order. Cell names read
  <libc>-<jdk>-<config>-<arch>[-slow] and the glob is built by `*`-joining, so
  an axis order of aarch64 before musl produced `*aarch64*musl*` -- a proposal
  that fnmatch can never match against the cells it was derived from. A test
  now asserts every generated glob matches its own cells.
- A failed snapshot sets EVIDENCE_SUSPECT rather than only warning, so results
  that could not be copied reach the classifier as suspect evidence instead of
  looking like an attempt whose missing tests all passed.

All 35 quarantine tests and 3 summary tests pass; :ddprof-test:compileTestJava
succeeds with the new dependency.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A quarantined test that fails makes Gradle exit non-zero by itself, so gating
on any non-zero final-attempt exit code gated the ordinary case the list
exists to excuse. Only "failed on an earlier attempt, then passed outright on
the final one" could ever be excused, and on slow suites -- MAX_ATTEMPTS=1, so
no earlier attempt exists -- the list had no effect at all. That contradicts
quarantine.txt's first line.

A non-zero exit now gates only with positive evidence that the attempt was cut
short rather than run to completion:

- Gradle reporting a dead test JVM in the attempt log ("finished with non-zero
  exit value"), or the JVM's own fatal-error banner
- the final attempt having reached fewer tests than another attempt managed,
  which needs more than one attempt and so does not help slow suites

Both are reported in the outcome JSON. A missing log yields no evidence and
does not gate, matching how non_test_task_failures() already treats it.

No test passed --final-attempt-exit-code, so it defaulted to None, took the
(None, 0) exemption, and the whole guard went unexercised. Three fixtures now
cover it: a quarantined failure with exit 1 and an ordinary test-task failure
in the log must not gate; the same with a dead-JVM log must gate; and a final
attempt that reached fewer tests than an earlier one must gate. 38 assertions
pass, and each new guard was mutation-checked.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Gradle records a JUnit 5 @test method as name="method()", so the id built from
the report is Class.method(), while quarantine.txt documents -- and its own
worked example uses -- Class.method. covers() compares by exact equality, so
an entry written exactly as documented matched nothing: the failure still
gated, validate still reported the list as valid, and the only symptom was a
quarantine that quietly excused nothing. Confirmed against this repo's own
reports, which carry both name="attributeOverflowReturnsFalse()" and
name="[1]".

normalise_test_id() strips the trailing parentheses, and covers() applies it.
That keeps the normalisation in the one function every caller's match goes
through, and leaves reports and annotations showing the id JUnit produced
rather than a rewritten one.

Two shapes can never match, so validate now rejects them instead of passing
them as valid:

- a test field carrying JUnit's parentheses, which the normalisation above
  makes unnecessary
- one whose last segment is an invocation index. @ParameterizedTest and
  @RetryingTest invocations appear as "[1]", "[2]" with no method name at all,
  so the index identifies neither the method nor a stable case; the class-wide
  "<class>.*" form is the only thing that can cover them, and quarantine.txt
  now says so.

The paste-ready proposal keeps a real method name for the first shape.
sanitize_quarantine_test_pattern() widened anything containing punctuation to
the whole class, and "()" is punctuation, so every per-method proposal came
out as <class>.* -- muting far more than the evidence supported. The parens
are now stripped before that test, while an indexed invocation still proposes
the class, which is correct for it.

42 assertions pass. Each guard was mutation-checked: dropping the
normalisation, either validation, or the precise proposal turns its own
assertion red.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every item here shares one shape: something the machinery could not read, or
could not express, was treated as permission to proceed rather than as a
reason to stop. Each now has a fixture, and each fixture was mutation-checked
against its own guard.

quarantine.py

- An unreadable review_by (blank, or not a calendar date -- parse() accepts
  both) made an entry never expire. Since validate() only runs in PR CI while
  find_entry() is reached from nightly and release-validated too, the one
  entry nobody can review outlived every entry that could. It now counts as
  expired.
- A trailing ".*" was an unbounded prefix match, so "com.datadoghq.profiler.*"
  suspended the merge gate for the whole repository from a single
  validate-clean line. It now covers one class's methods, as quarantine.txt
  has always claimed, and validate rejects a pattern that reads as a package.
- A bare ".*" was validated against the empty string and accepted, then
  matched nothing.
- The synthetic cell universe listed a non-existent 8-graal and omitted every
  -j9/-ibm/-orcl variant plus 21-graal and 25-graal. cells_overlap() fails
  closed for a glob matching nothing synthetic, so two genuinely disjoint
  entries (say *17-j9* and *21-graal*) were rejected as duplicates. The JDK
  axis is now the cross-product of the bases and the suffixes CI builds.

flake_report.py

- --evidence-suspect gated unconditionally, before establishing there was any
  failure to excuse: a suite that passed while its results tree could not be
  cleared was turned from exit 0 into a red job. Suspect evidence is a reason
  to distrust a quarantine excuse, so it now applies only where an excuse is
  being weighed.
- A single attempt cannot distinguish flaky from broken -- passed_in is empty
  for every failure because nothing re-ran -- yet every slow-suite failure was
  reported as "persistent" and offered no quarantine entry. Those now land in
  an "unclassified" bucket which the summary renders with proposals and says
  plainly that flakiness was not measured.

flake_summary.py

- cells_glob() matched a hardcoded token list, omitting the JDK and the
  regular/slow axes -- the two the matrix varies along most. A flake seen only
  on glibc-8-j9-debug-amd64 proposed *glibc*debug*amd64*, quarantining it on
  all 13 JDKs and on the slow suite too. The axes are now read positionally
  out of the cell grammar, so an axis nobody thought of cannot be dropped.
- A proposal widened to the whole class (the only form covers() can express
  for an invocation index) said nothing about it. It now carries a WIDENED
  line naming the id that could not be expressed exactly.

run_tests_with_retry.sh

- An empty command list degraded to a bare redirection, exited 0, recorded no
  attempt, and passed the cell without running a test.
- A destination that could not be cleared left cp -r merging this attempt's
  evidence with an earlier one's, so attempt_results() reported tests as
  observed-and-passed that this attempt never ran -- which is how a persistent
  failure acquires a flaky label and a paste-ready entry that buries a real
  defect. Both that and the run-start clear now record suspect evidence
  instead of discarding the status.

prepare_reports.sh

- The fallback that ships build/test-results keyed off the attempt-*
  directory, which snapshot() creates before the copy that may fail. An empty
  or partial snapshot therefore uploaded an artifact with no JUnit XML in it.
  It now tests for actual XML, and has its own hermetic suite, wired into the
  validate job.

ProfilerTestRunner

- The musl XML path discarded mkdirs()'s result, and the listener reports
  write failures to a PrintWriter rather than throwing, so a musl run could
  finish with no TEST-*.xml and look like a cell with nothing to report. It
  now fails loudly when reports were requested and cannot be written.

Also: the non-test-task guard was asserted on stdout the fixture echoed
itself, so deleting it left the test green; it is now asserted on the recorded
gate_reason with the fixture exiting 0 to isolate it from the exit-code guard.
And the job-name regex is checked against test_workflow.yml's own name:
expressions, so a rename fails in the validate job rather than silently
rendering a summary that reports zero test jobs.

56 quarantine assertions, 4 summary, 2 prepare-reports.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running the quarantine scripts or their tests locally leaves
.github/scripts/__pycache__/, which showed up as untracked in every worktree
that had run them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@rkennke
rkennke marked this pull request as ready for review September 15, 2026 12:35
@rkennke
rkennke requested a review from a team as a code owner September 15, 2026 12:35

@datadog-official datadog-official Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Datadog Autotest: FAIL

A normal musl test failure makes Gradle print the same text that the classifier treats as a dead test JVM. This makes every quarantined musl failure keep the CI job red.

Open Bits AI session

🤖 Datadog Autotest · Commit 5f01c39 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest

# itself cannot separate "the quarantined test failed" from "the JVM died with
# tests still unrun" -- but Gradle names the latter explicitly.
_CUT_SHORT_RE = re.compile(
r"finished with non-zero exit value"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Normal musl failures look like test JVM crashes

Quarantine cannot suspend CI gating for musl test failures.

Assertion details
  • Input: A musl test cell completes with a quarantined test failure.
  • Expected: A complete musl run with only quarantined failures must not gate the CI job.
  • Actual: ProfilerTestRunner exits with code 1 for a normal assertion failure. Gradle Exec then prints this marker. The classifier treats the run as cut short and keeps the job red.

Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest · Open Bits AI session

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5f01c39cc0

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

# itself cannot separate "the quarantined test failed" from "the JVM died with
# tests still unrun" -- but Gradle names the latter explicitly.
_CUT_SHORT_RE = re.compile(
r"finished with non-zero exit value"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Distinguish ordinary musl Exec failures from JVM crashes

When a quarantined test fails on a musl cell, ProfilerTestRunner exits 1 and the Gradle Exec task reports that its process “finished with non-zero exit value”; this regex therefore marks every ordinary persistent or single-attempt musl test failure as a cut-short run. The gating branch then overrides the quarantine hit and leaves both musl workflows red, so quarantine cannot work for slow musl suites or for a failure that reproduces on retry. Restrict this marker to native Test-executor crashes or otherwise account for musl's Exec task semantics.

AGENTS.md reference: AGENTS.md:L221-L225

Useful? React with 👍 / 👎.

Comment on lines +191 to +193
widths = {len(f) for f in fields}
if len(widths) != 1:
return None

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep proposals narrow across mixed JDK label widths

When the same failure occurs in cells such as glibc-8-j9-debug-amd64 and glibc-17-debug-amd64—both forms occur in the workflow matrix—the split arrays have different widths and this returns None. render_proposals() converts that to an empty cells list, which format_entry() renders as -, so the paste-ready proposal quarantines the test on every libc, configuration, architecture, and slow suite rather than just the observed cells. Parse the fixed axes from the ends or fall back to listing the observed cells instead of proposing a global quarantine.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sphinx:critical Sphinx: critical — human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant