From 85ef1742733045b6c72347d2f0525fb8ee310bbd Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Wed, 2 Sep 2026 20:42:45 +0200 Subject: [PATCH 1/9] ci: quarantine flaky tests against a ticket instead of retrying past 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. --- .github/scripts/flake_report.py | 145 ++++++++++++++ .github/scripts/flake_summary.py | 183 ++++++++++++++++++ .github/scripts/generate-test-summary.sh | 80 ++++---- .github/scripts/prepare_reports.sh | 4 + .github/scripts/quarantine.py | 191 +++++++++++++++++++ .github/scripts/run_tests_with_retry.sh | 151 +++++++++++++++ .github/scripts/tests/test_quarantine.sh | 229 +++++++++++++++++++++++ .github/workflows/ci.yml | 18 ++ .github/workflows/test_workflow.yml | 135 ++++++++----- ddprof-test/quarantine.txt | 35 ++++ 10 files changed, 1078 insertions(+), 93 deletions(-) create mode 100755 .github/scripts/flake_report.py create mode 100755 .github/scripts/flake_summary.py create mode 100755 .github/scripts/quarantine.py create mode 100755 .github/scripts/run_tests_with_retry.sh create mode 100755 .github/scripts/tests/test_quarantine.sh create mode 100644 ddprof-test/quarantine.txt diff --git a/.github/scripts/flake_report.py b/.github/scripts/flake_report.py new file mode 100755 index 0000000000..1f858d21a8 --- /dev/null +++ b/.github/scripts/flake_report.py @@ -0,0 +1,145 @@ +#!/usr/bin/env python3 +"""Classify a cell's test failures and decide whether they should gate. + +Two questions, answered separately: + + Is it flaky? Failed on one attempt and passed on another. This is what the + retry exists to establish -- it does NOT excuse the failure. + Does it gate? Only the quarantine list answers that. A failure not on the + list turns the job red whether it is flaky or broken, which is + what keeps a flake from being quietly tolerated forever. + +So a flaky test still fails CI until somebody quarantines it with a ticket. To +make that cheap, `report` prints a filled-in quarantine entry to paste. +""" + +import argparse +import glob +import json +import os +import sys +import xml.etree.ElementTree as ET + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) +import quarantine # noqa: E402 + + +def _attempt_number(path): + return int(path.rsplit("-", 1)[1]) + + +def failed_tests(root_dir): + """Map of "class.test" -> first line of the failure message, for JUnit XML + anywhere under root_dir.""" + failures = {} + pattern = os.path.join(root_dir, "**", "TEST-*.xml") + for path in glob.glob(pattern, recursive=True): + try: + tree = ET.parse(path) + except ET.ParseError: + # A JVM that died mid-suite leaves a truncated report. That is not + # evidence the tests in it passed, but it is not attributable to a + # named test either, so it is left to the exit code to report. + continue + for case in tree.iter("testcase"): + problem = case.find("failure") + if problem is None: + problem = case.find("error") + if problem is None: + continue + test_id = "{}.{}".format(case.get("classname") or "", case.get("name") or "") + message = (problem.get("message") or problem.get("type") or "").strip() + failures[test_id] = message.splitlines()[0][:200] if message else "failed" + return failures + + +def collect_attempts(evidence_dir): + """[(attempt number, failures)] ordered by attempt.""" + dirs = glob.glob(os.path.join(evidence_dir, "attempt-*")) + return [(_attempt_number(d), failed_tests(d)) for d in sorted(dirs, key=_attempt_number)] + + +def cmd_count(args): + print(len(failed_tests(args.dir))) + return 0 + + +def cmd_report(args): + attempts = collect_attempts(args.evidence_dir) + ran = len(attempts) + entries = quarantine.load(args.list) + + results = [] + for test_id in sorted({t for _, f in attempts for t in f}): + failed_in = [n for n, f in attempts if test_id in f] + hit = next( + (e for e in entries if quarantine.covers(e, test_id) and quarantine.applies_to(e, args.cell)), + None, + ) + results.append({ + "test": test_id, + "failed_attempts": failed_in, + "message": next(f[test_id] for _, f in attempts if test_id in f), + # Passing on any attempt is what makes it flaky, so a test that + # failed in fewer attempts than were run has passed at least once. + "flaky": len(failed_in) < ran, + "quarantined": hit is not None, + "ticket": hit.get("ticket") if hit else None, + }) + + gating = [r for r in results if not r["quarantined"]] + + report = { + "cell": args.cell, + "attempts": ran, + "status": args.final_status, + "flaky": [r for r in results if r["flaky"] and not r["quarantined"]], + "persistent": [r for r in results if not r["flaky"] and not r["quarantined"]], + "quarantined": [r for r in results if r["quarantined"]], + "gating_count": len(gating), + "failure_count": len(results), + } + + os.makedirs(os.path.dirname(args.out) or ".", exist_ok=True) + with open(args.out, "w") as handle: + json.dump(report, handle, indent=2) + handle.write("\n") + + for entry in report["quarantined"]: + print("::notice title=Quarantined test failed::{}: {} ({}) — not gating".format( + args.cell, entry["test"], entry["ticket"])) + + for entry in report["flaky"]: + attempts_desc = ", ".join(str(n) for n in entry["failed_attempts"]) + print("::error title=Flaky test::{}: {} failed on attempt {} and passed on retry. " + "It is not quarantined, so it fails the build. See the PR comment for a " + "quarantine entry to paste.".format(args.cell, entry["test"], attempts_desc)) + + print("[flake-report] {}: {} flaky, {} persistent, {} quarantined; {} gating".format( + args.cell, len(report["flaky"]), len(report["persistent"]), + len(report["quarantined"]), report["gating_count"]), file=sys.stderr) + return 0 + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--list", default=quarantine.DEFAULT_LIST) + sub = parser.add_subparsers(dest="command", required=True) + + count = sub.add_parser("count", help="print the number of distinct failed tests") + count.add_argument("--dir", required=True) + count.set_defaults(func=cmd_count) + + report = sub.add_parser("report", help="classify failures and decide gating") + report.add_argument("--cell", required=True) + report.add_argument("--evidence-dir", required=True) + report.add_argument("--final-status", required=True, choices=["pass", "fail"]) + report.add_argument("--out", required=True) + report.set_defaults(func=cmd_report) + + args = parser.parse_args() + return args.func(args) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/scripts/flake_summary.py b/.github/scripts/flake_summary.py new file mode 100755 index 0000000000..5ccff49862 --- /dev/null +++ b/.github/scripts/flake_summary.py @@ -0,0 +1,183 @@ +#!/usr/bin/env python3 +"""Turn the per-cell reports written by flake_report.py into PR-comment markdown. + +The matrix runs the same suite across dozens of cells, so the useful unit is the +test, not the cell: one flaky test shows up as eight red cells, and eight +unrelated breakages also show up as eight red cells. Grouping by test tells +those apart. + +For anything that looks flaky, this also prints the quarantine entry to paste +and what to do with it. The judgement -- is this really flaky, is it worth a +ticket -- stays with a person; the typing does not. +""" + +import argparse +import datetime +import glob +import json +import os +import sys +from collections import OrderedDict + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) +import quarantine # noqa: E402 + +DEFAULT_REVIEW_DAYS = quarantine.DEFAULT_REVIEW_DAYS + + +def load_reports(root_dir): + reports = [] + for path in sorted(glob.glob(os.path.join(root_dir, "**", "*.json"), recursive=True)): + try: + with open(path) as handle: + data = json.load(handle) + except (OSError, ValueError): + continue + if isinstance(data, dict) and "cell" in data: + reports.append(data) + return reports + + +def group_by_test(reports, key): + """OrderedDict of test id -> {cells, message, ticket}.""" + grouped = OrderedDict() + for report in reports: + for entry in report.get(key, []): + slot = grouped.setdefault(entry["test"], { + "cells": [], + "message": entry.get("message", ""), + "ticket": entry.get("ticket"), + }) + slot["cells"].append(report["cell"]) + return grouped + + +def short_name(test_id): + """com.datadoghq.profiler.FooTest.bar -> FooTest.bar""" + parts = test_id.rsplit(".", 2) + return ".".join(parts[-2:]) if len(parts) >= 2 else test_id + + +def render_table(grouped, cell_limit=4, row_limit=25, ticket_column=False): + header = "| Test | Cells | " + ("Ticket | " if ticket_column else "") + "Message |" + rule = "|------|-------|" + ("--------|" if ticket_column else "") + "---------|" + lines = [header, rule] + for test_id, info in list(grouped.items())[:row_limit]: + cells = info["cells"] + shown = ", ".join("`{}`".format(c) for c in cells[:cell_limit]) + if len(cells) > cell_limit: + shown += " _+{} more_".format(len(cells) - cell_limit) + message = (info["message"] or "").replace("|", "\\|")[:120] + ticket = "{} | ".format(info.get("ticket") or "—") if ticket_column else "" + lines.append("| `{}` | {} | {}{} |".format(short_name(test_id), shown, ticket, message)) + if len(grouped) > row_limit: + lines.append("") + lines.append("_...and {} more. See the job logs._".format(len(grouped) - row_limit)) + return lines + + +def cells_glob(cells): + """A glob covering these cells, when they share an obvious axis. + + Suggesting `*arm64*` for something that only ever failed on arm64 is more + useful than listing four cell names, and narrower than quarantining + everywhere -- which would hide the same test breaking on x64 tomorrow. + """ + for axis in ("arm64", "aarch64", "musl", "asan", "tsan"): + if all(axis in c for c in cells): + return ["*{}*".format(axis)] + return None + + +def render_proposals(flaky): + today = datetime.date.today() + review_by = (today + datetime.timedelta(days=DEFAULT_REVIEW_DAYS)).isoformat() + out = [ + "
", + "Consider quarantining these — click for ready-made entries", + "", + "A quarantined test still runs and still reports; its failures just stop", + "turning CI red. To quarantine one:", + "", + "1. Open a **PROF** ticket for the test, linking the failing job.", + "2. Append the line below to `ddprof-test/quarantine.txt`, replacing", + " `PROF-XXXXX` with the ticket number.", + "3. Check the `cells` and `reason` columns — the proposal only knows what", + " failed in this run, and a narrower `cells` glob keeps the same test", + " gating everywhere it has not misbehaved.", + "", + "CI fails once `review_by` passes, so an entry expires instead of piling up.", + "", + "```", + "# test | ticket | added | review_by | cells | reason", + ] + for test_id, info in flaky.items(): + reason = "{} (seen in: {})".format( + info["message"] or "intermittent failure", + ", ".join(sorted(set(info["cells"]))[:4]), + ).replace("|", "/") + out.append(quarantine.format_entry( + test_id, + "PROF-XXXXX", + today.isoformat(), + review_by, + cells_glob(info["cells"]) or [], + reason, + )) + out.append("```") + out.append("") + out.append("
") + out.append("") + return out + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--dir", required=True, help="directory of downloaded ci-outcome artifacts") + args = parser.parse_args() + + reports = load_reports(args.dir) + if not reports: + return 0 + + flaky = group_by_test(reports, "flaky") + persistent = group_by_test(reports, "persistent") + quarantined = group_by_test(reports, "quarantined") + + out = [] + if flaky: + out.append("### :warning: Flaky tests — failed, then passed on retry") + out.append("") + out.extend(render_table(flaky)) + out.append("") + out.append( + "**These fail the build.** Passing on a second run makes a test flaky, " + "not passing. Fix it, or quarantine it against a ticket so the debt is " + "tracked rather than forgotten." + ) + out.append("") + out.extend(render_proposals(flaky)) + if persistent: + out.append("### :x: Failing tests") + out.append("") + out.extend(render_table(persistent)) + out.append("") + if quarantined: + out.append("### :mute: Quarantined failures — not gating") + out.append("") + out.extend(render_table(quarantined, ticket_column=True)) + out.append("") + + retried = [r for r in reports if r.get("attempts", 1) > 1] + if retried: + out.append("_Retried {} of {} cells._".format(len(retried), len(reports))) + out.append("") + + sys.stdout.write("\n".join(out)) + if out: + sys.stdout.write("\n") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/scripts/generate-test-summary.sh b/.github/scripts/generate-test-summary.sh index a6cbcfcc58..454828a75d 100755 --- a/.github/scripts/generate-test-summary.sh +++ b/.github/scripts/generate-test-summary.sh @@ -77,6 +77,8 @@ declare -A job_url=() job_url["__init__"]=1; unset 'job_url[__init__]' declare -A job_duration=() job_duration["__init__"]=1; unset 'job_duration[__init__]' +declare -A job_cell=() +job_cell["__init__"]=1; unset 'job_cell[__init__]' declare -a failed_jobs=() declare -a all_platforms=() declare -a all_java_versions=() @@ -116,6 +118,8 @@ while IFS= read -r job; do job_status["$key"]="$conclusion" job_url["$key"]="$html_url" job_duration["$key"]="$duration" + # Matches the cell label run_tests_with_retry.sh names its report after. + job_cell["$key"]="${libc}-${java_version}-${config}-${arch}" # Track failed jobs if [[ "$conclusion" == "failure" ]]; then @@ -176,51 +180,30 @@ done declare -A failure_details=() failure_details["__init__"]=1; unset 'failure_details[__init__]' -if ((failed_count > 0)); then - log "Downloading failure artifacts..." - mkdir -p ./failure-artifacts - - # Try to download test reports - gh run download "$RUN_ID" --pattern '(test-reports)*' --dir ./failure-artifacts 2>/dev/null || true - - # Parse JUnit XML for failure details - for key in "${failed_jobs[@]}"; do - IFS='|' read -r platform java_version <<< "$key" - - # Find matching test report directory - # Pattern: (test-reports) test-linux-{libc}-{arch} ({java}, {config}) - IFS='/' read -r libc_arch config <<< "$platform" - report_pattern="./failure-artifacts/*${libc_arch}*${java_version}*${config}*" - - failures="" - for report_dir in $report_pattern; do - if [[ -d "$report_dir" ]]; then - # Parse JUnit XML files - for xml_file in "$report_dir"/**/TEST-*.xml; do - if [[ -f "$xml_file" ]]; then - # Extract failed test cases - while IFS= read -r testcase; do - classname=$(echo "$testcase" | grep -oP 'classname="\K[^"]+' || echo "") - testname=$(echo "$testcase" | grep -oP 'name="\K[^"]+' || echo "") - # Get failure message (first line only, truncated) - failure_msg=$(echo "$testcase" | grep -oP ']*message="\K[^"]*' | head -c 100 || echo "") - - if [[ -n "$classname" && -n "$testname" ]]; then - short_class="${classname##*.}" - failures+="| \`${short_class}.${testname}\` | ${failure_msg:-Test failed} |"$'\n' - fi - done < <(grep -Pzo '(?s)]*>.*?' "$xml_file" 2>/dev/null | tr '\0' '\n' | grep -E '<(failure|error)' || true) - fi - done - fi - done - - failure_details["$key"]="$failures" - done - - # Cleanup - rm -rf ./failure-artifacts -fi +# Per-cell outcome reports, written by run_tests_with_retry.sh and uploaded +# whether the cell passed or failed. A cell that only went green on a retry +# produces no failure artifact at all, so this is the one place its flaky test +# is recorded. +OUTCOME_DIR="./ci-outcome-artifacts" +log "Downloading CI outcome reports..." +mkdir -p "$OUTCOME_DIR" +gh run download "$RUN_ID" --pattern '(ci-outcome)*' --dir "$OUTCOME_DIR" 2>/dev/null || true + +for key in "${failed_jobs[@]}"; do + cell="${job_cell[$key]:-}" + [[ -n "$cell" ]] || continue + + failures="" + while IFS= read -r report; do + while IFS=$'\t' read -r test_id message; do + [[ -n "$test_id" ]] || continue + short_name="${test_id#"${test_id%.*.*}."}" + failures+="| \`${short_name}\` | ${message:-Test failed} |"$'\n' + done < <(jq -r '.persistent[] | [.test, .message] | @tsv' "$report" 2>/dev/null || true) + done < <(find "$OUTCOME_DIR" -name "${cell}.json" 2>/dev/null) + + failure_details["$key"]="$failures" +done # --- Generate markdown --- log "Generating markdown summary..." @@ -284,6 +267,11 @@ log "Generating markdown summary..." echo "" fi + # Flaky and failing tests, grouped by test rather than by cell. One flaky + # test reddens a dozen cells and so does a dozen unrelated breakages; only + # grouping by test tells those apart. + python3 "$(dirname "$0")/flake_summary.py" --dir "$OUTCOME_DIR" || true + # Failed tests details if ((failed_count > 0)); then echo "### Failed Tests" @@ -331,5 +319,7 @@ log "Generating markdown summary..." } > "$OUTPUT_FILE" +rm -rf "$OUTCOME_DIR" + log "Summary written to $OUTPUT_FILE" log "Total jobs: $total_jobs, Passed: $passed_jobs, Failed: $failed_count" diff --git a/.github/scripts/prepare_reports.sh b/.github/scripts/prepare_reports.sh index 4ff852450e..3f410de5ca 100755 --- a/.github/scripts/prepare_reports.sh +++ b/.github/scripts/prepare_reports.sh @@ -12,6 +12,10 @@ cp ddprof-test/javacore*.txt test-reports/ || true cp ddprof-test/build/hs_err* test-reports/ || true cp -r ddprof-lib/build/tmp test-reports/native_build || true cp -r ddprof-test/build/reports/tests test-reports/tests || true +# The JUnit XML, not just the rendered HTML: it is what names the failed tests +# for the PR summary, and what flake_report.py compares between retry attempts. +cp -r ddprof-test/build/test-results test-reports/test-results || true +cp -r flake-evidence test-reports/flake-evidence || true cp build/logs/gdb-watchdog.log test-reports/ || true cp -r /tmp/recordings test-reports/recordings || true find ddprof-lib/build -name 'libjavaProfiler.*' -exec cp {} test-reports/ \; || true diff --git a/.github/scripts/quarantine.py b/.github/scripts/quarantine.py new file mode 100755 index 0000000000..590094f242 --- /dev/null +++ b/.github/scripts/quarantine.py @@ -0,0 +1,191 @@ +#!/usr/bin/env python3 +"""The quarantine list: which failing tests do not turn CI red. + +Three jobs, one per subcommand: + + match split a cell's failures into gating and quarantined + validate enforce the format, the ticket, and the review_by date + propose print an entry ready to paste for a test CI thinks is flaky + +The list is a plain text table (see ddprof-test/quarantine.txt) rather than +JSON or YAML: it is edited by hand far more often than by machine, so real +comments, one-line diffs and clean `git blame` matter more than a schema. It +also has to parse inside the Alpine test containers, where PyYAML cannot be +assumed -- this needs nothing but str.split. +""" + +import argparse +import datetime +import fnmatch +import json +import os +import re +import sys + +DEFAULT_LIST = os.path.join("ddprof-test", "quarantine.txt") +TICKET_RE = re.compile(r"^PROF-\d+$") +DATE_RE = re.compile(r"^\d{4}-\d{2}-\d{2}$") +FIELDS = ("test", "ticket", "added", "review_by", "cells", "reason") +# Long enough not to be busywork, short enough that a quarantine outlives +# neither the release it was added in nor the memory of why. +DEFAULT_REVIEW_DAYS = 90 + + +def parse(path): + """([entry], [(line number, message)]) — entries and malformed lines. + + Each entry carries `_line` so validate() can point at the offender. + """ + entries, errors = [], [] + if not os.path.exists(path): + return entries, errors + + with open(path) as handle: + for number, raw in enumerate(handle, start=1): + line = raw.strip() + if not line or line.startswith("#"): + continue + + parts = [p.strip() for p in line.split("|")] + if len(parts) != len(FIELDS): + errors.append((number, "expected {} fields separated by '|', found {}".format( + len(FIELDS), len(parts)))) + continue + + entry = dict(zip(FIELDS, parts)) + entry["cells"] = [c.strip() for c in entry["cells"].split(",") + if c.strip() and c.strip() != "-"] + entry["_line"] = number + entries.append(entry) + + return entries, errors + + +def load(path): + """Entries only, for callers that just need to match against the list.""" + return parse(path)[0] + + +def applies_to(entry, cell): + """Does this entry cover the given cell? No globs means everywhere.""" + globs = entry.get("cells") + if not globs: + return True + return any(fnmatch.fnmatch(cell, g) for g in globs) + + +def covers(entry, test_id): + pattern = entry["test"] + if pattern.endswith(".*"): + return test_id.startswith(pattern[:-1]) + return test_id == pattern + + +def format_entry(test, ticket, added, review_by, cells, reason): + return " | ".join([test, ticket, added, review_by, ",".join(cells) or "-", reason]) + + +def cmd_match(args): + entries = load(args.list) + failures = [line.strip() for line in sys.stdin if line.strip()] + + gating, quarantined = [], [] + for test_id in failures: + hit = next( + (e for e in entries if covers(e, test_id) and applies_to(e, args.cell)), + None, + ) + (quarantined if hit else gating).append(test_id) + + json.dump({"gating": gating, "quarantined": quarantined}, sys.stdout) + sys.stdout.write("\n") + return 0 + + +def cmd_validate(args): + entries, problems = parse(args.list) + today = datetime.date.today() + seen = {} + + def complain(line, message): + problems.append((line, message)) + + for entry in entries: + line = entry["_line"] + name = entry["test"] + + for field in FIELDS: + if field == "cells": + continue # optional, normalised to [] above + if not entry[field]: + complain(line, "field '{}' is empty".format(field)) + + if name in seen: + complain(line, "'{}' is already quarantined on line {}".format(name, seen[name])) + seen[name] = line + + if entry["ticket"] and not TICKET_RE.match(entry["ticket"]): + complain(line, "ticket '{}' is not a PROF-".format(entry["ticket"])) + + for field in ("added", "review_by"): + if entry[field] and not DATE_RE.match(entry[field]): + complain(line, "{} '{}' is not YYYY-MM-DD".format(field, entry[field])) + + if DATE_RE.match(entry["review_by"]): + due = datetime.date.fromisoformat(entry["review_by"]) + if due < today: + complain(line, ( + "'{}' has been quarantined since {} and its review was due {} " + "({} days ago). Fix the test and delete this line, or renew " + "review_by with a note on {}." + ).format(name, entry["added"], entry["review_by"], + (today - due).days, entry["ticket"] or "the ticket")) + + for line, message in sorted(problems): + print("::error file={},line={}::{}".format(args.list, line, message)) + + if problems: + print("\n{} problem(s) in {}".format(len(problems), args.list), file=sys.stderr) + return 1 + + print("{}: {} quarantined test(s), all valid".format(args.list, len(entries))) + return 0 + + +def cmd_propose(args): + today = datetime.date.today() + print(format_entry( + args.test, + "PROF-XXXXX", + today.isoformat(), + (today + datetime.timedelta(days=DEFAULT_REVIEW_DAYS)).isoformat(), + args.cells or [], + args.reason, + )) + return 0 + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--list", default=DEFAULT_LIST) + sub = parser.add_subparsers(dest="command", required=True) + + match = sub.add_parser("match", help="split stdin's failed test ids by quarantine status") + match.add_argument("--cell", required=True) + match.set_defaults(func=cmd_match) + + validate = sub.add_parser("validate", help="check the list's format and review dates") + validate.set_defaults(func=cmd_validate) + + propose = sub.add_parser("propose", help="print a paste-ready entry") + propose.add_argument("--test", required=True) + propose.add_argument("--reason", required=True) + propose.add_argument("--cells", nargs="*") + propose.set_defaults(func=cmd_propose) + + args = parser.parse_args() + return args.func(args) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/scripts/run_tests_with_retry.sh b/.github/scripts/run_tests_with_retry.sh new file mode 100755 index 0000000000..9c0048f9b9 --- /dev/null +++ b/.github/scripts/run_tests_with_retry.sh @@ -0,0 +1,151 @@ +#!/usr/bin/env bash +# Run a test suite, retry once to find out whether a failure reproduces, and +# let the quarantine list -- not the retry -- decide whether the job goes red. +# +# Usage: run_tests_with_retry.sh [--list ] -- +# +# The command is passed through verbatim, so a caller can hand over a plain +# ./gradlew invocation, one wrapped in setarch, or the docker run that drives +# the Alpine aarch64 suite. +# +# Environment: +# MAX_ATTEMPTS attempts to allow (default 2; 1 disables retry) +# MAX_FAILURES_TO_RETRY don't retry past this many failed tests (default 3) +# RETRY_ON_NO_TEST_FAILURES retry a failure that named no test (default 0) +# +# The retry buys a label, not a pass. A test that fails then passes is flaky; a +# test that fails twice is broken. Both still fail the build unless quarantined +# -- the difference decides what the PR comment advises, not whether CI is green. +# +# A retry is spent only when the shape of the failure suggests it might not +# reproduce: a handful of failed tests. A suite where fifty tests went red, or +# where none did (a compile error, an OOM-killed runner, a JVM that never +# started), is not flakiness and a second run only doubles the wait. +# +# The retry is a full re-run rather than a `--tests` filter over the failures. +# Re-running a test alone would clear any failure that only happens in company +# -- an ordering or shared-state bug -- and a test mislabelled "flaky" invites a +# quarantine entry that buries a real defect. + +set -uo pipefail + +QUARANTINE_LIST="ddprof-test/quarantine.txt" +if [ "${1:-}" = "--list" ]; then + QUARANTINE_LIST="$2" + shift 2 +fi + +CELL="${1:?usage: run_tests_with_retry.sh [--list ] -- }" +shift +[ "${1:-}" = "--" ] && shift + +MAX_ATTEMPTS="${MAX_ATTEMPTS:-2}" +MAX_FAILURES_TO_RETRY="${MAX_FAILURES_TO_RETRY:-3}" +RETRY_ON_NO_TEST_FAILURES="${RETRY_ON_NO_TEST_FAILURES:-0}" + +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +RESULTS_DIR="ddprof-test/build/test-results" +EVIDENCE_DIR="flake-evidence" +OUTCOME_FILE="ci-outcome/${CELL}.json" + +# Snapshot this attempt's JUnit XML before the next one overwrites it -- the +# whole point is to compare attempts, and Gradle reuses the same directory. +snapshot() { + local attempt="$1" + local dest="${EVIDENCE_DIR}/attempt-${attempt}" + rm -rf "$dest" + mkdir -p "$dest" + if [ -d "$RESULTS_DIR" ]; then + cp -r "$RESULTS_DIR"/. "$dest"/ 2>/dev/null || true + fi +} + +EXIT_CODE=1 +for attempt in $(seq 1 "$MAX_ATTEMPTS"); do + mkdir -p build/logs + rm -rf "$RESULTS_DIR" + + "$@" 2>&1 \ + | tee -a build/test-raw.log \ + | python3 -u "${HERE}/filter_gradle_log.py" + EXIT_CODE=${PIPESTATUS[0]} + + snapshot "$attempt" + + if [ "$EXIT_CODE" -eq 0 ]; then + break + fi + + if [ "$attempt" -ge "$MAX_ATTEMPTS" ]; then + break + fi + + failed=$(python3 "${HERE}/flake_report.py" count --dir "${EVIDENCE_DIR}/attempt-${attempt}") + if [ "$failed" -eq 0 ] && [ "$RETRY_ON_NO_TEST_FAILURES" != "1" ]; then + # No test was named, so the suite did not get far enough to have one fail: + # a compile error, a missing toolchain, a runner that ran out of disk. None + # of those get better on a second run. + echo "::notice::Attempt ${attempt} failed with no named test failures (build or infrastructure); not retrying" + break + fi + if [ "$failed" -gt "$MAX_FAILURES_TO_RETRY" ]; then + echo "::notice::Attempt ${attempt} failed ${failed} tests (> ${MAX_FAILURES_TO_RETRY}); a break, not a flake — not retrying" + break + fi + + if [ "$failed" -eq 0 ]; then + echo "::warning::Attempt ${attempt} failed before any test ran, retrying once" + else + echo "::warning::Attempt ${attempt} failed ${failed} test(s), retrying once to tell a flake from a break" + fi + ./gradlew --stop 2>/dev/null || true +done + +if [ "$EXIT_CODE" -eq 0 ]; then + FINAL_STATUS=pass +else + FINAL_STATUS=fail +fi + +python3 "${HERE}/flake_report.py" --list "$QUARANTINE_LIST" report \ + --cell "$CELL" \ + --evidence-dir "$EVIDENCE_DIR" \ + --final-status "$FINAL_STATUS" \ + --out "$OUTCOME_FILE" +REPORT_STATUS=$? + +# A classifier that did not run cannot vouch for a green suite: it is the only +# thing that would have noticed a test failing on the first attempt and passing +# on the second. Fail loudly rather than inherit a pass we cannot justify. +if [ "$REPORT_STATUS" -ne 0 ]; then + echo "::error::Could not classify results for ${CELL} (flake_report.py exited ${REPORT_STATUS}); failing the job rather than trusting an unexamined pass" + exit 1 +fi + +# The quarantine list, not the retry, decides whether the job goes red. +# +# any un-quarantined failure -> red, even if the retry passed. A flake that +# nobody has quarantined is still a failure; +# letting the retry excuse it is how flakes get +# tolerated for years. +# every failure quarantined -> green. That is what the list is for, and the +# entry behind it carries a ticket and a date. +# no test named -> keep the command's own exit code: a compile +# error or a dead runner is nothing to do with +# quarantine. +if [ -f "$OUTCOME_FILE" ]; then + read -r gating failures <<< "$(python3 -c " +import json, sys +d = json.load(open(sys.argv[1])) +print(d['gating_count'], d['failure_count']) +" "$OUTCOME_FILE")" + + if [ "${gating:-0}" -gt 0 ]; then + EXIT_CODE=1 + elif [ "${failures:-0}" -gt 0 ]; then + echo "::warning::All ${failures} failing test(s) in ${CELL} are quarantined; not failing the job" + EXIT_CODE=0 + fi +fi + +exit "$EXIT_CODE" diff --git a/.github/scripts/tests/test_quarantine.sh b/.github/scripts/tests/test_quarantine.sh new file mode 100755 index 0000000000..1921b578a0 --- /dev/null +++ b/.github/scripts/tests/test_quarantine.sh @@ -0,0 +1,229 @@ +#!/usr/bin/env bash + +set -euo pipefail + +# Copyright 2026, Datadog, Inc + +# Hermetic tests for the flaky-test quarantine machinery. +# Run with: .github/scripts/tests/test_quarantine.sh +# +# The gating decision here is the one that can let a real defect through, and +# the retry path only executes when something has already failed -- which is to +# say, never on a green CI run. So it is exercised against fixtures instead. + +ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")/../../.." && pwd) +SCRIPTS="$ROOT/.github/scripts" +TEMP_DIR=$(mktemp -d) +TESTS=0 + +cleanup() { + rm -rf "$TEMP_DIR" +} +trap cleanup EXIT + +fail() { + echo "FAIL: $*" >&2 + exit 1 +} + +pass() { + TESTS=$((TESTS + 1)) + echo " ok: $*" +} + +today() { python3 -c 'import datetime; print(datetime.date.today())'; } +day_offset() { python3 -c "import datetime,sys; print(datetime.date.today()+datetime.timedelta(days=int(sys.argv[1])))" "$1"; } + +write_list() { + # write_list [entry line...] + local path="$1"; shift + printf '# test | ticket | added | review_by | cells | reason\n' > "$path" + local line + for line in "$@"; do + printf '%s\n' "$line" >> "$path" + done +} + +entry() { + # entry [cells] + printf '%s | %s | %s | %s | %s | flaky under test\n' \ + "$1" "$2" "$(today)" "$3" "${4:--}" +} + +# Writes a JUnit XML report naming one failed test. +write_failure_xml() { + # write_failure_xml + mkdir -p "$1" + cat > "$1/TEST-$2.xml" < + + + + + +EOF +} + +write_pass_xml() { + mkdir -p "$1" + cat > "$1/TEST-$2.xml" < + + + +EOF +} + +echo "== quarantine.py validate ==" + +LIST="$TEMP_DIR/list.txt" + +write_list "$LIST" +python3 "$SCRIPTS/quarantine.py" --list "$LIST" validate >/dev/null \ + || fail "empty list should be valid" +pass "an empty list is valid" + +write_list "$LIST" "$(entry a.B.c PROF-1 "$(day_offset 30)")" +python3 "$SCRIPTS/quarantine.py" --list "$LIST" validate >/dev/null \ + || fail "a complete, unexpired entry should be valid" +pass "a complete entry is valid" + +write_list "$LIST" "a.B.c | | $(today) | $(day_offset 30) | - | no ticket" +if python3 "$SCRIPTS/quarantine.py" --list "$LIST" validate >/dev/null 2>&1; then + fail "an entry with no ticket should be rejected" +fi +pass "an entry with no ticket is rejected" + +write_list "$LIST" "$(entry a.B.c JIRA-1 "$(day_offset 30)")" +if python3 "$SCRIPTS/quarantine.py" --list "$LIST" validate >/dev/null 2>&1; then + fail "a ticket outside the PROF project should be rejected" +fi +pass "a non-PROF ticket is rejected" + +write_list "$LIST" "$(entry a.B.c PROF-1 "$(day_offset -1)")" +if python3 "$SCRIPTS/quarantine.py" --list "$LIST" validate >/dev/null 2>&1; then + fail "an entry past review_by should be rejected" +fi +pass "an expired entry is rejected" + +write_list "$LIST" "$(entry a.B.c PROF-1 "$(day_offset 30)")" "$(entry a.B.c PROF-2 "$(day_offset 30)")" +if python3 "$SCRIPTS/quarantine.py" --list "$LIST" validate >/dev/null 2>&1; then + fail "the same test listed twice should be rejected" +fi +pass "a duplicate entry is rejected" + +# The list that ships in the repo must itself be valid, or CI is lying. +python3 "$SCRIPTS/quarantine.py" --list "$ROOT/ddprof-test/quarantine.txt" validate >/dev/null \ + || fail "the committed quarantine list is invalid" +pass "the committed quarantine list is valid" + +echo "== quarantine.py match ==" + +write_list "$LIST" "$(entry a.B.c PROF-1 "$(day_offset 30)" '*arm64*')" + +result=$(printf 'a.B.c\n' | python3 "$SCRIPTS/quarantine.py" --list "$LIST" match --cell "glibc-17-debug-arm64") +echo "$result" | grep -q '"quarantined": \["a.B.c"\]' \ + || fail "expected a.B.c quarantined on an arm64 cell, got: $result" +pass "a cell glob matches the cells it names" + +result=$(printf 'a.B.c\n' | python3 "$SCRIPTS/quarantine.py" --list "$LIST" match --cell "glibc-17-debug-amd64") +echo "$result" | grep -q '"gating": \["a.B.c"\]' \ + || fail "expected a.B.c gating on an amd64 cell, got: $result" +pass "a cell glob does not match other cells" + +write_list "$LIST" "$(entry 'a.B.*' PROF-1 "$(day_offset 30)")" +result=$(printf 'a.B.c\na.B.d\na.C.e\n' | python3 "$SCRIPTS/quarantine.py" --list "$LIST" match --cell "any") +echo "$result" | grep -q '"gating": \["a.C.e"\]' \ + || fail "expected only a.C.e to gate under a class wildcard, got: $result" +pass "a class wildcard covers that class only" + +echo "== gating: run_tests_with_retry.sh ==" + +# A suite that fails one test on the first attempt and passes on the second. +make_flaky_suite() { + local dir="$1" + mkdir -p "$dir" + cat > "$dir/suite.sh" </dev/null || echo 0) + 1 )); echo \$n > .n +OUT=ddprof-test/build/test-results/testDebug +mkdir -p "\$OUT" +if [ "\$n" -eq 1 ]; then +$(declare -f write_failure_xml) + write_failure_xml "\$OUT" "com.dd.WobblyTest" "sometimesFails" "got 2 samples, wanted 50" + exit 1 +fi +$(declare -f write_pass_xml) +write_pass_xml "\$OUT" "com.dd.WobblyTest" "sometimesFails" +exit 0 +EOS + chmod +x "$dir/suite.sh" +} + +# Not quarantined: passing on the retry must not rescue the job. +CASE="$TEMP_DIR/case-gating" +make_flaky_suite "$CASE" +write_list "$CASE/list.txt" +set +e +(cd "$CASE" && "$SCRIPTS/run_tests_with_retry.sh" --list list.txt "glibc-17-debug-amd64" -- ./suite.sh >/dev/null 2>&1) +rc=$? +set -e +[ "$rc" -ne 0 ] || fail "an un-quarantined flaky test must fail the job (got exit $rc)" +python3 -c " +import json,sys +d = json.load(open(sys.argv[1])) +assert d['gating_count'] == 1, d +assert d['flaky'][0]['test'] == 'com.dd.WobblyTest.sometimesFails', d +" "$CASE/ci-outcome/glibc-17-debug-amd64.json" || fail "flaky test not classified as gating" +pass "an un-quarantined flake fails the job and is recorded as flaky" + +# Same suite, now quarantined: the job goes green and the failure is recorded. +CASE="$TEMP_DIR/case-quarantined" +make_flaky_suite "$CASE" +write_list "$CASE/list.txt" "$(entry com.dd.WobblyTest.sometimesFails PROF-1 "$(day_offset 30)")" +set +e +(cd "$CASE" && "$SCRIPTS/run_tests_with_retry.sh" --list list.txt "glibc-17-debug-amd64" -- ./suite.sh >/dev/null 2>&1) +rc=$? +set -e +[ "$rc" -eq 0 ] || fail "a quarantined test must not fail the job (got exit $rc)" +python3 -c " +import json,sys +d = json.load(open(sys.argv[1])) +assert d['gating_count'] == 0, d +assert d['quarantined'][0]['ticket'] == 'PROF-1', d +" "$CASE/ci-outcome/glibc-17-debug-amd64.json" || fail "quarantined failure not recorded" +pass "a quarantined failure keeps the job green and is still recorded" + +# A build error names no test, so quarantine has nothing to say about it. +CASE="$TEMP_DIR/case-build-error" +mkdir -p "$CASE" +printf '#!/usr/bin/env bash\necho "error: cannot find symbol"\nexit 1\n' > "$CASE/suite.sh" +chmod +x "$CASE/suite.sh" +write_list "$CASE/list.txt" "$(entry 'com.dd.WobblyTest.*' PROF-1 "$(day_offset 30)")" +set +e +(cd "$CASE" && "$SCRIPTS/run_tests_with_retry.sh" --list list.txt "glibc-17-debug-amd64" -- ./suite.sh >/dev/null 2>&1) +rc=$? +set -e +[ "$rc" -ne 0 ] || fail "a build error must fail the job regardless of the quarantine list" +pass "a failure naming no test is never excused by quarantine" + +# Regression: an unreadable list once made flake_report.py exit non-zero, and a +# `|| true` turned that into a silent green on a suite whose first attempt had +# failed. A classifier that did not run must never be mistaken for a clean run. +CASE="$TEMP_DIR/case-broken-list" +make_flaky_suite "$CASE" +printf 'this line has too few fields\n' > "$CASE/list.txt" +set +e +output=$(cd "$CASE" && "$SCRIPTS/run_tests_with_retry.sh" --list list.txt "glibc-17-debug-amd64" -- ./suite.sh 2>&1) +rc=$? +set -e +[ "$rc" -ne 0 ] || fail "a malformed list must not yield a green job (got exit $rc)" +pass "a list that cannot be read fails the job instead of passing silently" +# A malformed line is skipped rather than fatal, so the flake is still caught; +# either way the job must be red. +echo "$output" | grep -q "Flaky test\|Could not classify" \ + || fail "expected the flake or the classifier failure to be reported, got: $output" +pass "the reason for the red is reported" + +echo +echo "All $TESTS quarantine tests passed." diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 603579f378..5370bee391 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -45,6 +45,24 @@ jobs: .github/scripts/tests/test_release_automation.sh .github/scripts/tests/test_release_automation.sh + # Fails when a quarantine entry is malformed, ticketless, or past its + # review_by date. Without this the list only ever grows, and a quarantine + # becomes a permanent mute rather than tracked debt. + validate-quarantine: + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - name: Validate the test quarantine list + run: | + bash -n .github/scripts/run_tests_with_retry.sh + python3 .github/scripts/quarantine.py validate + .github/scripts/tests/test_quarantine.sh + check-for-pr: runs-on: ubuntu-latest outputs: diff --git a/.github/workflows/test_workflow.yml b/.github/workflows/test_workflow.yml index d611384666..a370e3a59b 100644 --- a/.github/workflows/test_workflow.yml +++ b/.github/workflows/test_workflow.yml @@ -155,26 +155,24 @@ jobs: exit 0 fi - MAX_ATTEMPTS=1 + # The slow/e2e suite already runs the best part of an hour, so a retry + # would risk the 180-minute job timeout. It records failures without + # re-running them. + export MAX_ATTEMPTS=${{ inputs.slow_tests && 1 || 2 }} + + # ASan init can nondeterministically collide with the JVM's + # ASLR-influenced mmap layout (google/sanitizers#856). The JVM aborts + # before any test runs, so that failure names no test and would + # otherwise be classed as a build error and left unretried. + export RETRY_ON_NO_TEST_FAILURES=0 if [[ "${{ matrix.config }}" == "asan" ]]; then - # ASan init can nondeterministically collide with the JVM's ASLR-influenced - # mmap layout (google/sanitizers#856); retry once before failing the job. - MAX_ATTEMPTS=2 + export RETRY_ON_NO_TEST_FAILURES=1 fi - for attempt in $(seq 1 $MAX_ATTEMPTS); do - mkdir -p build/logs - ${GRADLEW_PREFIX} ./gradlew -PCI -PkeepJFRs ${{ inputs.skip_gtest == true && '-Pskip-gtest' || '' }} :ddprof-test:test${{ inputs.slow_tests && 'Slow' || '' }}${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs 2>&1 \ - | tee -a build/test-raw.log \ - | python3 -u .github/scripts/filter_gradle_log.py - EXIT_CODE=${PIPESTATUS[0]} - - if [ $EXIT_CODE -eq 0 ]; then break; fi - if [ $attempt -lt $MAX_ATTEMPTS ]; then - echo "::warning::Attempt $attempt failed (exit $EXIT_CODE), retrying..." - ./gradlew --stop 2>/dev/null || true - fi - done + .github/scripts/run_tests_with_retry.sh \ + "glibc-${{ matrix.java_version }}-${{ matrix.config }}-amd64" -- \ + ${GRADLEW_PREFIX} ./gradlew -PCI -PkeepJFRs ${{ inputs.skip_gtest == true && '-Pskip-gtest' || '' }} :ddprof-test:test${{ inputs.slow_tests && 'Slow' || '' }}${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs + EXIT_CODE=$? # Kill the watchdog if tests finished before it fired if [[ -n "${GDB_WATCHDOG_PID:-}" ]]; then @@ -222,6 +220,16 @@ jobs: with: name: (test-reports) test-linux-glibc-amd64 (${{ matrix.java_version }}, ${{ matrix.config }}, ${{ inputs.slow_tests && 'slow' || 'regular' }}) path: test-reports + # Always uploaded, unlike the reports above: a cell that went green only + # because of a retry produces no failure artifact, and that is exactly the + # run whose evidence the summary needs. The file is a few hundred bytes. + - name: Upload CI outcome + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + if: always() + with: + name: (ci-outcome) test-linux-glibc-amd64 (${{ matrix.java_version }}, ${{ matrix.config }}, ${{ inputs.slow_tests && 'slow' || 'regular' }}) + path: ci-outcome + if-no-files-found: ignore - name: Upload signal-safety violation log uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 if: failure() @@ -310,11 +318,12 @@ jobs: export JAVA_VERSION echo "JAVA_VERSION=${JAVA_VERSION}" - mkdir -p build/logs - ./gradlew -PCI -PkeepJFRs :ddprof-test:test${{ inputs.slow_tests && 'Slow' || '' }}${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs 2>&1 \ - | tee -a build/test-raw.log \ - | python3 -u .github/scripts/filter_gradle_log.py - EXIT_CODE=${PIPESTATUS[0]} + export MAX_ATTEMPTS=${{ inputs.slow_tests && 1 || 2 }} + + .github/scripts/run_tests_with_retry.sh \ + "musl-${{ matrix.java_version }}-${{ matrix.config }}-amd64" -- \ + ./gradlew -PCI -PkeepJFRs :ddprof-test:test${{ inputs.slow_tests && 'Slow' || '' }}${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs + EXIT_CODE=$? if [ $EXIT_CODE -ne 0 ]; then echo "musl-${{ matrix.java_version }}-${{ matrix.config }}-amd64" >> failures_musl-${{ matrix.java_version }}-${{ matrix.config }}-amd64.txt @@ -357,6 +366,16 @@ jobs: with: name: (test-reports) test-linux-musl-amd64 (${{ matrix.java_version }}, ${{ matrix.config }}, ${{ inputs.slow_tests && 'slow' || 'regular' }}) path: test-reports + # Always uploaded, unlike the reports above: a cell that went green only + # because of a retry produces no failure artifact, and that is exactly the + # run whose evidence the summary needs. The file is a few hundred bytes. + - name: Upload CI outcome + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + if: always() + with: + name: (ci-outcome) test-linux-musl-amd64 (${{ matrix.java_version }}, ${{ matrix.config }}, ${{ inputs.slow_tests && 'slow' || 'regular' }}) + path: ci-outcome + if-no-files-found: ignore - name: Upload signal-safety violation log uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 if: failure() @@ -483,26 +502,24 @@ jobs: exit 0 fi - MAX_ATTEMPTS=1 + # The slow/e2e suite already runs the best part of an hour, so a retry + # would risk the 180-minute job timeout. It records failures without + # re-running them. + export MAX_ATTEMPTS=${{ inputs.slow_tests && 1 || 2 }} + + # ASan init can nondeterministically collide with the JVM's + # ASLR-influenced mmap layout (google/sanitizers#856). The JVM aborts + # before any test runs, so that failure names no test and would + # otherwise be classed as a build error and left unretried. + export RETRY_ON_NO_TEST_FAILURES=0 if [[ "${{ matrix.config }}" == "asan" ]]; then - # ASan init can nondeterministically collide with the JVM's ASLR-influenced - # mmap layout (google/sanitizers#856); retry once before failing the job. - MAX_ATTEMPTS=2 + export RETRY_ON_NO_TEST_FAILURES=1 fi - for attempt in $(seq 1 $MAX_ATTEMPTS); do - mkdir -p build/logs - ${GRADLEW_PREFIX} ./gradlew -PCI -PkeepJFRs ${{ inputs.skip_gtest == true && '-Pskip-gtest' || '' }} :ddprof-test:test${{ inputs.slow_tests && 'Slow' || '' }}${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs 2>&1 \ - | tee -a build/test-raw.log \ - | python3 -u .github/scripts/filter_gradle_log.py - EXIT_CODE=${PIPESTATUS[0]} - - if [ $EXIT_CODE -eq 0 ]; then break; fi - if [ $attempt -lt $MAX_ATTEMPTS ]; then - echo "::warning::Attempt $attempt failed (exit $EXIT_CODE), retrying..." - ./gradlew --stop 2>/dev/null || true - fi - done + .github/scripts/run_tests_with_retry.sh \ + "glibc-${{ matrix.java_version }}-${{ matrix.config }}-aarch64" -- \ + ${GRADLEW_PREFIX} ./gradlew -PCI -PkeepJFRs ${{ inputs.skip_gtest == true && '-Pskip-gtest' || '' }} :ddprof-test:test${{ inputs.slow_tests && 'Slow' || '' }}${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs + EXIT_CODE=$? # Kill the watchdog if tests finished before it fired if [[ -n "${GDB_WATCHDOG_PID:-}" ]]; then @@ -550,6 +567,16 @@ jobs: with: name: (test-reports) test-linux-glibc-aarch64 (${{ matrix.java_version }}, ${{ matrix.config }}, ${{ inputs.slow_tests && 'slow' || 'regular' }}) path: test-reports + # Always uploaded, unlike the reports above: a cell that went green only + # because of a retry produces no failure artifact, and that is exactly the + # run whose evidence the summary needs. The file is a few hundred bytes. + - name: Upload CI outcome + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + if: always() + with: + name: (ci-outcome) test-linux-glibc-aarch64 (${{ matrix.java_version }}, ${{ matrix.config }}, ${{ inputs.slow_tests && 'slow' || 'regular' }}) + path: ci-outcome + if-no-files-found: ignore - name: Upload signal-safety violation log uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 if: failure() @@ -608,16 +635,18 @@ jobs: set +e # the effective JAVA_VERSION is computed in the test_alpine_aarch64.sh script mkdir -p build/logs - docker run --cpus 4 --rm -v /tmp:/tmp -v "${GITHUB_WORKSPACE}:${GITHUB_WORKSPACE}" -w "${GITHUB_WORKSPACE}" alpine:3.21 /bin/sh -c " - \"$GITHUB_WORKSPACE/.github/scripts/test_alpine_aarch64.sh\" \ - \"${{ github.sha }}\" \"musl/${{ matrix.java_version }}-${{ matrix.config }}-aarch64\" \ - \"${{ matrix.config }}\" \"${{ env.JAVA_HOME }}\" \"${{ env.JAVA_TEST_HOME }}\" \ - \"${{ inputs.slow_tests }}\" - " 2>&1 \ - | tee -a build/test-raw.log \ - | python3 -u .github/scripts/filter_gradle_log.py + export MAX_ATTEMPTS=${{ inputs.slow_tests && 1 || 2 }} - EXIT_CODE=${PIPESTATUS[0]} + .github/scripts/run_tests_with_retry.sh \ + "musl-${{ matrix.java_version }}-${{ matrix.config }}-aarch64" -- \ + docker run --cpus 4 --rm -v /tmp:/tmp -v "${GITHUB_WORKSPACE}:${GITHUB_WORKSPACE}" -w "${GITHUB_WORKSPACE}" alpine:3.21 /bin/sh -c " + \"$GITHUB_WORKSPACE/.github/scripts/test_alpine_aarch64.sh\" \ + \"${{ github.sha }}\" \"musl/${{ matrix.java_version }}-${{ matrix.config }}-aarch64\" \ + \"${{ matrix.config }}\" \"${{ env.JAVA_HOME }}\" \"${{ env.JAVA_TEST_HOME }}\" \ + \"${{ inputs.slow_tests }}\" + " + + EXIT_CODE=$? if [ $EXIT_CODE -ne 0 ]; then echo "musl-${{ matrix.java_version }}-${{ matrix.config }}-aarch64" >> failures_musl-${{ matrix.java_version }}-${{ matrix.config }}-aarch64.txt @@ -674,6 +703,16 @@ jobs: with: name: (test-reports) test-linux-musl-aarch64 (${{ matrix.java_version }}, ${{ matrix.config }}, ${{ inputs.slow_tests && 'slow' || 'regular' }}) path: test-reports + # Always uploaded, unlike the reports above: a cell that went green only + # because of a retry produces no failure artifact, and that is exactly the + # run whose evidence the summary needs. The file is a few hundred bytes. + - name: Upload CI outcome + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + if: always() + with: + name: (ci-outcome) test-linux-musl-aarch64 (${{ matrix.java_version }}, ${{ matrix.config }}, ${{ inputs.slow_tests && 'slow' || 'regular' }}) + path: ci-outcome + if-no-files-found: ignore - name: Upload signal-safety violation log uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 if: failure() diff --git a/ddprof-test/quarantine.txt b/ddprof-test/quarantine.txt new file mode 100644 index 0000000000..d99ea1014a --- /dev/null +++ b/ddprof-test/quarantine.txt @@ -0,0 +1,35 @@ +# Tests whose failures do not turn CI red. +# +# FORMAT — one entry per line, six fields separated by "|", whitespace around +# each field ignored. Blank lines and lines starting with "#" are ignored. +# +# test | ticket | added | review_by | cells | reason +# +# test Fully qualified .. A trailing ".*" covers every +# method in the class. +# ticket PROF-. Required — a quarantine without a ticket is just +# a test nobody runs. +# added YYYY-MM-DD, the day it went in. +# review_by YYYY-MM-DD. CI FAILS once this date passes, so staying +# quarantined is a decision somebody renews rather than the +# default. 90 days is the usual span. +# cells Comma-separated globs against the cell name +# (---), e.g. "*arm64*" or +# "musl-*,*-asan-*". Leave as "-" to quarantine everywhere; prefer +# narrowing it, so the same test breaking elsewhere still gates. +# reason Free text — what is unreliable and how often. Last field, so it +# may contain anything but "|". +# +# A quarantined test STILL RUNS and still reports; only the gating is +# suspended. That keeps the pass rate visible, which is how you find out the +# test got fixed, or that a "flake" has quietly become permanently broken. +# +# Quarantine is for a test that fails intermittently and that nobody has time +# to fix right now. It is not for a test that is simply wrong — fix or delete +# that one. The intended exit from this file is a fix and a deleted line. +# +# When CI sees a flake it prints a ready-made line in the PR comment. The +# ticket and the judgement are still yours. +# +# Example (delete when the first real entry lands): +# com.datadoghq.profiler.cpu.CpuSamplingTest.testSampling | PROF-12345 | 2026-09-02 | 2026-12-02 | *arm64* | Under-samples on emulated arm64; 2 of 40 runs From 1623cbaa2ad402df4662ee4050469b4a97f8513f Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Thu, 3 Sep 2026 15:11:56 +0200 Subject: [PATCH 2/9] ci: address review of the quarantine machinery 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 --- 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 --- .github/scripts/flake_report.py | 72 +++++++++--- .github/scripts/flake_summary.py | 4 +- .github/scripts/generate-test-summary.sh | 17 ++- .github/scripts/prepare_reports.sh | 5 +- .github/scripts/quarantine.py | 61 +++++++++-- .github/scripts/run_tests_with_retry.sh | 86 +++++++++++++-- .github/scripts/tests/test_quarantine.sh | 133 +++++++++++++++++++++-- .github/workflows/ci.yml | 4 + ddprof-test/quarantine.txt | 4 +- 9 files changed, 335 insertions(+), 51 deletions(-) diff --git a/.github/scripts/flake_report.py b/.github/scripts/flake_report.py index 1f858d21a8..6cd24d3f77 100755 --- a/.github/scripts/flake_report.py +++ b/.github/scripts/flake_report.py @@ -28,9 +28,28 @@ def _attempt_number(path): return int(path.rsplit("-", 1)[1]) -def failed_tests(root_dir): - """Map of "class.test" -> first line of the failure message, for JUnit XML - anywhere under root_dir.""" +def _numbered_attempt_dirs(evidence_dir): + """attempt- directories, numeric suffixes only. + + A stray `attempt-tmp` left by a tool must not raise out of sorted() and + take the whole classification -- and with it the job -- down with it. + """ + found = [] + for path in glob.glob(os.path.join(evidence_dir, "attempt-*")): + suffix = path.rsplit("-", 1)[1] + if suffix.isdigit(): + found.append(path) + return sorted(found, key=_attempt_number) + + +def attempt_results(root_dir): + """(observed test ids, {failed test id: message}) from JUnit XML under root_dir. + + `observed` is every testcase the attempt recorded a result for, pass or + fail. Knowing a test ran and passed is what distinguishes a flake from a + test that simply never got reached on the retry. + """ + observed = set() failures = {} pattern = os.path.join(root_dir, "**", "TEST-*.xml") for path in glob.glob(pattern, recursive=True): @@ -42,21 +61,37 @@ def failed_tests(root_dir): # named test either, so it is left to the exit code to report. continue for case in tree.iter("testcase"): + test_id = "{}.{}".format(case.get("classname") or "", case.get("name") or "") + if case.find("skipped") is None: + observed.add(test_id) problem = case.find("failure") if problem is None: problem = case.find("error") if problem is None: continue - test_id = "{}.{}".format(case.get("classname") or "", case.get("name") or "") message = (problem.get("message") or problem.get("type") or "").strip() failures[test_id] = message.splitlines()[0][:200] if message else "failed" - return failures + return observed, failures + + +def failed_tests(root_dir): + """Just the failures, for callers that do not care what else ran.""" + return attempt_results(root_dir)[1] def collect_attempts(evidence_dir): - """[(attempt number, failures)] ordered by attempt.""" - dirs = glob.glob(os.path.join(evidence_dir, "attempt-*")) - return [(_attempt_number(d), failed_tests(d)) for d in sorted(dirs, key=_attempt_number)] + """[(attempt number, observed, failures)] ordered by attempt. + + An attempt that recorded no testcase at all is dropped: it tells us nothing + about any individual test, and counting it would make every failure from + the other attempts look as though it had passed somewhere. + """ + attempts = [] + for path in _numbered_attempt_dirs(evidence_dir): + observed, failures = attempt_results(path) + if observed: + attempts.append((_attempt_number(path), observed, failures)) + return attempts def cmd_count(args): @@ -70,19 +105,20 @@ def cmd_report(args): entries = quarantine.load(args.list) results = [] - for test_id in sorted({t for _, f in attempts for t in f}): - failed_in = [n for n, f in attempts if test_id in f] - hit = next( - (e for e in entries if quarantine.covers(e, test_id) and quarantine.applies_to(e, args.cell)), - None, - ) + for test_id in sorted({t for _, _, f in attempts for t in f}): + failed_in = [n for n, _, f in attempts if test_id in f] + # Flaky means seen both ways: failed here, ran and passed there. A test + # that is merely missing from the retry never re-ran -- an attempt that + # aborted early, a filtered suite -- and claiming that as a pass would + # hand out quarantine proposals for tests nobody has cleared. + passed_in = [n for n, seen, f in attempts if test_id in seen and test_id not in f] + hit = quarantine.find_entry(entries, test_id, args.cell) results.append({ "test": test_id, "failed_attempts": failed_in, - "message": next(f[test_id] for _, f in attempts if test_id in f), - # Passing on any attempt is what makes it flaky, so a test that - # failed in fewer attempts than were run has passed at least once. - "flaky": len(failed_in) < ran, + "passed_attempts": passed_in, + "message": next(f[test_id] for _, _, f in attempts if test_id in f), + "flaky": bool(passed_in), "quarantined": hit is not None, "ticket": hit.get("ticket") if hit else None, }) diff --git a/.github/scripts/flake_summary.py b/.github/scripts/flake_summary.py index 5ccff49862..287aa18a1b 100755 --- a/.github/scripts/flake_summary.py +++ b/.github/scripts/flake_summary.py @@ -79,11 +79,11 @@ def render_table(grouped, cell_limit=4, row_limit=25, ticket_column=False): def cells_glob(cells): """A glob covering these cells, when they share an obvious axis. - Suggesting `*arm64*` for something that only ever failed on arm64 is more + Suggesting `*aarch64*` for something that only ever failed on aarch64 is more useful than listing four cell names, and narrower than quarantining everywhere -- which would hide the same test breaking on x64 tomorrow. """ - for axis in ("arm64", "aarch64", "musl", "asan", "tsan"): + for axis in ("aarch64", "amd64", "musl", "asan", "tsan"): if all(axis in c for c in cells): return ["*{}*".format(axis)] return None diff --git a/.github/scripts/generate-test-summary.sh b/.github/scripts/generate-test-summary.sh index 454828a75d..e5949bfba3 100755 --- a/.github/scripts/generate-test-summary.sh +++ b/.github/scripts/generate-test-summary.sh @@ -195,11 +195,23 @@ for key in "${failed_jobs[@]}"; do failures="" while IFS= read -r report; do + # Flaky as well as persistent: a job that went red purely because an + # un-quarantined test failed once and passed on the retry is exactly + # the case this machinery creates, and it would otherwise render as + # "no detailed failure information". + if ! rows=$(jq -r '(.persistent + .flaky)[] | [.test, .message] | @tsv' "$report" 2>&1); then + log "WARNING: could not parse outcome report $report: $rows" + failures+="| _unreadable outcome report_ | \`$(basename "$report")\` could not be parsed; see the job log |"$'\n' + continue + fi while IFS=$'\t' read -r test_id message; do [[ -n "$test_id" ]] || continue short_name="${test_id#"${test_id%.*.*}."}" + # A pipe in a failure message would split the row into extra + # columns and break the table, the way flake_summary.py escapes it. + message="${message//|/\\|}" failures+="| \`${short_name}\` | ${message:-Test failed} |"$'\n' - done < <(jq -r '.persistent[] | [.test, .message] | @tsv' "$report" 2>/dev/null || true) + done <<< "$rows" done < <(find "$OUTCOME_DIR" -name "${cell}.json" 2>/dev/null) failure_details["$key"]="$failures" @@ -270,7 +282,8 @@ log "Generating markdown summary..." # Flaky and failing tests, grouped by test rather than by cell. One flaky # test reddens a dozen cells and so does a dozen unrelated breakages; only # grouping by test tells those apart. - python3 "$(dirname "$0")/flake_summary.py" --dir "$OUTCOME_DIR" || true + python3 "$(dirname "$0")/flake_summary.py" --dir "$OUTCOME_DIR" \ + || echo "_Could not render the flaky-test summary; see the job log._" # Failed tests details if ((failed_count > 0)); then diff --git a/.github/scripts/prepare_reports.sh b/.github/scripts/prepare_reports.sh index 3f410de5ca..3ca5674911 100755 --- a/.github/scripts/prepare_reports.sh +++ b/.github/scripts/prepare_reports.sh @@ -12,8 +12,9 @@ cp ddprof-test/javacore*.txt test-reports/ || true cp ddprof-test/build/hs_err* test-reports/ || true cp -r ddprof-lib/build/tmp test-reports/native_build || true cp -r ddprof-test/build/reports/tests test-reports/tests || true -# The JUnit XML, not just the rendered HTML: it is what names the failed tests -# for the PR summary, and what flake_report.py compares between retry attempts. +# The JUnit XML of the final attempt, not just the rendered HTML, for reading +# by hand. Each attempt starts by deleting this directory, so the per-attempt +# evidence flake_report.py compares lives in flake-evidence/ (copied below). cp -r ddprof-test/build/test-results test-reports/test-results || true cp -r flake-evidence test-reports/flake-evidence || true cp build/logs/gdb-watchdog.log test-reports/ || true diff --git a/.github/scripts/quarantine.py b/.github/scripts/quarantine.py index 590094f242..75364b1d39 100755 --- a/.github/scripts/quarantine.py +++ b/.github/scripts/quarantine.py @@ -30,6 +30,16 @@ # neither the release it was added in nor the memory of why. DEFAULT_REVIEW_DAYS = 90 +# Cell names are ---. Only libc and arch are a closed +# set -- jdk and config come from the workflow inputs and grow without warning +# -- so those two are the only axes worth checking a glob against. +KNOWN_ARCHES = ("amd64", "aarch64") +KNOWN_LIBCS = ("glibc", "musl") +# Anything that reads like an architecture. A glob naming one that CI never +# builds silently quarantines nothing, which is how "*arm64*" shipped in this +# file's own example: the arch is spelled aarch64. +ARCH_LIKE_RE = re.compile(r"(?:x86|x64|amd|arm|aarch|i386|ppc|s390)[\w_]*") + def parse(path): """([entry], [(line number, message)]) — entries and malformed lines. @@ -81,6 +91,18 @@ def covers(entry, test_id): return test_id == pattern +def find_entry(entries, test_id, cell): + """The first entry quarantining this test on this cell, or None. + + Every caller that decides whether a failure gates goes through here, so the + matching rule cannot drift between the subcommand and flake_report.py. + """ + return next( + (e for e in entries if covers(e, test_id) and applies_to(e, cell)), + None, + ) + + def format_entry(test, ticket, added, review_by, cells, reason): return " | ".join([test, ticket, added, review_by, ",".join(cells) or "-", reason]) @@ -91,10 +113,7 @@ def cmd_match(args): gating, quarantined = [], [] for test_id in failures: - hit = next( - (e for e in entries if covers(e, test_id) and applies_to(e, args.cell)), - None, - ) + hit = find_entry(entries, test_id, args.cell) (quarantined if hit else gating).append(test_id) json.dump({"gating": gating, "quarantined": quarantined}, sys.stdout) @@ -103,6 +122,14 @@ def cmd_match(args): def cmd_validate(args): + # parse() tolerates a missing file so that matching still works before the + # first entry lands. Validation must not: "0 quarantined test(s), all + # valid" for a list that has been renamed or deleted would report success + # at the exact moment gating silently stopped applying everywhere. + if not os.path.exists(args.list): + print("::error::quarantine list '{}' does not exist".format(args.list)) + return 1 + entries, problems = parse(args.list) today = datetime.date.today() seen = {} @@ -120,9 +147,16 @@ def complain(line, message): if not entry[field]: complain(line, "field '{}' is empty".format(field)) - if name in seen: - complain(line, "'{}' is already quarantined on line {}".format(name, seen[name])) - seen[name] = line + # Two entries for one test are fine when they cover different cells -- + # that is what narrowing by cell is for. Two that cover the same cells + # are a copy-paste, and the second one's ticket and review_by never + # take effect. + key = (name, tuple(sorted(entry["cells"]))) + if key in seen: + where = ", ".join(entry["cells"]) or "every cell" + complain(line, "'{}' is already quarantined for {} on line {}".format( + name, where, seen[key])) + seen[key] = line if entry["ticket"] and not TICKET_RE.match(entry["ticket"]): complain(line, "ticket '{}' is not a PROF-".format(entry["ticket"])) @@ -131,6 +165,19 @@ def complain(line, message): if entry[field] and not DATE_RE.match(entry[field]): complain(line, "{} '{}' is not YYYY-MM-DD".format(field, entry[field])) + for pattern in entry["cells"]: + for token in ARCH_LIKE_RE.findall(pattern): + if token not in KNOWN_ARCHES: + complain(line, ( + "cell glob '{}' names architecture '{}', which CI never " + "builds (cells end in {}); it would quarantine nothing" + ).format(pattern, token, " or ".join(KNOWN_ARCHES))) + head = pattern.split("-", 1)[0] + if head and "*" not in head and "?" not in head and head not in KNOWN_LIBCS: + complain(line, ( + "cell glob '{}' starts with '{}'; cell names start with {}" + ).format(pattern, head, " or ".join(KNOWN_LIBCS))) + if DATE_RE.match(entry["review_by"]): due = datetime.date.fromisoformat(entry["review_by"]) if due < today: diff --git a/.github/scripts/run_tests_with_retry.sh b/.github/scripts/run_tests_with_retry.sh index 9c0048f9b9..bff0c899e5 100755 --- a/.github/scripts/run_tests_with_retry.sh +++ b/.github/scripts/run_tests_with_retry.sh @@ -50,23 +50,60 @@ OUTCOME_FILE="ci-outcome/${CELL}.json" # Snapshot this attempt's JUnit XML before the next one overwrites it -- the # whole point is to compare attempts, and Gradle reuses the same directory. +# The Alpine aarch64 suite runs as root inside Docker while this script runs as +# the host user, so the XML it writes is root-owned. Without this the snapshot +# and the next attempt's cleanup both fail, and the cell loses flake +# classification entirely -- silently, since both used to discard their errors. +make_results_readable() { + [ -d "$RESULTS_DIR" ] || return 0 + [ -w "$RESULTS_DIR" ] && return 0 + command -v sudo >/dev/null 2>&1 || return 0 + sudo chmod -R a+rwX "$RESULTS_DIR" 2>/dev/null \ + || echo "::warning::Could not take ownership of ${RESULTS_DIR}; flake evidence may be incomplete" +} + snapshot() { local attempt="$1" local dest="${EVIDENCE_DIR}/attempt-${attempt}" - rm -rf "$dest" + make_results_readable + rm -rf "$dest" || echo "::warning::Could not clear ${dest}; attempt ${attempt} evidence may be stale" mkdir -p "$dest" if [ -d "$RESULTS_DIR" ]; then - cp -r "$RESULTS_DIR"/. "$dest"/ 2>/dev/null || true + cp -r "$RESULTS_DIR"/. "$dest"/ \ + || echo "::warning::Could not snapshot ${RESULTS_DIR} for attempt ${attempt}; flake classification for this cell will be incomplete" fi } +# Which Gradle tasks are the tests. A failure in anything else is not something +# the quarantine list has any business excusing. +TEST_TASK_PATTERN="${TEST_TASK_PATTERN:-:ddprof-test:test}" + +# g-0's guard: task failures the quarantine list must never wave through. +non_test_task_failures() { + local log="$1" + [ -f "$log" ] || return 0 + grep -oE "Execution failed for task '[^']+'" "$log" 2>/dev/null \ + | sed -E "s/^Execution failed for task '//; s/'$//" \ + | grep -v -F "$TEST_TASK_PATTERN" \ + | sort -u +} + +# Self-contained state: a leftover attempt-2 from an earlier run on a reused +# workspace would be read back as this run's evidence, inflating the attempt +# count and importing failures that never happened here. +rm -rf "$EVIDENCE_DIR" "$(dirname "$OUTCOME_FILE")" + EXIT_CODE=1 +ATTEMPT_LOG="" for attempt in $(seq 1 "$MAX_ATTEMPTS"); do mkdir -p build/logs + make_results_readable rm -rf "$RESULTS_DIR" + ATTEMPT_LOG="build/logs/attempt-${attempt}.log" "$@" 2>&1 \ | tee -a build/test-raw.log \ + | tee "$ATTEMPT_LOG" \ | python3 -u "${HERE}/filter_gradle_log.py" EXIT_CODE=${PIPESTATUS[0]} @@ -80,7 +117,16 @@ for attempt in $(seq 1 "$MAX_ATTEMPTS"); do break fi - failed=$(python3 "${HERE}/flake_report.py" count --dir "${EVIDENCE_DIR}/attempt-${attempt}") + failed=$(python3 "${HERE}/flake_report.py" count --dir "${EVIDENCE_DIR}/attempt-${attempt}") || failed="" + case "$failed" in + ''|*[!0-9]*) + # Every guard below is a numeric comparison; on a non-number they would + # all quietly evaluate false and retry the very failures meant to be + # taken at face value. + echo "::warning::Could not count test failures for attempt ${attempt}; not retrying" + break + ;; + esac if [ "$failed" -eq 0 ] && [ "$RETRY_ON_NO_TEST_FAILURES" != "1" ]; then # No test was named, so the suite did not get far enough to have one fail: # a compile error, a missing toolchain, a runner that ran out of disk. None @@ -134,17 +180,37 @@ fi # error or a dead runner is nothing to do with # quarantine. if [ -f "$OUTCOME_FILE" ]; then - read -r gating failures <<< "$(python3 -c " + summary=$(python3 -c " import json, sys d = json.load(open(sys.argv[1])) print(d['gating_count'], d['failure_count']) -" "$OUTCOME_FILE")" - - if [ "${gating:-0}" -gt 0 ]; then +" "$OUTCOME_FILE") || { + echo "::error::Could not read ${OUTCOME_FILE}; failing the job rather than guessing whether its failures gate" + exit 1 + } + read -r gating failures <<< "$summary" + case "${gating}:${failures}" in + *[!0-9:]*|:*|*:) + echo "::error::${OUTCOME_FILE} did not yield two counts (got '${summary}'); failing the job" + exit 1 + ;; + esac + + if [ "$gating" -gt 0 ]; then EXIT_CODE=1 - elif [ "${failures:-0}" -gt 0 ]; then - echo "::warning::All ${failures} failing test(s) in ${CELL} are quarantined; not failing the job" - EXIT_CODE=0 + elif [ "$failures" -gt 0 ]; then + # Quarantine excuses the tests it names. It does not excuse the build: + # if this same invocation also failed a compile, a native gtest or a + # verification task, that failure has nothing to do with the list and + # zeroing the exit code here would bury it. + other=$(non_test_task_failures "$ATTEMPT_LOG") + if [ -n "$other" ]; then + echo "::error::All ${failures} failing test(s) in ${CELL} are quarantined, but the build also failed in $(echo "$other" | tr '\n' ' ')— failing the job" + EXIT_CODE=1 + else + echo "::warning::All ${failures} failing test(s) in ${CELL} are quarantined; not failing the job" + EXIT_CODE=0 + fi fi fi diff --git a/.github/scripts/tests/test_quarantine.sh b/.github/scripts/tests/test_quarantine.sh index 1921b578a0..e4a0726034 100755 --- a/.github/scripts/tests/test_quarantine.sh +++ b/.github/scripts/tests/test_quarantine.sh @@ -119,11 +119,11 @@ pass "the committed quarantine list is valid" echo "== quarantine.py match ==" -write_list "$LIST" "$(entry a.B.c PROF-1 "$(day_offset 30)" '*arm64*')" +write_list "$LIST" "$(entry a.B.c PROF-1 "$(day_offset 30)" '*aarch64*')" -result=$(printf 'a.B.c\n' | python3 "$SCRIPTS/quarantine.py" --list "$LIST" match --cell "glibc-17-debug-arm64") +result=$(printf 'a.B.c\n' | python3 "$SCRIPTS/quarantine.py" --list "$LIST" match --cell "glibc-17-debug-aarch64") echo "$result" | grep -q '"quarantined": \["a.B.c"\]' \ - || fail "expected a.B.c quarantined on an arm64 cell, got: $result" + || fail "expected a.B.c quarantined on an aarch64 cell, got: $result" pass "a cell glob matches the cells it names" result=$(printf 'a.B.c\n' | python3 "$SCRIPTS/quarantine.py" --list "$LIST" match --cell "glibc-17-debug-amd64") @@ -218,12 +218,129 @@ output=$(cd "$CASE" && "$SCRIPTS/run_tests_with_retry.sh" --list list.txt "glibc rc=$? set -e [ "$rc" -ne 0 ] || fail "a malformed list must not yield a green job (got exit $rc)" -pass "a list that cannot be read fails the job instead of passing silently" -# A malformed line is skipped rather than fatal, so the flake is still caught; -# either way the job must be red. -echo "$output" | grep -q "Flaky test\|Could not classify" \ - || fail "expected the flake or the classifier failure to be reported, got: $output" +pass "a list with a malformed line still fails the job" +# A malformed line is skipped rather than fatal, so here the flake is what +# gates. The classifier-failure path is a separate case below. +echo "$output" | grep -q "Flaky test" \ + || fail "expected the flake to be reported, got: $output" pass "the reason for the red is reported" +# The guard above only bites when flake_report.py itself exits non-zero, which +# a merely malformed line does not do. Point --list at a directory so the +# classifier genuinely fails: the suite passes on its retry, so without the +# REPORT_STATUS guard this job would be green. +CASE="$TEMP_DIR/case-unreadable-list" +make_flaky_suite "$CASE" +mkdir -p "$CASE/list.txt" +set +e +output=$(cd "$CASE" && "$SCRIPTS/run_tests_with_retry.sh" --list list.txt "glibc-17-debug-amd64" -- ./suite.sh 2>&1) +rc=$? +set -e +[ "$rc" -ne 0 ] || fail "a classifier that could not run must not yield a green job (got exit $rc)" +echo "$output" | grep -q "Could not classify results for" \ + || fail "expected the classifier failure to be named, got: $output" +pass "a classifier that cannot run fails the job rather than passing unexamined" + +# Quarantine excuses the tests it names, never the build around them. A suite +# whose only named failure is quarantined but which also failed a non-test +# Gradle task must stay red. +CASE="$TEMP_DIR/case-quarantined-plus-build-failure" +mkdir -p "$CASE" +cat > "$CASE/suite.sh" < Task :ddprof-lib:verifyNative FAILED" +echo "Execution failed for task ':ddprof-lib:verifyNative'." +exit 1 +EOS +chmod +x "$CASE/suite.sh" +write_list "$CASE/list.txt" "$(entry 'com.dd.WobblyTest.*' PROF-1 "$(day_offset 30)")" +set +e +output=$(cd "$CASE" && MAX_ATTEMPTS=1 "$SCRIPTS/run_tests_with_retry.sh" --list list.txt "glibc-17-debug-amd64" -- ./suite.sh 2>&1) +rc=$? +set -e +[ "$rc" -ne 0 ] || fail "a non-test task failure must not be excused by quarantine (got exit $rc)" +echo "$output" | grep -q "verifyNative" \ + || fail "expected the offending task to be named, got: $output" +pass "quarantine excuses the tests it names, not a build failure alongside them" + +# A test missing from the retry never re-ran, so it is not evidence of a flake. +CASE="$TEMP_DIR/case-absent-is-not-passed" +mkdir -p "$CASE/flake-evidence/attempt-1" "$CASE/flake-evidence/attempt-2" +write_failure_xml "$CASE/flake-evidence/attempt-1" "com.dd.GoneTest" "vanishes" "boom" +write_pass_xml "$CASE/flake-evidence/attempt-2" "com.dd.OtherTest" "unrelated" +write_list "$CASE/list.txt" +python3 "$SCRIPTS/flake_report.py" --list "$CASE/list.txt" report \ + --cell "glibc-17-debug-amd64" --evidence-dir "$CASE/flake-evidence" \ + --final-status fail --out "$CASE/out.json" >/dev/null 2>&1 +python3 -c " +import json,sys +d = json.load(open(sys.argv[1])) +assert not d['flaky'], 'a test absent from the retry must not be called flaky: %r' % d['flaky'] +assert len(d['persistent']) == 1, d +" "$CASE/out.json" || fail "absence from a later attempt was treated as a pass" +pass "a test missing from the retry is not mistaken for a flake" + +# A stray attempt-* directory must not abort classification. +CASE="$TEMP_DIR/case-stray-attempt" +mkdir -p "$CASE/flake-evidence/attempt-tmp" +write_failure_xml "$CASE/flake-evidence/attempt-1" "com.dd.WobblyTest" "sometimesFails" "boom" +write_list "$CASE/list.txt" +python3 "$SCRIPTS/flake_report.py" --list "$CASE/list.txt" report \ + --cell "glibc-17-debug-amd64" --evidence-dir "$CASE/flake-evidence" \ + --final-status fail --out "$CASE/out.json" >/dev/null 2>&1 \ + || fail "a non-numeric attempt directory must be ignored, not fatal" +pass "a stray attempt directory is ignored" + +echo "== validate rejects unmatchable cell globs ==" + +write_list "$LIST" "$(entry a.B.c PROF-1 "$(day_offset 30)" '*arm64*')" +if python3 "$SCRIPTS/quarantine.py" --list "$LIST" validate >/dev/null 2>&1; then + fail "a cell glob naming an architecture CI never builds should be rejected" +fi +pass "an unmatchable cell glob is rejected" + +write_list "$LIST" "$(entry a.B.c PROF-1 "$(day_offset 30)" '*aarch64*')" +python3 "$SCRIPTS/quarantine.py" --list "$LIST" validate >/dev/null \ + || fail "a real cell glob should be accepted" +pass "a real cell glob is accepted" + +if python3 "$SCRIPTS/quarantine.py" --list "$TEMP_DIR/does-not-exist.txt" validate >/dev/null 2>&1; then + fail "validating a missing list should fail rather than report success" +fi +pass "a missing list fails validation instead of reporting zero problems" + +# Two entries for one test are legitimate when they cover different cells. +write_list "$LIST" \ + "$(entry a.B.c PROF-1 "$(day_offset 30)" '*aarch64*')" \ + "$(entry a.B.c PROF-2 "$(day_offset 30)" '*amd64*')" +python3 "$SCRIPTS/quarantine.py" --list "$LIST" validate >/dev/null \ + || fail "the same test on disjoint cells should be allowed" +pass "one test may have separate entries for separate cells" + +echo "== flake_summary.py renders ==" + +CASE="$TEMP_DIR/case-summary" +mkdir -p "$CASE/outcomes" +cat > "$CASE/outcomes/glibc-17-debug-aarch64.json" <<'EOS' +{"cell": "glibc-17-debug-aarch64", "attempts": 2, "status": "fail", + "flaky": [{"test": "com.dd.WobblyTest.sometimesFails", "failed_attempts": [1], + "passed_attempts": [2], "message": "got 2 | wanted 50", + "flaky": true, "quarantined": false, "ticket": null}], + "persistent": [], "quarantined": [], "gating_count": 1, "failure_count": 1} +EOS +summary=$(python3 "$SCRIPTS/flake_summary.py" --dir "$CASE/outcomes") \ + || fail "flake_summary.py must render without error" +echo "$summary" | grep -q "sometimesFails" \ + || fail "expected the flaky test in the summary, got: $summary" +echo "$summary" | grep -q "PROF-XXXXX" \ + || fail "expected a paste-ready quarantine proposal, got: $summary" +echo "$summary" | grep -q 'got 2 \\| wanted 50' \ + || fail "expected the pipe in the message to be escaped, got: $summary" +pass "the PR summary renders the flaky table and a proposal" + echo echo "All $TESTS quarantine tests passed." diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5370bee391..e932b7de80 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,6 +60,10 @@ jobs: - name: Validate the test quarantine list run: | bash -n .github/scripts/run_tests_with_retry.sh + bash -n .github/scripts/generate-test-summary.sh + python3 -m py_compile .github/scripts/quarantine.py \ + .github/scripts/flake_report.py \ + .github/scripts/flake_summary.py python3 .github/scripts/quarantine.py validate .github/scripts/tests/test_quarantine.sh diff --git a/ddprof-test/quarantine.txt b/ddprof-test/quarantine.txt index d99ea1014a..fb5f42c362 100644 --- a/ddprof-test/quarantine.txt +++ b/ddprof-test/quarantine.txt @@ -14,7 +14,7 @@ # quarantined is a decision somebody renews rather than the # default. 90 days is the usual span. # cells Comma-separated globs against the cell name -# (---), e.g. "*arm64*" or +# (---), e.g. "*aarch64*" or # "musl-*,*-asan-*". Leave as "-" to quarantine everywhere; prefer # narrowing it, so the same test breaking elsewhere still gates. # reason Free text — what is unreliable and how often. Last field, so it @@ -32,4 +32,4 @@ # ticket and the judgement are still yours. # # Example (delete when the first real entry lands): -# com.datadoghq.profiler.cpu.CpuSamplingTest.testSampling | PROF-12345 | 2026-09-02 | 2026-12-02 | *arm64* | Under-samples on emulated arm64; 2 of 40 runs +# com.datadoghq.profiler.cpu.CpuSamplingTest.testSampling | PROF-12345 | 2026-09-02 | 2026-12-02 | *aarch64* | Under-samples on emulated aarch64; 2 of 40 runs From f6ddf20fee33b9ebb3e23bda55d1f7f326a5475b Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Thu, 3 Sep 2026 17:38:06 +0200 Subject: [PATCH 3/9] ci: scope the quarantine excuse to the attempt that actually ran last 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/.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 --- .github/scripts/flake_report.py | 32 ++++- .github/scripts/flake_summary.py | 62 +++++++-- .github/scripts/generate-test-summary.sh | 18 ++- .github/scripts/quarantine.py | 102 ++++++++------ .github/scripts/run_tests_with_retry.sh | 84 ++++++++---- .../tests/test_generate_test_summary.sh | 127 ++++++++++++++++++ .github/scripts/tests/test_quarantine.sh | 74 +++++++++- .github/workflows/ci.yml | 1 + .github/workflows/test_workflow.yml | 71 +++++++--- .gitignore | 4 + 10 files changed, 466 insertions(+), 109 deletions(-) create mode 100755 .github/scripts/tests/test_generate_test_summary.sh diff --git a/.github/scripts/flake_report.py b/.github/scripts/flake_report.py index 6cd24d3f77..6ad0c506c9 100755 --- a/.github/scripts/flake_report.py +++ b/.github/scripts/flake_report.py @@ -61,7 +61,13 @@ def attempt_results(root_dir): # named test either, so it is left to the exit code to report. continue for case in tree.iter("testcase"): - test_id = "{}.{}".format(case.get("classname") or "", case.get("name") or "") + name = case.get("name") + if not name: + # A testcase element with no name cannot be attributed to any + # real test; "Class." is not a test id worth counting, tabling + # or proposing for quarantine. + continue + test_id = "{}.{}".format(case.get("classname") or "", name) if case.find("skipped") is None: observed.add(test_id) problem = case.find("failure") @@ -116,7 +122,6 @@ def cmd_report(args): results.append({ "test": test_id, "failed_attempts": failed_in, - "passed_attempts": passed_in, "message": next(f[test_id] for _, _, f in attempts if test_id in f), "flaky": bool(passed_in), "quarantined": hit is not None, @@ -125,15 +130,33 @@ def cmd_report(args): gating = [r for r in results if not r["quarantined"]] + # The caller's exit-code decision must never be made from failures + # aggregated across every attempt: those can all be quarantined while the + # final attempt itself failed for a reason that named no test at all (a + # docker or Gradle configuration failure, an OOM-killed daemon, an ASan + # init abort). Report the final attempt's own standing separately so the + # caller can require it to have actually produced test results, with every + # one of its own named failures quarantined, before trusting the list. + final = attempts[-1] if attempts else None + final_attempt_ran = final is not None and final[0] == args.final_attempt + final_attempt_gating_count = None + if final_attempt_ran: + _, _, final_failures = final + final_attempt_gating_count = sum( + 1 for test_id in final_failures + if quarantine.find_entry(entries, test_id, args.cell) is None + ) + report = { "cell": args.cell, "attempts": ran, - "status": args.final_status, "flaky": [r for r in results if r["flaky"] and not r["quarantined"]], "persistent": [r for r in results if not r["flaky"] and not r["quarantined"]], "quarantined": [r for r in results if r["quarantined"]], "gating_count": len(gating), "failure_count": len(results), + "final_attempt_ran": final_attempt_ran, + "final_attempt_gating_count": final_attempt_gating_count, } os.makedirs(os.path.dirname(args.out) or ".", exist_ok=True) @@ -169,7 +192,8 @@ def main(): report = sub.add_parser("report", help="classify failures and decide gating") report.add_argument("--cell", required=True) report.add_argument("--evidence-dir", required=True) - report.add_argument("--final-status", required=True, choices=["pass", "fail"]) + report.add_argument("--final-attempt", required=True, type=int, + help="the attempt number the caller actually ran last") report.add_argument("--out", required=True) report.set_defaults(func=cmd_report) diff --git a/.github/scripts/flake_summary.py b/.github/scripts/flake_summary.py index 287aa18a1b..447f2fc82e 100755 --- a/.github/scripts/flake_summary.py +++ b/.github/scripts/flake_summary.py @@ -16,6 +16,7 @@ import glob import json import os +import re import sys from collections import OrderedDict @@ -24,18 +25,47 @@ DEFAULT_REVIEW_DAYS = quarantine.DEFAULT_REVIEW_DAYS +# Test ids and failure messages come from the PR's own test code, not from +# anything CI controls, and this comment is rendered as markdown and offered +# up as a ready-to-paste quarantine entry. Neither may carry markdown, HTML, or +# a fence-breaking ``` sequence into that render. +_SAFE_TEST_ID_RE = re.compile(r"[^A-Za-z0-9_.$-]") + + +def sanitize_test_id(test_id): + return _SAFE_TEST_ID_RE.sub("_", test_id) + + +def sanitize_inline(text): + """Strip newlines and backticks so text can't break a table row, a code + span, or the ``` fence around the quarantine proposals.""" + return text.replace("`", "'").replace("\n", " ").replace("\r", " ") + def load_reports(root_dir): + """(reports, files found, files skipped). + + Skipped covers anything that looked like a report but wasn't usable: JSON + that failed to parse, or parsed into something that isn't a report at all. + Distinguishing "found nothing" from "found reports, all clean" from "found + reports, some unreadable" is the point -- a total artifact-download failure + must not render the same as a spotless run. + """ reports = [] - for path in sorted(glob.glob(os.path.join(root_dir, "**", "*.json"), recursive=True)): + skipped = 0 + paths = sorted(glob.glob(os.path.join(root_dir, "**", "*.json"), recursive=True)) + for path in paths: try: with open(path) as handle: data = json.load(handle) except (OSError, ValueError): + skipped += 1 continue if isinstance(data, dict) and "cell" in data: reports.append(data) - return reports + else: + skipped += 1 + return reports, len(paths), skipped def group_by_test(reports, key): @@ -67,9 +97,11 @@ def render_table(grouped, cell_limit=4, row_limit=25, ticket_column=False): shown = ", ".join("`{}`".format(c) for c in cells[:cell_limit]) if len(cells) > cell_limit: shown += " _+{} more_".format(len(cells) - cell_limit) - message = (info["message"] or "").replace("|", "\\|")[:120] + message = sanitize_inline((info["message"] or "").replace("|", "\\|"))[:120] + message_cell = "`{}`".format(message) if message else "" ticket = "{} | ".format(info.get("ticket") or "—") if ticket_column else "" - lines.append("| `{}` | {} | {}{} |".format(short_name(test_id), shown, ticket, message)) + lines.append("| `{}` | {} | {}{} |".format( + sanitize_test_id(short_name(test_id)), shown, ticket, message_cell)) if len(grouped) > row_limit: lines.append("") lines.append("_...and {} more. See the job logs._".format(len(grouped) - row_limit)) @@ -112,12 +144,12 @@ def render_proposals(flaky): "# test | ticket | added | review_by | cells | reason", ] for test_id, info in flaky.items(): - reason = "{} (seen in: {})".format( + reason = sanitize_inline("{} (seen in: {})".format( info["message"] or "intermittent failure", ", ".join(sorted(set(info["cells"]))[:4]), - ).replace("|", "/") + )).replace("|", "/") out.append(quarantine.format_entry( - test_id, + sanitize_test_id(test_id), "PROF-XXXXX", today.isoformat(), review_by, @@ -136,8 +168,18 @@ def main(): parser.add_argument("--dir", required=True, help="directory of downloaded ci-outcome artifacts") args = parser.parse_args() - reports = load_reports(args.dir) + reports, files_found, files_skipped = load_reports(args.dir) + if not files_found: + # Distinct from "reports loaded, all clean": this is what a total + # ci-outcome artifact-download failure looks like, and it must not + # render as a silent, spotless run. + sys.stdout.write("_No CI outcome reports were found for this run._\n") + return 0 if not reports: + if files_skipped: + sys.stdout.write( + "_{} CI outcome report(s) were found but could not be parsed._\n" + .format(files_skipped)) return 0 flaky = group_by_test(reports, "flaky") @@ -145,6 +187,10 @@ def main(): quarantined = group_by_test(reports, "quarantined") out = [] + if files_skipped: + out.append("_{} of {} CI outcome report(s) could not be parsed and were skipped._".format( + files_skipped, files_found)) + out.append("") if flaky: out.append("### :warning: Flaky tests — failed, then passed on retry") out.append("") diff --git a/.github/scripts/generate-test-summary.sh b/.github/scripts/generate-test-summary.sh index e5949bfba3..f34d3b6ccb 100755 --- a/.github/scripts/generate-test-summary.sh +++ b/.github/scripts/generate-test-summary.sh @@ -91,18 +91,26 @@ while IFS= read -r job; do started_at=$(echo "$job" | jq -r '.started_at') completed_at=$(echo "$job" | jq -r '.completed_at') - # Only process test jobs (match pattern: test-linux-{libc}-{arch} ({java}, {config})) + # Only process test jobs (match pattern: + # test-linux-{libc}-{arch} ({java}, {config}, {slow|regular})) # Note: regex stored in variable to avoid bash parsing issues with ) character # Note: No ^ anchor because reusable workflow jobs are prefixed with caller job name - # e.g., "test-matrix / test-linux-glibc-amd64 (8, debug)" - test_job_pattern='test-linux-([a-z]+)-([a-z0-9]+) \(([^,]+), ([^)]+)\)$' + # e.g., "test-matrix / test-linux-glibc-amd64 (8, debug, regular)" + # The trailing slow/regular comes from a workflow input, not a matrix axis, + # so it has to be captured here too -- test_workflow.yml is called twice + # with overlapping configs in the same run (nightly, release-validated), + # and without it two different jobs collapse onto the same cell. + test_job_pattern='test-linux-([a-z]+)-([a-z0-9]+) \(([^,]+), ([^,]+), (slow|regular)\)$' if [[ "$name" =~ $test_job_pattern ]]; then libc="${BASH_REMATCH[1]}" arch="${BASH_REMATCH[2]}" java_version="${BASH_REMATCH[3]}" config="${BASH_REMATCH[4]}" + suite="${BASH_REMATCH[5]}" + suite_suffix="" + [[ "$suite" == "slow" ]] && suite_suffix="-slow" - platform="${libc}-${arch}/${config}" + platform="${libc}-${arch}/${config}${suite_suffix}" # Calculate duration if [[ -n "$started_at" && "$started_at" != "null" && -n "$completed_at" && "$completed_at" != "null" ]]; then @@ -119,7 +127,7 @@ while IFS= read -r job; do job_url["$key"]="$html_url" job_duration["$key"]="$duration" # Matches the cell label run_tests_with_retry.sh names its report after. - job_cell["$key"]="${libc}-${java_version}-${config}-${arch}" + job_cell["$key"]="${libc}-${java_version}-${config}-${arch}${suite_suffix}" # Track failed jobs if [[ "$conclusion" == "failure" ]]; then diff --git a/.github/scripts/quarantine.py b/.github/scripts/quarantine.py index 75364b1d39..06006cc9f9 100755 --- a/.github/scripts/quarantine.py +++ b/.github/scripts/quarantine.py @@ -1,11 +1,13 @@ #!/usr/bin/env python3 """The quarantine list: which failing tests do not turn CI red. -Three jobs, one per subcommand: +Two jobs, one per subcommand: match split a cell's failures into gating and quarantined validate enforce the format, the ticket, and the review_by date - propose print an entry ready to paste for a test CI thinks is flaky + +The paste-ready entry a PR comment proposes for a flaky test is rendered by +flake_summary.py's own call to format_entry() below, not by this module's CLI. The list is a plain text table (see ddprof-test/quarantine.txt) rather than JSON or YAML: it is edited by hand far more often than by machine, so real @@ -40,6 +42,38 @@ # file's own example: the arch is spelled aarch64. ARCH_LIKE_RE = re.compile(r"(?:x86|x64|amd|arm|aarch|i386|ppc|s390)[\w_]*") +# A synthetic universe of cell names, used only to ask whether two entries' +# cell globs could both match the same real cell. Wide enough to catch a glob +# written against any axis (jdk, config, or the libc/arch pair) without having +# to enumerate the workflow's actual, ever-growing matrix. +_SYNTHETIC_JDKS = ("8", "8-graal", "11", "17", "17-graal", "21", "25") +_SYNTHETIC_CONFIGS = ("debug", "release", "asan", "tsan") +SYNTHETIC_CELLS = tuple( + "{}-{}-{}-{}".format(libc, jdk, config, arch) + for libc in KNOWN_LIBCS + for jdk in _SYNTHETIC_JDKS + for config in _SYNTHETIC_CONFIGS + for arch in KNOWN_ARCHES +) + + +def cells_overlap(globs_a, globs_b): + """Could some real cell match both sets of globs? No globs means every cell. + + Equal glob lists always overlap without needing the synthetic universe, + which matters when a glob names an axis (like a jdk or config) that + SYNTHETIC_CELLS does not model. + """ + if not globs_a or not globs_b: + return True + if sorted(globs_a) == sorted(globs_b): + return True + return any( + any(fnmatch.fnmatch(cell, g) for g in globs_a) + and any(fnmatch.fnmatch(cell, g) for g in globs_b) + for cell in SYNTHETIC_CELLS + ) + def parse(path): """([entry], [(line number, message)]) — entries and malformed lines. @@ -132,7 +166,7 @@ def cmd_validate(args): entries, problems = parse(args.list) today = datetime.date.today() - seen = {} + seen_by_name = {} def complain(line, message): problems.append((line, message)) @@ -147,16 +181,18 @@ def complain(line, message): if not entry[field]: complain(line, "field '{}' is empty".format(field)) - # Two entries for one test are fine when they cover different cells -- - # that is what narrowing by cell is for. Two that cover the same cells - # are a copy-paste, and the second one's ticket and review_by never - # take effect. - key = (name, tuple(sorted(entry["cells"]))) - if key in seen: - where = ", ".join(entry["cells"]) or "every cell" - complain(line, "'{}' is already quarantined for {} on line {}".format( - name, where, seen[key])) - seen[key] = line + # Two entries for one test are fine when they cover disjoint cells -- + # that is what narrowing by cell is for. Two whose cell globs overlap + # are a copy-paste, and find_entry() only ever returns the first + # match, so the second one's ticket and review_by never take effect + # on the cells the two share. + for prior in seen_by_name.get(name, []): + if cells_overlap(prior["cells"], entry["cells"]): + where = ", ".join(entry["cells"]) or "every cell" + complain(line, "'{}' is already quarantined for {} on line {}".format( + name, where, prior["_line"])) + break + seen_by_name.setdefault(name, []).append(entry) if entry["ticket"] and not TICKET_RE.match(entry["ticket"]): complain(line, "ticket '{}' is not a PROF-".format(entry["ticket"])) @@ -179,14 +215,19 @@ def complain(line, message): ).format(pattern, head, " or ".join(KNOWN_LIBCS))) if DATE_RE.match(entry["review_by"]): - due = datetime.date.fromisoformat(entry["review_by"]) - if due < today: - complain(line, ( - "'{}' has been quarantined since {} and its review was due {} " - "({} days ago). Fix the test and delete this line, or renew " - "review_by with a note on {}." - ).format(name, entry["added"], entry["review_by"], - (today - due).days, entry["ticket"] or "the ticket")) + try: + due = datetime.date.fromisoformat(entry["review_by"]) + except ValueError: + complain(line, "review_by '{}' is not a real calendar date".format( + entry["review_by"])) + else: + if due < today: + complain(line, ( + "'{}' has been quarantined since {} and its review was due {} " + "({} days ago). Fix the test and delete this line, or renew " + "review_by with a note on {}." + ).format(name, entry["added"], entry["review_by"], + (today - due).days, entry["ticket"] or "the ticket")) for line, message in sorted(problems): print("::error file={},line={}::{}".format(args.list, line, message)) @@ -199,19 +240,6 @@ def complain(line, message): return 0 -def cmd_propose(args): - today = datetime.date.today() - print(format_entry( - args.test, - "PROF-XXXXX", - today.isoformat(), - (today + datetime.timedelta(days=DEFAULT_REVIEW_DAYS)).isoformat(), - args.cells or [], - args.reason, - )) - return 0 - - def main(): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--list", default=DEFAULT_LIST) @@ -224,12 +252,6 @@ def main(): validate = sub.add_parser("validate", help="check the list's format and review dates") validate.set_defaults(func=cmd_validate) - propose = sub.add_parser("propose", help="print a paste-ready entry") - propose.add_argument("--test", required=True) - propose.add_argument("--reason", required=True) - propose.add_argument("--cells", nargs="*") - propose.set_defaults(func=cmd_propose) - args = parser.parse_args() return args.func(args) diff --git a/.github/scripts/run_tests_with_retry.sh b/.github/scripts/run_tests_with_retry.sh index bff0c899e5..4f215b423b 100755 --- a/.github/scripts/run_tests_with_retry.sh +++ b/.github/scripts/run_tests_with_retry.sh @@ -56,9 +56,19 @@ OUTCOME_FILE="ci-outcome/${CELL}.json" # classification entirely -- silently, since both used to discard their errors. make_results_readable() { [ -d "$RESULTS_DIR" ] || return 0 - [ -w "$RESULTS_DIR" ] && return 0 + # The permission problem this exists to fix is per-file (Docker writes the + # XML as root while the directory it lands in stays host-owned), so a + # directory-level writability check would miss it. Probe by ownership rather + # than with find's -writable, which busybox does not implement -- there the + # test would fail open into a silent no-op, which is the failure this whole + # function exists to stop. A probe that cannot decide takes ownership anyway. + if foreign=$(find "$RESULTS_DIR" ! -user "$(id -u)" -print 2>/dev/null | head -n 1); then + [ -n "$foreign" ] || { [ -w "$RESULTS_DIR" ] && [ -w "$(dirname "$RESULTS_DIR")" ] && return 0; } + fi command -v sudo >/dev/null 2>&1 || return 0 - sudo chmod -R a+rwX "$RESULTS_DIR" 2>/dev/null \ + # Include the parent so the pre-attempt `rm -rf "$RESULTS_DIR"` below (which + # needs to unlink the directory itself, not just its contents) can succeed. + sudo chmod -R a+rwX "$(dirname "$RESULTS_DIR")" 2>/dev/null \ || echo "::warning::Could not take ownership of ${RESULTS_DIR}; flake evidence may be incomplete" } @@ -78,7 +88,7 @@ snapshot() { # the quarantine list has any business excusing. TEST_TASK_PATTERN="${TEST_TASK_PATTERN:-:ddprof-test:test}" -# g-0's guard: task failures the quarantine list must never wave through. +# Task failures the quarantine list must never wave through. non_test_task_failures() { local log="$1" [ -f "$log" ] || return 0 @@ -98,7 +108,8 @@ ATTEMPT_LOG="" for attempt in $(seq 1 "$MAX_ATTEMPTS"); do mkdir -p build/logs make_results_readable - rm -rf "$RESULTS_DIR" + rm -rf "$RESULTS_DIR" \ + || echo "::warning::Could not clear ${RESULTS_DIR} before attempt ${attempt}; it may inherit the previous attempt's results and a flake will look persistent" ATTEMPT_LOG="build/logs/attempt-${attempt}.log" "$@" 2>&1 \ @@ -147,16 +158,10 @@ for attempt in $(seq 1 "$MAX_ATTEMPTS"); do ./gradlew --stop 2>/dev/null || true done -if [ "$EXIT_CODE" -eq 0 ]; then - FINAL_STATUS=pass -else - FINAL_STATUS=fail -fi - python3 "${HERE}/flake_report.py" --list "$QUARANTINE_LIST" report \ --cell "$CELL" \ --evidence-dir "$EVIDENCE_DIR" \ - --final-status "$FINAL_STATUS" \ + --final-attempt "$attempt" \ --out "$OUTCOME_FILE" REPORT_STATUS=$? @@ -174,8 +179,14 @@ fi # nobody has quarantined is still a failure; # letting the retry excuse it is how flakes get # tolerated for years. -# every failure quarantined -> green. That is what the list is for, and the -# entry behind it carries a ticket and a date. +# every failure quarantined -> green, but only when the *final* attempt is +# the one vouching for that: failures +# aggregated across every attempt can all be +# quarantined while the final attempt itself +# failed for a reason that named no test at +# all (a docker or Gradle failure, an +# OOM-killed daemon, an ASan init abort), and +# the list has no business excusing that. # no test named -> keep the command's own exit code: a compile # error or a dead runner is nothing to do with # quarantine. @@ -183,33 +194,52 @@ if [ -f "$OUTCOME_FILE" ]; then summary=$(python3 -c " import json, sys d = json.load(open(sys.argv[1])) -print(d['gating_count'], d['failure_count']) +final_gating = d['final_attempt_gating_count'] +print(d['gating_count'], d['failure_count'], int(d['final_attempt_ran']), + final_gating if final_gating is not None else -1) " "$OUTCOME_FILE") || { echo "::error::Could not read ${OUTCOME_FILE}; failing the job rather than guessing whether its failures gate" exit 1 } - read -r gating failures <<< "$summary" - case "${gating}:${failures}" in - *[!0-9:]*|:*|*:) - echo "::error::${OUTCOME_FILE} did not yield two counts (got '${summary}'); failing the job" + read -r gating failures final_ran final_gating <<< "$summary" + case "${gating}:${failures}:${final_ran}" in + *[!0-9:]*|:*|*:|*::*) + echo "::error::${OUTCOME_FILE} did not yield usable counts (got '${summary}'); failing the job" exit 1 ;; esac + case "$final_gating" in + -1|*[!0-9]*) + [ "$final_gating" = "-1" ] || { + echo "::error::${OUTCOME_FILE} did not yield a usable final-attempt gating count (got '${summary}'); failing the job" + exit 1 + } + ;; + esac if [ "$gating" -gt 0 ]; then EXIT_CODE=1 elif [ "$failures" -gt 0 ]; then - # Quarantine excuses the tests it names. It does not excuse the build: - # if this same invocation also failed a compile, a native gtest or a - # verification task, that failure has nothing to do with the list and - # zeroing the exit code here would bury it. - other=$(non_test_task_failures "$ATTEMPT_LOG") - if [ -n "$other" ]; then - echo "::error::All ${failures} failing test(s) in ${CELL} are quarantined, but the build also failed in $(echo "$other" | tr '\n' ' ')— failing the job" + if [ "$final_ran" -ne 1 ] || [ "$final_gating" -ne 0 ]; then + # The final attempt either produced no test results of its own (a + # build or infrastructure failure, not something quarantine speaks to) + # or still has its own named failures unquarantined -- either way the + # list has nothing to say about why this attempt is red. + echo "::error::${CELL}'s final attempt did not itself pass with only quarantined failures (ran=${final_ran}, its own gating count=${final_gating}); failing the job rather than trusting failures from an earlier attempt" EXIT_CODE=1 else - echo "::warning::All ${failures} failing test(s) in ${CELL} are quarantined; not failing the job" - EXIT_CODE=0 + # Quarantine excuses the tests it names. It does not excuse the build: + # if this same invocation also failed a compile, a native gtest or a + # verification task, that failure has nothing to do with the list and + # zeroing the exit code here would bury it. + other=$(non_test_task_failures "$ATTEMPT_LOG") + if [ -n "$other" ]; then + echo "::error::All ${failures} failing test(s) in ${CELL} are quarantined, but the build also failed in $(echo "$other" | tr '\n' ' ')— failing the job" + EXIT_CODE=1 + else + echo "::warning::All ${failures} failing test(s) in ${CELL} are quarantined; not failing the job" + EXIT_CODE=0 + fi fi fi fi diff --git a/.github/scripts/tests/test_generate_test_summary.sh b/.github/scripts/tests/test_generate_test_summary.sh new file mode 100755 index 0000000000..5ba0677864 --- /dev/null +++ b/.github/scripts/tests/test_generate_test_summary.sh @@ -0,0 +1,127 @@ +#!/usr/bin/env bash + +set -euo pipefail + +# Copyright 2026, Datadog, Inc + +# Hermetic tests for generate-test-summary.sh's handling of downloaded +# ci-outcome reports. +# Run with: .github/scripts/tests/test_generate_test_summary.sh +# +# `gh` is the only external dependency this script has that can't run inside a +# sandbox, so it is the only thing stubbed out below; everything else (jq, +# the report-parsing logic) runs for real against fixture data. + +ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")/../../.." && pwd) +SCRIPT="$ROOT/.github/scripts/generate-test-summary.sh" +TEMP_DIR=$(mktemp -d) +TESTS=0 + +cleanup() { + rm -rf "$TEMP_DIR" +} +trap cleanup EXIT + +fail() { + echo "FAIL: $*" >&2 + exit 1 +} + +pass() { + TESTS=$((TESTS + 1)) + echo " ok: $*" +} + +# A stub `gh` good enough for this script's two call sites: the jobs listing +# and the ci-outcome artifact download. Real GitHub is never reached. +STUB_BIN="$TEMP_DIR/stub-bin" +mkdir -p "$STUB_BIN" +cat > "$STUB_BIN/gh" <<'EOS' +#!/usr/bin/env bash +if [ "$1" = "api" ]; then + cat "$GH_JOBS_FIXTURE" + exit 0 +fi +if [ "$1" = "run" ] && [ "$2" = "download" ]; then + dir="" + prev="" + for arg in "$@"; do + if [ "$prev" = "--dir" ]; then dir="$arg"; fi + prev="$arg" + done + mkdir -p "$dir" + cp "$GH_OUTCOME_FIXTURE_DIR"/*.json "$dir/" 2>/dev/null || true + exit 0 +fi +echo "stub gh: unexpected invocation: $*" >&2 +exit 1 +EOS +chmod +x "$STUB_BIN/gh" +PATH="$STUB_BIN:$PATH" +export PATH + +write_jobs_fixture() { + # write_jobs_fixture + cat > "$1" < "$CASE/outcomes/glibc-17-debug-amd64.json" <<'EOJ' +{"cell": "glibc-17-debug-amd64", "attempts": 1, "persistent": + [{"test": "com.dd.FooTest.bar", "message": "assertion failed: boom"}], + "flaky": [], "quarantined": [], "gating_count": 1, "failure_count": 1, + "final_attempt_ran": true, "final_attempt_gating_count": 1} +EOJ +( + cd "$CASE/work" + export GH_JOBS_FIXTURE="$CASE/jobs/jobs.json" GH_OUTCOME_FIXTURE_DIR="$CASE/outcomes" + export GITHUB_REPOSITORY="DataDog/java-profiler" GITHUB_SHA="deadbeefcafef00dfeedfacebeefcafebeefcafe" + "$SCRIPT" 12345 "$CASE/work/summary.md" +) || fail "generate-test-summary.sh exited non-zero on a valid outcome report" +summary=$(cat "$CASE/work/summary.md") +echo "$summary" | grep -q "FooTest.bar" \ + || fail "expected the real failing test in the summary, got: $summary" +echo "$summary" | grep -q "assertion failed: boom" \ + || fail "expected the real failure message in the summary, got: $summary" +if echo "$summary" | grep -q "_unreadable outcome report_"; then + fail "a valid outcome report was rendered as unreadable, got: $summary" +fi +pass "a valid outcome report renders its real failure, not the unreadable fallback" + +# A malformed outcome report (invalid JSON) must be rendered as unreadable, +# not silently dropped or fed further down the pipeline as if it were rows. +CASE="$TEMP_DIR/case-malformed-report" +mkdir -p "$CASE/jobs" "$CASE/outcomes" "$CASE/work" +write_jobs_fixture "$CASE/jobs/jobs.json" failure +printf 'this is not json\n' > "$CASE/outcomes/glibc-17-debug-amd64.json" +( + cd "$CASE/work" + export GH_JOBS_FIXTURE="$CASE/jobs/jobs.json" GH_OUTCOME_FIXTURE_DIR="$CASE/outcomes" + export GITHUB_REPOSITORY="DataDog/java-profiler" GITHUB_SHA="deadbeefcafef00dfeedfacebeefcafebeefcafe" + "$SCRIPT" 12345 "$CASE/work/summary.md" +) || fail "generate-test-summary.sh exited non-zero on a malformed outcome report" +summary=$(cat "$CASE/work/summary.md") +echo "$summary" | grep -q "_unreadable outcome report_" \ + || fail "expected a malformed outcome report to be flagged unreadable, got: $summary" +pass "a malformed outcome report is flagged unreadable rather than silently ignored" + +echo +echo "All $TESTS generate-test-summary tests passed." diff --git a/.github/scripts/tests/test_quarantine.sh b/.github/scripts/tests/test_quarantine.sh index e4a0726034..bb7610a9f4 100755 --- a/.github/scripts/tests/test_quarantine.sh +++ b/.github/scripts/tests/test_quarantine.sh @@ -160,6 +160,35 @@ EOS chmod +x "$dir/suite.sh" } +# A clean run on the first attempt must exit 0 and report nothing gating. +# Every other case in this section starts from a failure; without this one, a +# regression that made a clean run report a gating failure would leave every +# other assertion here passing. +CASE="$TEMP_DIR/case-green" +mkdir -p "$CASE" +cat > "$CASE/suite.sh" </dev/null 2>&1) +rc=$? +set -e +[ "$rc" -eq 0 ] || fail "a suite that passes on the first attempt must exit 0 (got exit $rc)" +python3 -c " +import json,sys +d = json.load(open(sys.argv[1])) +assert d['gating_count'] == 0 and d['failure_count'] == 0, d +assert not d['flaky'] and not d['persistent'], d +" "$CASE/ci-outcome/glibc-17-debug-amd64.json" || fail "a clean run was not reported as clean" +pass "a suite that passes on the first attempt is green and reports no failures" + # Not quarantined: passing on the retry must not rescue the job. CASE="$TEMP_DIR/case-gating" make_flaky_suite "$CASE" @@ -267,6 +296,37 @@ echo "$output" | grep -q "verifyNative" \ || fail "expected the offending task to be named, got: $output" pass "quarantine excuses the tests it names, not a build failure alongside them" +# Regression: a failure aggregated from an EARLIER attempt must never rescue a +# FINAL attempt that failed for a reason naming no test at all (here: nothing +# that prints "Execution failed for task", so the non_test_task_failures grep +# alone would miss it). Attempt 1 fails a named, quarantined test; attempt 2 +# aborts before writing any JUnit XML, the way a docker or JVM-init failure +# would. The job must stay red even though every named failure is quarantined. +CASE="$TEMP_DIR/case-final-attempt-no-tests" +mkdir -p "$CASE" +cat > "$CASE/suite.sh" </dev/null || echo 0) + 1 )); echo \$n > .n +OUT=ddprof-test/build/test-results/testDebug +mkdir -p "\$OUT" +if [ "\$n" -eq 1 ]; then +$(declare -f write_failure_xml) + write_failure_xml "\$OUT" "com.dd.WobblyTest" "sometimesFails" "got 2 samples, wanted 50" + exit 1 +fi +rm -rf "\$OUT" +echo "docker: Error response from daemon: OCI runtime create failed" +exit 1 +EOS +chmod +x "$CASE/suite.sh" +write_list "$CASE/list.txt" "$(entry com.dd.WobblyTest.sometimesFails PROF-1 "$(day_offset 30)")" +set +e +output=$(cd "$CASE" && "$SCRIPTS/run_tests_with_retry.sh" --list list.txt "glibc-17-debug-amd64" -- ./suite.sh 2>&1) +rc=$? +set -e +[ "$rc" -ne 0 ] || fail "a final attempt that named no test must not be excused by an earlier attempt's quarantined failure (got exit $rc)" +pass "a final attempt naming no test is never excused by an earlier attempt's quarantine hit" + # A test missing from the retry never re-ran, so it is not evidence of a flake. CASE="$TEMP_DIR/case-absent-is-not-passed" mkdir -p "$CASE/flake-evidence/attempt-1" "$CASE/flake-evidence/attempt-2" @@ -275,7 +335,7 @@ write_pass_xml "$CASE/flake-evidence/attempt-2" "com.dd.OtherTest" "unrelated" write_list "$CASE/list.txt" python3 "$SCRIPTS/flake_report.py" --list "$CASE/list.txt" report \ --cell "glibc-17-debug-amd64" --evidence-dir "$CASE/flake-evidence" \ - --final-status fail --out "$CASE/out.json" >/dev/null 2>&1 + --final-attempt 2 --out "$CASE/out.json" >/dev/null 2>&1 python3 -c " import json,sys d = json.load(open(sys.argv[1])) @@ -284,16 +344,24 @@ assert len(d['persistent']) == 1, d " "$CASE/out.json" || fail "absence from a later attempt was treated as a pass" pass "a test missing from the retry is not mistaken for a flake" -# A stray attempt-* directory must not abort classification. +# A stray attempt-* directory must not abort classification, and attempt-1 +# must still be read as the (only, and so final) real attempt. CASE="$TEMP_DIR/case-stray-attempt" mkdir -p "$CASE/flake-evidence/attempt-tmp" write_failure_xml "$CASE/flake-evidence/attempt-1" "com.dd.WobblyTest" "sometimesFails" "boom" write_list "$CASE/list.txt" python3 "$SCRIPTS/flake_report.py" --list "$CASE/list.txt" report \ --cell "glibc-17-debug-amd64" --evidence-dir "$CASE/flake-evidence" \ - --final-status fail --out "$CASE/out.json" >/dev/null 2>&1 \ + --final-attempt 1 --out "$CASE/out.json" >/dev/null 2>&1 \ || fail "a non-numeric attempt directory must be ignored, not fatal" pass "a stray attempt directory is ignored" +python3 -c " +import json,sys +d = json.load(open(sys.argv[1])) +assert d['attempts'] == 1, d +assert d['persistent'] and d['persistent'][0]['test'] == 'com.dd.WobblyTest.sometimesFails', d +" "$CASE/out.json" || fail "attempt-1 was not read back despite the stray attempt-tmp" +pass "attempt-1 is still read as evidence while the stray directory is skipped" echo "== validate rejects unmatchable cell globs ==" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e932b7de80..69990a911e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -66,6 +66,7 @@ jobs: .github/scripts/flake_summary.py python3 .github/scripts/quarantine.py validate .github/scripts/tests/test_quarantine.sh + .github/scripts/tests/test_generate_test_summary.sh check-for-pr: runs-on: ubuntu-latest diff --git a/.github/workflows/test_workflow.yml b/.github/workflows/test_workflow.yml index a370e3a59b..3f5f055066 100644 --- a/.github/workflows/test_workflow.yml +++ b/.github/workflows/test_workflow.yml @@ -43,6 +43,11 @@ jobs: echo "configs=$configs" >> $GITHUB_OUTPUT test-linux-glibc-amd64: needs: cache-jdks + # The default job name has no room for slow_tests -- it is a workflow + # input, not a matrix axis -- so two calls to this workflow in the same + # run (one regular, one slow) would otherwise show identical job names + # and be indistinguishable to generate-test-summary.sh. + name: test-linux-glibc-amd64 (${{ matrix.java_version }}, ${{ matrix.config }}, ${{ inputs.slow_tests && 'slow' || 'regular' }}) strategy: fail-fast: false matrix: @@ -155,22 +160,30 @@ jobs: exit 0 fi - # The slow/e2e suite already runs the best part of an hour, so a retry - # would risk the 180-minute job timeout. It records failures without - # re-running them. - export MAX_ATTEMPTS=${{ inputs.slow_tests && 1 || 2 }} - - # ASan init can nondeterministically collide with the JVM's - # ASLR-influenced mmap layout (google/sanitizers#856). The JVM aborts - # before any test runs, so that failure names no test and would - # otherwise be classed as a build error and left unretried. + # Default: a failure naming no test is a build error, not worth a + # retry. export RETRY_ON_NO_TEST_FAILURES=0 if [[ "${{ matrix.config }}" == "asan" ]]; then + # ASan init can nondeterministically collide with the JVM's + # ASLR-influenced mmap layout (google/sanitizers#856). The JVM + # aborts before any test runs, so that failure names no test and + # would otherwise be classed as a build error and left unretried. export RETRY_ON_NO_TEST_FAILURES=1 fi + # The slow/e2e suite already runs the best part of an hour, so a + # retry would risk the 180-minute job timeout, and it records + # failures without re-running them. That rationale does not hold + # under ASan: the retry above fires on an init abort that costs + # seconds, not a full slow run, so ASan keeps its second attempt + # even when slow. + export MAX_ATTEMPTS=2 + if [[ "${{ inputs.slow_tests }}" == "true" && "${{ matrix.config }}" != "asan" ]]; then + export MAX_ATTEMPTS=1 + fi + .github/scripts/run_tests_with_retry.sh \ - "glibc-${{ matrix.java_version }}-${{ matrix.config }}-amd64" -- \ + "glibc-${{ matrix.java_version }}-${{ matrix.config }}-amd64${{ inputs.slow_tests && '-slow' || '' }}" -- \ ${GRADLEW_PREFIX} ./gradlew -PCI -PkeepJFRs ${{ inputs.skip_gtest == true && '-Pskip-gtest' || '' }} :ddprof-test:test${{ inputs.slow_tests && 'Slow' || '' }}${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs EXIT_CODE=$? @@ -248,6 +261,8 @@ jobs: test-linux-musl-amd64: needs: [cache-jdks, filter-musl-configs] if: needs.filter-musl-configs.outputs.has_configs == 'true' + # See the comment on test-linux-glibc-amd64's name above. + name: test-linux-musl-amd64 (${{ matrix.java_version }}, ${{ matrix.config }}, ${{ inputs.slow_tests && 'slow' || 'regular' }}) strategy: fail-fast: false matrix: @@ -321,7 +336,7 @@ jobs: export MAX_ATTEMPTS=${{ inputs.slow_tests && 1 || 2 }} .github/scripts/run_tests_with_retry.sh \ - "musl-${{ matrix.java_version }}-${{ matrix.config }}-amd64" -- \ + "musl-${{ matrix.java_version }}-${{ matrix.config }}-amd64${{ inputs.slow_tests && '-slow' || '' }}" -- \ ./gradlew -PCI -PkeepJFRs :ddprof-test:test${{ inputs.slow_tests && 'Slow' || '' }}${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs EXIT_CODE=$? @@ -393,6 +408,8 @@ jobs: test-linux-glibc-aarch64: needs: cache-jdks + # See the comment on test-linux-glibc-amd64's name above. + name: test-linux-glibc-aarch64 (${{ matrix.java_version }}, ${{ matrix.config }}, ${{ inputs.slow_tests && 'slow' || 'regular' }}) strategy: fail-fast: false matrix: @@ -502,22 +519,30 @@ jobs: exit 0 fi - # The slow/e2e suite already runs the best part of an hour, so a retry - # would risk the 180-minute job timeout. It records failures without - # re-running them. - export MAX_ATTEMPTS=${{ inputs.slow_tests && 1 || 2 }} - - # ASan init can nondeterministically collide with the JVM's - # ASLR-influenced mmap layout (google/sanitizers#856). The JVM aborts - # before any test runs, so that failure names no test and would - # otherwise be classed as a build error and left unretried. + # Default: a failure naming no test is a build error, not worth a + # retry. export RETRY_ON_NO_TEST_FAILURES=0 if [[ "${{ matrix.config }}" == "asan" ]]; then + # ASan init can nondeterministically collide with the JVM's + # ASLR-influenced mmap layout (google/sanitizers#856). The JVM + # aborts before any test runs, so that failure names no test and + # would otherwise be classed as a build error and left unretried. export RETRY_ON_NO_TEST_FAILURES=1 fi + # The slow/e2e suite already runs the best part of an hour, so a + # retry would risk the 180-minute job timeout, and it records + # failures without re-running them. That rationale does not hold + # under ASan: the retry above fires on an init abort that costs + # seconds, not a full slow run, so ASan keeps its second attempt + # even when slow. + export MAX_ATTEMPTS=2 + if [[ "${{ inputs.slow_tests }}" == "true" && "${{ matrix.config }}" != "asan" ]]; then + export MAX_ATTEMPTS=1 + fi + .github/scripts/run_tests_with_retry.sh \ - "glibc-${{ matrix.java_version }}-${{ matrix.config }}-aarch64" -- \ + "glibc-${{ matrix.java_version }}-${{ matrix.config }}-aarch64${{ inputs.slow_tests && '-slow' || '' }}" -- \ ${GRADLEW_PREFIX} ./gradlew -PCI -PkeepJFRs ${{ inputs.skip_gtest == true && '-Pskip-gtest' || '' }} :ddprof-test:test${{ inputs.slow_tests && 'Slow' || '' }}${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs EXIT_CODE=$? @@ -595,6 +620,8 @@ jobs: test-linux-musl-aarch64: needs: [cache-jdks, filter-musl-configs] if: needs.filter-musl-configs.outputs.has_configs == 'true' + # See the comment on test-linux-glibc-amd64's name above. + name: test-linux-musl-aarch64 (${{ matrix.java_version }}, ${{ matrix.config }}, ${{ inputs.slow_tests && 'slow' || 'regular' }}) strategy: fail-fast: false matrix: @@ -638,7 +665,7 @@ jobs: export MAX_ATTEMPTS=${{ inputs.slow_tests && 1 || 2 }} .github/scripts/run_tests_with_retry.sh \ - "musl-${{ matrix.java_version }}-${{ matrix.config }}-aarch64" -- \ + "musl-${{ matrix.java_version }}-${{ matrix.config }}-aarch64${{ inputs.slow_tests && '-slow' || '' }}" -- \ docker run --cpus 4 --rm -v /tmp:/tmp -v "${GITHUB_WORKSPACE}:${GITHUB_WORKSPACE}" -w "${GITHUB_WORKSPACE}" alpine:3.21 /bin/sh -c " \"$GITHUB_WORKSPACE/.github/scripts/test_alpine_aarch64.sh\" \ \"${{ github.sha }}\" \"musl/${{ matrix.java_version }}-${{ matrix.config }}-aarch64\" \ diff --git a/.gitignore b/.gitignore index 1c9dd43f2d..ee257fca7c 100644 --- a/.gitignore +++ b/.gitignore @@ -41,6 +41,10 @@ datadog/maven/resources # Temporary documentation and work state doc/temp/ +# Working state left by run_tests_with_retry.sh +/flake-evidence/ +/ci-outcome/ + # CLAUDE.md is auto-generated from AGENTS.md bootstrap instructions CLAUDE.md From 3786d2e64bd5d8e302899702947b892d3baf57fb Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Fri, 4 Sep 2026 14:30:50 +0200 Subject: [PATCH 4/9] ci: never read a cut-short attempt as a quarantined pass 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 --- .github/scripts/flake_report.py | 126 +++++++++++- .github/scripts/flake_summary.py | 87 ++++++-- .github/scripts/generate-test-summary.sh | 67 +----- .github/scripts/prepare_reports.sh | 15 +- .github/scripts/quarantine.py | 123 +++++++---- .github/scripts/run_tests_with_retry.sh | 194 ++++++++++-------- .../tests/test_generate_test_summary.sh | 45 +++- .github/scripts/tests/test_quarantine.sh | 178 ++++++++++++++-- .github/workflows/test_workflow.yml | 36 ++-- ddprof-test/quarantine.txt | 9 +- 10 files changed, 631 insertions(+), 249 deletions(-) diff --git a/.github/scripts/flake_report.py b/.github/scripts/flake_report.py index 6ad0c506c9..efaccbc7af 100755 --- a/.github/scripts/flake_report.py +++ b/.github/scripts/flake_report.py @@ -17,6 +17,7 @@ import glob import json import os +import re import sys import xml.etree.ElementTree as ET @@ -62,12 +63,14 @@ def attempt_results(root_dir): continue for case in tree.iter("testcase"): name = case.get("name") - if not name: - # A testcase element with no name cannot be attributed to any - # real test; "Class." is not a test id worth counting, tabling - # or proposing for quarantine. + classname = case.get("classname") + if not name or not classname: + # A testcase element missing either half of its id cannot be + # attributed to any real test; "Class." and ".method" are + # equally unusable as a test id worth counting, tabling, or + # proposing for quarantine. continue - test_id = "{}.{}".format(case.get("classname") or "", name) + test_id = "{}.{}".format(classname, name) if case.find("skipped") is None: observed.add(test_id) problem = case.find("failure") @@ -105,6 +108,27 @@ def cmd_count(args): return 0 +_NON_TEST_TASK_FAILURE_RE = re.compile(r"Execution failed for task '([^']+)'") + + +def non_test_task_failures(log_path, test_task_pattern): + """Gradle task names blamed for a failure, other than the test task itself. + + Quarantine excuses the tests it names; it does not excuse the build. If + this invocation's log also blames a compile, a native gtest, or a + verification task, that failure has nothing to do with the list. + """ + if not log_path or not os.path.isfile(log_path): + return [] + found = set() + with open(log_path, errors="replace") as handle: + for line in handle: + m = _NON_TEST_TASK_FAILURE_RE.search(line) + if m and test_task_pattern not in m.group(1): + found.add(m.group(1)) + return sorted(found) + + def cmd_report(args): attempts = collect_attempts(args.evidence_dir) ran = len(attempts) @@ -140,16 +164,91 @@ def cmd_report(args): final = attempts[-1] if attempts else None final_attempt_ran = final is not None and final[0] == args.final_attempt final_attempt_gating_count = None + final_attempt_failure_count = None if final_attempt_ran: _, _, final_failures = final + final_attempt_failure_count = len(final_failures) final_attempt_gating_count = sum( 1 for test_id in final_failures if quarantine.find_entry(entries, test_id, args.cell) is None ) + other_task_failures = non_test_task_failures(args.attempt_log, args.test_task_pattern) + + # The gating verdict, owned here rather than re-derived by the caller from + # raw counts: three independent readers of this file re-deciding the same + # thing is how a schema change turns into shotgun surgery. + # + # any un-quarantined failure -> gate, even if a retry passed. A flake + # nobody has quarantined is still a + # failure. + # every failure quarantined -> excuse, but only when the *final* + # attempt itself ran and recorded results + # (final_attempt_ran), and every failure + # it did name is quarantined. A final + # attempt that crashed before recording a + # single testcase drops out of `attempts` + # entirely, so final_attempt_ran is False + # and it is never waved through just + # because an *earlier* attempt's failures + # all happen to be quarantined. A final + # attempt that ran and simply passed + # outright (zero failures of its own) is + # the ordinary flaky-then-fixed case and + # must not gate. + # final attempt exited -> gate. It ran, recorded results, and + # non-zero having named named no failure of its own, yet the + # no failure of its own command still failed: the JVM aborted + # part-way through, so the tests it never + # reached are absent from the XML rather + # than passing. Gradle blames the crash on + # the test task itself, so the non-test + # task check above cannot see it. + # no failure named -> no opinion; the caller keeps its own + # exit code (a compile error or a dead + # runner is nothing to do with + # quarantine). + if args.evidence_suspect: + gates = True + gate_reason = ( + "flake evidence for this cell is suspect (the results directory " + "could not be reliably cleared between attempts), so a prior " + "attempt's results may be mistaken for the final attempt's own" + ) + elif gating: + gates = True + gate_reason = "{} un-quarantined failure(s)".format(len(gating)) + elif results: + if not final_attempt_ran or final_attempt_gating_count != 0: + gates = True + gate_reason = ( + "the final attempt did not itself pass with only quarantined " + "failures (ran={}, its own named failures={}, its own " + "unquarantined count={}); failures from an earlier attempt " + "cannot be trusted instead" + ).format(final_attempt_ran, final_attempt_failure_count, final_attempt_gating_count) + elif other_task_failures: + gates = True + gate_reason = "all failing tests are quarantined, but the build also failed in {}".format( + ", ".join(other_task_failures)) + elif args.final_attempt_exit_code not in (None, 0) and not final_attempt_failure_count: + gates = True + gate_reason = ( + "the final attempt named no failure of its own yet exited {}; " + "the run was cut short rather than passing, so the tests missing " + "from its results cannot be read as quarantined" + ).format(args.final_attempt_exit_code) + else: + gates = False + gate_reason = "all {} failing test(s) are quarantined".format(len(results)) + else: + gates = None + gate_reason = None + report = { "cell": args.cell, "attempts": ran, + "attempts_run": args.final_attempt, "flaky": [r for r in results if r["flaky"] and not r["quarantined"]], "persistent": [r for r in results if not r["flaky"] and not r["quarantined"]], "quarantined": [r for r in results if r["quarantined"]], @@ -157,6 +256,11 @@ def cmd_report(args): "failure_count": len(results), "final_attempt_ran": final_attempt_ran, "final_attempt_gating_count": final_attempt_gating_count, + "final_attempt_failure_count": final_attempt_failure_count, + "other_task_failures": other_task_failures, + "final_attempt_exit_code": args.final_attempt_exit_code, + "gates": gates, + "gate_reason": gate_reason, } os.makedirs(os.path.dirname(args.out) or ".", exist_ok=True) @@ -195,6 +299,18 @@ def main(): report.add_argument("--final-attempt", required=True, type=int, help="the attempt number the caller actually ran last") report.add_argument("--out", required=True) + report.add_argument("--final-attempt-exit-code", default=None, type=int, + help="the exit status of the final attempt's command; a non-zero " + "status with no named failure of its own means the run was " + "cut short and must not be excused by the list") + report.add_argument("--attempt-log", default=None, + help="the final attempt's raw log, to check for non-test task failures") + report.add_argument("--test-task-pattern", default=":ddprof-test:test", + help="Gradle task name the quarantine list is entitled to excuse") + report.add_argument("--evidence-suspect", action="store_true", + help="the caller could not reliably isolate this attempt's own " + "results (e.g. a stale results directory it could not clear); " + "never let the quarantine list excuse the exit code") report.set_defaults(func=cmd_report) args = parser.parse_args() diff --git a/.github/scripts/flake_summary.py b/.github/scripts/flake_summary.py index 447f2fc82e..52c5a86e29 100755 --- a/.github/scripts/flake_summary.py +++ b/.github/scripts/flake_summary.py @@ -31,11 +31,37 @@ # a fence-breaking ``` sequence into that render. _SAFE_TEST_ID_RE = re.compile(r"[^A-Za-z0-9_.$-]") +# The one place a failure message is truncated for display. flake_report.py +# stores messages at a wider cap (200 chars) for anyone reading the raw JSON; +# every renderer of this data (this module's tables, generate-test-summary.sh's +# per-job table) uses this same, narrower display width so the same failure +# does not render at two different lengths in one PR comment. +MESSAGE_DISPLAY_WIDTH = 120 + def sanitize_test_id(test_id): return _SAFE_TEST_ID_RE.sub("_", test_id) +def sanitize_quarantine_test_pattern(test_id): + """Sanitize a test id for the *paste-ready quarantine entry*, not display. + + sanitize_test_id() is safe for markdown but rewrites JUnit's parameterized- + and dynamic-test punctuation (brackets, parens, commas) to '_', producing a + pattern quarantine.covers() (exact string equality, or a trailing '.*') + can never match against the real id. When the method name would need that + rewriting to render safely, fall back to the class-wide '.*' pattern + instead, which is still an exact, matchable pattern and rendering-safe as + is (it contains no character _SAFE_TEST_ID_RE would touch). + """ + if not _SAFE_TEST_ID_RE.search(test_id): + return test_id + classname = test_id.rsplit(".", 1)[0] + if classname and not _SAFE_TEST_ID_RE.search(classname): + return classname + ".*" + return sanitize_test_id(test_id) + + def sanitize_inline(text): """Strip newlines and backticks so text can't break a table row, a code span, or the ``` fence around the quarantine proposals.""" @@ -69,7 +95,13 @@ def load_reports(root_dir): def group_by_test(reports, key): - """OrderedDict of test id -> {cells, message, ticket}.""" + """OrderedDict of test id -> {cells, message, ticket, tickets}. + + quarantine.py deliberately allows the same test to carry different + tickets on disjoint cell globs, so this keeps every ticket seen (not just + the first report's) and every message, rather than collapsing them to + whichever report happened to load first. + """ grouped = OrderedDict() for report in reports: for entry in report.get(key, []): @@ -77,8 +109,16 @@ def group_by_test(reports, key): "cells": [], "message": entry.get("message", ""), "ticket": entry.get("ticket"), + "tickets": [], + "messages": [], }) slot["cells"].append(report["cell"]) + ticket = entry.get("ticket") + if ticket and ticket not in slot["tickets"]: + slot["tickets"].append(ticket) + message = entry.get("message", "") + if message and message not in slot["messages"]: + slot["messages"].append(message) return grouped @@ -94,12 +134,22 @@ def render_table(grouped, cell_limit=4, row_limit=25, ticket_column=False): lines = [header, rule] for test_id, info in list(grouped.items())[:row_limit]: cells = info["cells"] - shown = ", ".join("`{}`".format(c) for c in cells[:cell_limit]) + # cell names come from this PR's own workflow file and must go + # through the same sanitizer as everything else rendered here. + shown = ", ".join("`{}`".format(sanitize_test_id(c)) for c in cells[:cell_limit]) if len(cells) > cell_limit: shown += " _+{} more_".format(len(cells) - cell_limit) - message = sanitize_inline((info["message"] or "").replace("|", "\\|"))[:120] + message = sanitize_inline((info["message"] or "").replace("|", "\\|"))[:MESSAGE_DISPLAY_WIDTH] message_cell = "`{}`".format(message) if message else "" - ticket = "{} | ".format(info.get("ticket") or "—") if ticket_column else "" + if ticket_column: + # ticket comes from this PR's own quarantine.txt line and is + # rendered into the same PR comment -- it must not be trusted + # unescaped any more than the test id or the message are. + tickets = info.get("tickets") or ([info["ticket"]] if info.get("ticket") else []) + ticket_text = ", ".join(sanitize_test_id(t) for t in tickets) or "—" + ticket = "{} | ".format(ticket_text) + else: + ticket = "" lines.append("| `{}` | {} | {}{} |".format( sanitize_test_id(short_name(test_id)), shown, ticket, message_cell)) if len(grouped) > row_limit: @@ -109,19 +159,23 @@ def render_table(grouped, cell_limit=4, row_limit=25, ticket_column=False): def cells_glob(cells): - """A glob covering these cells, when they share an obvious axis. + """A glob covering these cells, when they share one or more obvious axes. Suggesting `*aarch64*` for something that only ever failed on aarch64 is more useful than listing four cell names, and narrower than quarantining everywhere -- which would hide the same test breaking on x64 tomorrow. + Every shared axis narrows the glob further: a test failing only on + musl+aarch64 gets `*musl*aarch64*` rather than the wider `*aarch64*` + (which would also cover glibc aarch64). """ - for axis in ("aarch64", "amd64", "musl", "asan", "tsan"): - if all(axis in c for c in cells): - return ["*{}*".format(axis)] - return None + axes = ["aarch64", "amd64", "musl", "glibc", "asan", "tsan", "slow"] + shared = [axis for axis in axes if all(axis in c for c in cells)] + if not shared: + return None + return ["*" + "*".join(shared) + "*"] -def render_proposals(flaky): +def render_proposals(flaky, proposal_limit=25): today = datetime.date.today() review_by = (today + datetime.timedelta(days=DEFAULT_REVIEW_DAYS)).isoformat() out = [ @@ -143,19 +197,22 @@ def render_proposals(flaky): "```", "# test | ticket | added | review_by | cells | reason", ] - for test_id, info in flaky.items(): + items = list(flaky.items()) + for test_id, info in items[:proposal_limit]: reason = sanitize_inline("{} (seen in: {})".format( info["message"] or "intermittent failure", ", ".join(sorted(set(info["cells"]))[:4]), )).replace("|", "/") out.append(quarantine.format_entry( - sanitize_test_id(test_id), + sanitize_quarantine_test_pattern(test_id), "PROF-XXXXX", today.isoformat(), review_by, cells_glob(info["cells"]) or [], reason, )) + if len(items) > proposal_limit: + out.append("# ...and {} more. See the job logs.".format(len(items) - proposal_limit)) out.append("```") out.append("") out.append("") @@ -214,7 +271,11 @@ def main(): out.extend(render_table(quarantined, ticket_column=True)) out.append("") - retried = [r for r in reports if r.get("attempts", 1) > 1] + # attempts_run counts every attempt the runner actually executed; + # `attempts` counts only attempts that produced JUnit results, which + # undercounts a cell whose first attempt aborted before writing any XML + # (e.g. an ASan init abort) and only produced results on the retry. + retried = [r for r in reports if r.get("attempts_run", r.get("attempts", 1)) > 1] if retried: out.append("_Retried {} of {} cells._".format(len(retried), len(reports))) out.append("") diff --git a/.github/scripts/generate-test-summary.sh b/.github/scripts/generate-test-summary.sh index f34d3b6ccb..19fc6b710d 100755 --- a/.github/scripts/generate-test-summary.sh +++ b/.github/scripts/generate-test-summary.sh @@ -77,8 +77,6 @@ declare -A job_url=() job_url["__init__"]=1; unset 'job_url[__init__]' declare -A job_duration=() job_duration["__init__"]=1; unset 'job_duration[__init__]' -declare -A job_cell=() -job_cell["__init__"]=1; unset 'job_cell[__init__]' declare -a failed_jobs=() declare -a all_platforms=() declare -a all_java_versions=() @@ -126,8 +124,6 @@ while IFS= read -r job; do job_status["$key"]="$conclusion" job_url["$key"]="$html_url" job_duration["$key"]="$duration" - # Matches the cell label run_tests_with_retry.sh names its report after. - job_cell["$key"]="${libc}-${java_version}-${config}-${arch}${suite_suffix}" # Track failed jobs if [[ "$conclusion" == "failure" ]]; then @@ -184,10 +180,7 @@ for key in "${!job_status[@]}"; do fi done -# --- Download failure artifacts (if any failures) --- -declare -A failure_details=() -failure_details["__init__"]=1; unset 'failure_details[__init__]' - +# --- Download outcome artifacts (for flake_summary.py below) --- # Per-cell outcome reports, written by run_tests_with_retry.sh and uploaded # whether the cell passed or failed. A cell that only went green on a retry # produces no failure artifact at all, so this is the one place its flaky test @@ -197,34 +190,6 @@ log "Downloading CI outcome reports..." mkdir -p "$OUTCOME_DIR" gh run download "$RUN_ID" --pattern '(ci-outcome)*' --dir "$OUTCOME_DIR" 2>/dev/null || true -for key in "${failed_jobs[@]}"; do - cell="${job_cell[$key]:-}" - [[ -n "$cell" ]] || continue - - failures="" - while IFS= read -r report; do - # Flaky as well as persistent: a job that went red purely because an - # un-quarantined test failed once and passed on the retry is exactly - # the case this machinery creates, and it would otherwise render as - # "no detailed failure information". - if ! rows=$(jq -r '(.persistent + .flaky)[] | [.test, .message] | @tsv' "$report" 2>&1); then - log "WARNING: could not parse outcome report $report: $rows" - failures+="| _unreadable outcome report_ | \`$(basename "$report")\` could not be parsed; see the job log |"$'\n' - continue - fi - while IFS=$'\t' read -r test_id message; do - [[ -n "$test_id" ]] || continue - short_name="${test_id#"${test_id%.*.*}."}" - # A pipe in a failure message would split the row into extra - # columns and break the table, the way flake_summary.py escapes it. - message="${message//|/\\|}" - failures+="| \`${short_name}\` | ${message:-Test failed} |"$'\n' - done <<< "$rows" - done < <(find "$OUTCOME_DIR" -name "${cell}.json" 2>/dev/null) - - failure_details["$key"]="$failures" -done - # --- Generate markdown --- log "Generating markdown summary..." @@ -293,35 +258,23 @@ log "Generating markdown summary..." python3 "$(dirname "$0")/flake_summary.py" --dir "$OUTCOME_DIR" \ || echo "_Could not render the flaky-test summary; see the job log._" - # Failed tests details - if ((failed_count > 0)); then - echo "### Failed Tests" + # Failed jobs, linked to their logs. Which tests failed and why is the + # flaky/persistent/quarantined tables above -- rendering it a second time + # here, grouped by job instead of by test, only gave the same failure two + # different messages if the two renderers' sanitizing ever drifted. + if ((${#failed_jobs[@]} > 0)); then + echo "### Failed Jobs" echo "" - for key in "${failed_jobs[@]}"; do IFS='|' read -r platform java_version <<< "$key" url="${job_url[$key]:-}" - details="${failure_details[$key]:-}" - - echo "
" - echo "${platform} / ${java_version}" - echo "" if [[ -n "$url" ]]; then - echo "**Job:** [View logs]($url)" - echo "" - fi - - if [[ -n "$details" ]]; then - echo "| Test | Error |" - echo "|------|-------|" - echo -n "$details" + echo "- **${platform} / ${java_version}** — [view logs]($url)" else - echo "_No detailed failure information available. Check the job logs._" + echo "- **${platform} / ${java_version}**" fi - echo "" - echo "
" - echo "" done + echo "" fi # Summary statistics (single line) diff --git a/.github/scripts/prepare_reports.sh b/.github/scripts/prepare_reports.sh index 3ca5674911..e016a46da6 100755 --- a/.github/scripts/prepare_reports.sh +++ b/.github/scripts/prepare_reports.sh @@ -12,10 +12,17 @@ cp ddprof-test/javacore*.txt test-reports/ || true cp ddprof-test/build/hs_err* test-reports/ || true cp -r ddprof-lib/build/tmp test-reports/native_build || true cp -r ddprof-test/build/reports/tests test-reports/tests || true -# The JUnit XML of the final attempt, not just the rendered HTML, for reading -# by hand. Each attempt starts by deleting this directory, so the per-attempt -# evidence flake_report.py compares lives in flake-evidence/ (copied below). -cp -r ddprof-test/build/test-results test-reports/test-results || true +# The JUnit XML of every attempt, for reading by hand, normally comes from +# flake-evidence/ alone (copied below): run_tests_with_retry.sh snapshots +# each attempt's build/test-results there, and flake-evidence/attempt- +# holds exactly what build/test-results itself holds once the run is over. +# The one case that snapshots nothing at all is a suite that passed outright +# on its first attempt (skipped as a needless copy with no other attempt to +# compare against) -- copy build/test-results directly only then, so a green +# run still ships its JUnit XML. +if [ -z "$(find flake-evidence -mindepth 1 -maxdepth 1 -name 'attempt-*' 2>/dev/null)" ]; then + cp -r ddprof-test/build/test-results test-reports/test-results || true +fi cp -r flake-evidence test-reports/flake-evidence || true cp build/logs/gdb-watchdog.log test-reports/ || true cp -r /tmp/recordings test-reports/recordings || true diff --git a/.github/scripts/quarantine.py b/.github/scripts/quarantine.py index 06006cc9f9..f5a7890b2a 100755 --- a/.github/scripts/quarantine.py +++ b/.github/scripts/quarantine.py @@ -1,10 +1,10 @@ #!/usr/bin/env python3 """The quarantine list: which failing tests do not turn CI red. -Two jobs, one per subcommand: - - match split a cell's failures into gating and quarantined - validate enforce the format, the ticket, and the review_by date +Its `validate` subcommand enforces the format, the ticket, and the review_by +date. The gating decision itself (find_entry(), covers(), applies_to()) is a +library used in-process by flake_report.py -- there is no CLI for it, so the +rule CI actually runs cannot drift from a separate CLI wrapper. The paste-ready entry a PR comment proposes for a flaky test is rendered by flake_summary.py's own call to format_entry() below, not by this module's CLI. @@ -19,7 +19,6 @@ import argparse import datetime import fnmatch -import json import os import re import sys @@ -31,6 +30,16 @@ # Long enough not to be busywork, short enough that a quarantine outlives # neither the release it was added in nor the memory of why. DEFAULT_REVIEW_DAYS = 90 +# A review_by further out than this is not a review date, it is a way to write +# "never" without saying so. Padded above DEFAULT_REVIEW_DAYS since a proposal +# is dated `added` at the moment it is written, and review_by is measured from +# whenever the entry is actually appended -- which is not the same day. +MAX_REVIEW_DAYS = DEFAULT_REVIEW_DAYS + 30 +# The `test` field is an exact test id, optionally ending in a class-wide +# ".*" -- that is all covers() understands. Anything else (a bare "*", a "?", +# or a wildcard anywhere but as the final two characters) passes validate() +# today and then silently quarantines nothing at runtime. +BAD_TEST_WILDCARD_RE = re.compile(r"[*?]") # Cell names are ---. Only libc and arch are a closed # set -- jdk and config come from the workflow inputs and grow without warning @@ -44,30 +53,45 @@ # A synthetic universe of cell names, used only to ask whether two entries' # cell globs could both match the same real cell. Wide enough to catch a glob -# written against any axis (jdk, config, or the libc/arch pair) without having -# to enumerate the workflow's actual, ever-growing matrix. +# written against any axis (jdk, config, the libc/arch pair, or the slow/regular +# suite suffix) without having to enumerate the workflow's actual, ever-growing +# matrix. Deliberately over-inclusive (e.g. jdk variants like "8-j9" beyond the +# base list below): a synthetic cell that never occurs for real only makes +# overlap detection more conservative, never less. _SYNTHETIC_JDKS = ("8", "8-graal", "11", "17", "17-graal", "21", "25") _SYNTHETIC_CONFIGS = ("debug", "release", "asan", "tsan") +_SYNTHETIC_SUITE_SUFFIXES = ("", "-slow") SYNTHETIC_CELLS = tuple( - "{}-{}-{}-{}".format(libc, jdk, config, arch) + "{}-{}-{}-{}{}".format(libc, jdk, config, arch, suffix) for libc in KNOWN_LIBCS for jdk in _SYNTHETIC_JDKS for config in _SYNTHETIC_CONFIGS for arch in KNOWN_ARCHES + for suffix in _SYNTHETIC_SUITE_SUFFIXES ) +def _matches_any_synthetic_cell(globs): + return any(any(fnmatch.fnmatch(cell, g) for g in globs) for cell in SYNTHETIC_CELLS) + + def cells_overlap(globs_a, globs_b): """Could some real cell match both sets of globs? No globs means every cell. Equal glob lists always overlap without needing the synthetic universe, which matters when a glob names an axis (like a jdk or config) that - SYNTHETIC_CELLS does not model. + SYNTHETIC_CELLS does not model. And when a glob's axis is genuinely + unmodelled -- it matches nothing in the synthetic universe at all -- this + fails closed (treats it as overlapping) rather than open: a duplicate that + cells_overlap cannot evaluate is exactly the case validate() must not wave + through, since find_entry() would still only honour the first entry. """ if not globs_a or not globs_b: return True if sorted(globs_a) == sorted(globs_b): return True + if not _matches_any_synthetic_cell(globs_a) or not _matches_any_synthetic_cell(globs_b): + return True return any( any(fnmatch.fnmatch(cell, g) for g in globs_a) and any(fnmatch.fnmatch(cell, g) for g in globs_b) @@ -141,20 +165,6 @@ def format_entry(test, ticket, added, review_by, cells, reason): return " | ".join([test, ticket, added, review_by, ",".join(cells) or "-", reason]) -def cmd_match(args): - entries = load(args.list) - failures = [line.strip() for line in sys.stdin if line.strip()] - - gating, quarantined = [], [] - for test_id in failures: - hit = find_entry(entries, test_id, args.cell) - (quarantined if hit else gating).append(test_id) - - json.dump({"gating": gating, "quarantined": quarantined}, sys.stdout) - sys.stdout.write("\n") - return 0 - - def cmd_validate(args): # parse() tolerates a missing file so that matching still works before the # first entry lands. Validation must not: "0 quarantined test(s), all @@ -166,7 +176,7 @@ def cmd_validate(args): entries, problems = parse(args.list) today = datetime.date.today() - seen_by_name = {} + seen_by_name = [] def complain(line, message): problems.append((line, message)) @@ -181,18 +191,21 @@ def complain(line, message): if not entry[field]: complain(line, "field '{}' is empty".format(field)) - # Two entries for one test are fine when they cover disjoint cells -- - # that is what narrowing by cell is for. Two whose cell globs overlap - # are a copy-paste, and find_entry() only ever returns the first - # match, so the second one's ticket and review_by never take effect - # on the cells the two share. - for prior in seen_by_name.get(name, []): + # Two entries shadow each other on cells where they overlap when either + # pattern covers() the other -- not just when the `test` strings are + # identical. A trailing ".*" entry covers individual methods too, and + # find_entry() only ever returns the first match, so the second + # entry's ticket and review_by silently never take effect on the + # cells the two share. + for prior in seen_by_name: + if not (covers(prior, entry["test"]) or covers(entry, prior["test"])): + continue if cells_overlap(prior["cells"], entry["cells"]): where = ", ".join(entry["cells"]) or "every cell" - complain(line, "'{}' is already quarantined for {} on line {}".format( - name, where, prior["_line"])) + complain(line, "'{}' is already quarantined (as '{}') for {} on line {}".format( + name, prior["test"], where, prior["_line"])) break - seen_by_name.setdefault(name, []).append(entry) + seen_by_name.append(entry) if entry["ticket"] and not TICKET_RE.match(entry["ticket"]): complain(line, "ticket '{}' is not a PROF-".format(entry["ticket"])) @@ -201,13 +214,36 @@ def complain(line, message): if entry[field] and not DATE_RE.match(entry[field]): complain(line, "{} '{}' is not YYYY-MM-DD".format(field, entry[field])) + if entry["test"] and BAD_TEST_WILDCARD_RE.search( + entry["test"][:-2] if entry["test"].endswith(".*") else entry["test"] + ): + complain(line, ( + "test pattern '{}' has a wildcard outside a single trailing " + "'.*'; covers() only understands an exact id or a class-wide " + "'.*', so this would silently quarantine nothing" + ).format(entry["test"])) + + if entry["added"] and DATE_RE.match(entry["added"]): + try: + datetime.date.fromisoformat(entry["added"]) + except ValueError: + complain(line, "added '{}' is not a real calendar date".format(entry["added"])) + for pattern in entry["cells"]: - for token in ARCH_LIKE_RE.findall(pattern): - if token not in KNOWN_ARCHES: - complain(line, ( - "cell glob '{}' names architecture '{}', which CI never " - "builds (cells end in {}); it would quarantine nothing" - ).format(pattern, token, " or ".join(KNOWN_ARCHES))) + unknown_arch_tokens = [ + t for t in ARCH_LIKE_RE.findall(pattern) if t not in KNOWN_ARCHES + ] + # A token like "amd" or "aarch" (from "*amd*"/"*aarch*") is a + # legitimate abbreviation of a real arch and matches real cells; + # only complain when the glob, as actually evaluated by fnmatch, + # matches nothing in the synthetic universe -- that is what + # distinguishes a working glob from one like "*arm64*" that + # genuinely names an architecture CI never builds. + if unknown_arch_tokens and not _matches_any_synthetic_cell([pattern]): + complain(line, ( + "cell glob '{}' names architecture '{}', which CI never " + "builds (cells end in {}); it would quarantine nothing" + ).format(pattern, unknown_arch_tokens[0], " or ".join(KNOWN_ARCHES))) head = pattern.split("-", 1)[0] if head and "*" not in head and "?" not in head and head not in KNOWN_LIBCS: complain(line, ( @@ -228,6 +264,11 @@ def complain(line, message): "review_by with a note on {}." ).format(name, entry["added"], entry["review_by"], (today - due).days, entry["ticket"] or "the ticket")) + elif due > today + datetime.timedelta(days=MAX_REVIEW_DAYS): + complain(line, ( + "review_by '{}' is more than {} days out; that is not a " + "review date, it defeats the point of an expiring quarantine" + ).format(entry["review_by"], MAX_REVIEW_DAYS)) for line, message in sorted(problems): print("::error file={},line={}::{}".format(args.list, line, message)) @@ -245,10 +286,6 @@ def main(): parser.add_argument("--list", default=DEFAULT_LIST) sub = parser.add_subparsers(dest="command", required=True) - match = sub.add_parser("match", help="split stdin's failed test ids by quarantine status") - match.add_argument("--cell", required=True) - match.set_defaults(func=cmd_match) - validate = sub.add_parser("validate", help="check the list's format and review dates") validate.set_defaults(func=cmd_validate) diff --git a/.github/scripts/run_tests_with_retry.sh b/.github/scripts/run_tests_with_retry.sh index 4f215b423b..0fea0325ba 100755 --- a/.github/scripts/run_tests_with_retry.sh +++ b/.github/scripts/run_tests_with_retry.sh @@ -43,10 +43,25 @@ MAX_ATTEMPTS="${MAX_ATTEMPTS:-2}" MAX_FAILURES_TO_RETRY="${MAX_FAILURES_TO_RETRY:-3}" RETRY_ON_NO_TEST_FAILURES="${RETRY_ON_NO_TEST_FAILURES:-0}" +case "$MAX_ATTEMPTS" in + ''|*[!0-9]*|0) + echo "::error::MAX_ATTEMPTS must be a positive integer, got '${MAX_ATTEMPTS}'" + exit 1 + ;; +esac + HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -RESULTS_DIR="ddprof-test/build/test-results" +# Overridable so a caller outside :ddprof-test's own Gradle layout can point +# this at its own results directory instead of silently classifying an empty, +# never-populated evidence set as "no observed tests". +RESULTS_DIR="${RESULTS_DIR:-ddprof-test/build/test-results}" EVIDENCE_DIR="flake-evidence" OUTCOME_FILE="ci-outcome/${CELL}.json" +# Set when an attempt's evidence cannot be trusted as belonging to that +# attempt alone (e.g. a stale RESULTS_DIR that could not be cleared) -- the +# quarantine excuse must never fire on suspect evidence, no matter what the +# counts say. +EVIDENCE_SUSPECT=0 # Snapshot this attempt's JUnit XML before the next one overwrites it -- the # whole point is to compare attempts, and Gradle reuses the same directory. @@ -65,17 +80,35 @@ make_results_readable() { if foreign=$(find "$RESULTS_DIR" ! -user "$(id -u)" -print 2>/dev/null | head -n 1); then [ -n "$foreign" ] || { [ -w "$RESULTS_DIR" ] && [ -w "$(dirname "$RESULTS_DIR")" ] && return 0; } fi - command -v sudo >/dev/null 2>&1 || return 0 - # Include the parent so the pre-attempt `rm -rf "$RESULTS_DIR"` below (which - # needs to unlink the directory itself, not just its contents) can succeed. - sudo chmod -R a+rwX "$(dirname "$RESULTS_DIR")" 2>/dev/null \ - || echo "::warning::Could not take ownership of ${RESULTS_DIR}; flake evidence may be incomplete" + # Returning non-zero here is the whole contract: the tree holds files this + # user cannot read, so any snapshot taken from it is partial, and a partial + # snapshot is exactly what lets a final attempt's missing failures read back + # as a quarantined pass. The caller turns that into EVIDENCE_SUSPECT. + if ! command -v sudo >/dev/null 2>&1; then + echo "::warning::${RESULTS_DIR} has files not owned by $(id -un) and sudo is unavailable to fix that; flake evidence may be incomplete" + return 1 + fi + # Non-recursive on the parent: it only needs its own write bit so the + # pre-attempt `rm -rf "$RESULTS_DIR"` below can unlink the directory itself. + # Recursing over the whole module build tree (classes, jars, native libs, + # kept JFRs) would be orders of magnitude more inodes than needed and makes + # unrelated build output world-writable. + local ok=0 + sudo chmod a+rwX "$(dirname "$RESULTS_DIR")" 2>/dev/null \ + || { ok=1; echo "::warning::Could not make $(dirname "$RESULTS_DIR") writable; flake evidence may be incomplete"; } + sudo chmod -R a+rwX "$RESULTS_DIR" 2>/dev/null \ + || { ok=1; echo "::warning::Could not take ownership of ${RESULTS_DIR}; flake evidence may be incomplete"; } + return "$ok" } snapshot() { local attempt="$1" local dest="${EVIDENCE_DIR}/attempt-${attempt}" - make_results_readable + # The XML being snapshotted was written by the command that just ran, after + # the loop-top make_results_readable() -- under Docker it lands root-owned, + # so read access has to be taken again here or the cp below fails and the + # cell silently loses its flake evidence. + make_results_readable || EVIDENCE_SUSPECT=1 rm -rf "$dest" || echo "::warning::Could not clear ${dest}; attempt ${attempt} evidence may be stale" mkdir -p "$dest" if [ -d "$RESULTS_DIR" ]; then @@ -88,29 +121,29 @@ snapshot() { # the quarantine list has any business excusing. TEST_TASK_PATTERN="${TEST_TASK_PATTERN:-:ddprof-test:test}" -# Task failures the quarantine list must never wave through. -non_test_task_failures() { - local log="$1" - [ -f "$log" ] || return 0 - grep -oE "Execution failed for task '[^']+'" "$log" 2>/dev/null \ - | sed -E "s/^Execution failed for task '//; s/'$//" \ - | grep -v -F "$TEST_TASK_PATTERN" \ - | sort -u -} - # Self-contained state: a leftover attempt-2 from an earlier run on a reused # workspace would be read back as this run's evidence, inflating the attempt # count and importing failures that never happened here. rm -rf "$EVIDENCE_DIR" "$(dirname "$OUTCOME_FILE")" EXIT_CODE=1 -ATTEMPT_LOG="" +# A single, per-attempt-truncated log: only the final attempt's is ever read +# (by flake_report.py's non-test-task-failure check below), and keeping one +# copy per attempt on disk earned nothing but wasted space. +ATTEMPT_LOG="build/logs/attempt.log" for attempt in $(seq 1 "$MAX_ATTEMPTS"); do mkdir -p build/logs - make_results_readable - rm -rf "$RESULTS_DIR" \ - || echo "::warning::Could not clear ${RESULTS_DIR} before attempt ${attempt}; it may inherit the previous attempt's results and a flake will look persistent" - ATTEMPT_LOG="build/logs/attempt-${attempt}.log" + make_results_readable || EVIDENCE_SUSPECT=1 + if ! rm -rf "$RESULTS_DIR"; then + echo "::warning::Could not clear ${RESULTS_DIR} before attempt ${attempt}; it may inherit the previous attempt's results and a flake will look persistent" + # A stale RESULTS_DIR here means this attempt's snapshot can end up being + # the *previous* attempt's JUnit XML, which would let a final attempt that + # actually crashed without running a single test be read back as having + # "passed with only quarantined failures". Never let the quarantine excuse + # fire on evidence that might not be this attempt's own. + EVIDENCE_SUSPECT=1 + fi + : > "$ATTEMPT_LOG" "$@" 2>&1 \ | tee -a build/test-raw.log \ @@ -118,7 +151,14 @@ for attempt in $(seq 1 "$MAX_ATTEMPTS"); do | python3 -u "${HERE}/filter_gradle_log.py" EXIT_CODE=${PIPESTATUS[0]} - snapshot "$attempt" + # A first-attempt pass has no prior attempt to compare against, so its + # snapshot could only ever yield an empty flake report; skip the find + # traversal, possible sudo chmod, and recursive copy that nobody will read. + # A later-attempt pass still needs its snapshot -- that is the evidence that + # proves the earlier failure was a flake. + if [ "$EXIT_CODE" -ne 0 ] || [ "$attempt" -gt 1 ]; then + snapshot "$attempt" + fi if [ "$EXIT_CODE" -eq 0 ]; then break @@ -158,11 +198,17 @@ for attempt in $(seq 1 "$MAX_ATTEMPTS"); do ./gradlew --stop 2>/dev/null || true done -python3 "${HERE}/flake_report.py" --list "$QUARANTINE_LIST" report \ - --cell "$CELL" \ - --evidence-dir "$EVIDENCE_DIR" \ - --final-attempt "$attempt" \ +REPORT_ARGS=(--list "$QUARANTINE_LIST" report + --cell "$CELL" + --evidence-dir "$EVIDENCE_DIR" + --final-attempt "$attempt" --out "$OUTCOME_FILE" + --attempt-log "$ATTEMPT_LOG" + --final-attempt-exit-code "$EXIT_CODE" + --test-task-pattern "$TEST_TASK_PATTERN") +[ "$EVIDENCE_SUSPECT" = "1" ] && REPORT_ARGS+=(--evidence-suspect) + +python3 "${HERE}/flake_report.py" "${REPORT_ARGS[@]}" REPORT_STATUS=$? # A classifier that did not run cannot vouch for a green suite: it is the only @@ -173,75 +219,47 @@ if [ "$REPORT_STATUS" -ne 0 ]; then exit 1 fi -# The quarantine list, not the retry, decides whether the job goes red. -# -# any un-quarantined failure -> red, even if the retry passed. A flake that -# nobody has quarantined is still a failure; -# letting the retry excuse it is how flakes get -# tolerated for years. -# every failure quarantined -> green, but only when the *final* attempt is -# the one vouching for that: failures -# aggregated across every attempt can all be -# quarantined while the final attempt itself -# failed for a reason that named no test at -# all (a docker or Gradle failure, an -# OOM-killed daemon, an ASan init abort), and -# the list has no business excusing that. -# no test named -> keep the command's own exit code: a compile -# error or a dead runner is nothing to do with -# quarantine. +# flake_report.py owns the gating verdict -- it has every count and the +# quarantine list in hand, so re-deriving the decision here (as this script +# used to, with an inline python, two case sanity checks, and a bash +# if/elif chain) is one more independent reader of the outcome-JSON schema +# for no benefit. "gates" is true/false to override EXIT_CODE, or the string +# "none" when there is no failure to have an opinion about, in which case the +# command's own exit code stands (a compile error or a dead runner is nothing +# to do with quarantine). if [ -f "$OUTCOME_FILE" ]; then - summary=$(python3 -c " + decision=$(python3 -c " import json, sys -d = json.load(open(sys.argv[1])) -final_gating = d['final_attempt_gating_count'] -print(d['gating_count'], d['failure_count'], int(d['final_attempt_ran']), - final_gating if final_gating is not None else -1) +try: + d = json.load(open(sys.argv[1])) + if 'gates' not in d: + sys.exit('missing key: gates') +except Exception as e: + sys.exit(str(e)) +gates = d['gates'] +print('none' if gates is None else ('true' if gates else 'false')) +print(d.get('gate_reason') or '') " "$OUTCOME_FILE") || { - echo "::error::Could not read ${OUTCOME_FILE}; failing the job rather than guessing whether its failures gate" + echo "::error::Could not read ${OUTCOME_FILE} (${decision:-no output}); failing the job rather than guessing whether its failures gate" exit 1 } - read -r gating failures final_ran final_gating <<< "$summary" - case "${gating}:${failures}:${final_ran}" in - *[!0-9:]*|:*|*:|*::*) - echo "::error::${OUTCOME_FILE} did not yield usable counts (got '${summary}'); failing the job" - exit 1 + gates=$(echo "$decision" | sed -n '1p') + reason=$(echo "$decision" | sed -n '2p') + case "$gates" in + true) + echo "::error::${CELL} fails: ${reason}" + EXIT_CODE=1 ;; - esac - case "$final_gating" in - -1|*[!0-9]*) - [ "$final_gating" = "-1" ] || { - echo "::error::${OUTCOME_FILE} did not yield a usable final-attempt gating count (got '${summary}'); failing the job" - exit 1 - } + false) + echo "::warning::${CELL} is not failing the job: ${reason}" + EXIT_CODE=0 + ;; + none) ;; + *) + echo "::error::${OUTCOME_FILE} did not yield a usable gating decision (got '${gates}'); failing the job" + exit 1 ;; esac - - if [ "$gating" -gt 0 ]; then - EXIT_CODE=1 - elif [ "$failures" -gt 0 ]; then - if [ "$final_ran" -ne 1 ] || [ "$final_gating" -ne 0 ]; then - # The final attempt either produced no test results of its own (a - # build or infrastructure failure, not something quarantine speaks to) - # or still has its own named failures unquarantined -- either way the - # list has nothing to say about why this attempt is red. - echo "::error::${CELL}'s final attempt did not itself pass with only quarantined failures (ran=${final_ran}, its own gating count=${final_gating}); failing the job rather than trusting failures from an earlier attempt" - EXIT_CODE=1 - else - # Quarantine excuses the tests it names. It does not excuse the build: - # if this same invocation also failed a compile, a native gtest or a - # verification task, that failure has nothing to do with the list and - # zeroing the exit code here would bury it. - other=$(non_test_task_failures "$ATTEMPT_LOG") - if [ -n "$other" ]; then - echo "::error::All ${failures} failing test(s) in ${CELL} are quarantined, but the build also failed in $(echo "$other" | tr '\n' ' ')— failing the job" - EXIT_CODE=1 - else - echo "::warning::All ${failures} failing test(s) in ${CELL} are quarantined; not failing the job" - EXIT_CODE=0 - fi - fi - fi fi exit "$EXIT_CODE" diff --git a/.github/scripts/tests/test_generate_test_summary.sh b/.github/scripts/tests/test_generate_test_summary.sh index 5ba0677864..594d960ae3 100755 --- a/.github/scripts/tests/test_generate_test_summary.sh +++ b/.github/scripts/tests/test_generate_test_summary.sh @@ -77,10 +77,10 @@ EOJ echo "== generate-test-summary.sh: outcome report parsing ==" -# A well-formed, failing outcome report must be rendered as a real failure -# row -- not swallowed into the "unreadable outcome report" fallback. This is -# the path a mutated `if ! rows=$(jq ...)` (dropping the `!`) would break: jq -# succeeding on valid JSON would then take the branch meant for jq failing. +# A well-formed, failing outcome report must render its real failure via +# flake_summary.py's own table -- the one place this failure is rendered, now +# that generate-test-summary.sh no longer re-parses ci-outcome JSON itself +# and duplicates that table under each job. CASE="$TEMP_DIR/case-valid-report" mkdir -p "$CASE/jobs" "$CASE/outcomes" "$CASE/work" write_jobs_fixture "$CASE/jobs/jobs.json" failure @@ -101,13 +101,15 @@ echo "$summary" | grep -q "FooTest.bar" \ || fail "expected the real failing test in the summary, got: $summary" echo "$summary" | grep -q "assertion failed: boom" \ || fail "expected the real failure message in the summary, got: $summary" -if echo "$summary" | grep -q "_unreadable outcome report_"; then - fail "a valid outcome report was rendered as unreadable, got: $summary" -fi -pass "a valid outcome report renders its real failure, not the unreadable fallback" +echo "$summary" | grep -q "Failed Jobs" \ + || fail "expected the failed job to be listed and linked, got: $summary" +echo "$summary" | grep -q "https://example.invalid/job/1" \ + || fail "expected the failed job's log link, got: $summary" +pass "a valid outcome report renders its real failure via flake_summary.py, and the job is linked" -# A malformed outcome report (invalid JSON) must be rendered as unreadable, -# not silently dropped or fed further down the pipeline as if it were rows. +# A malformed outcome report (invalid JSON) must be flagged rather than +# silently dropped -- this is now flake_summary.py's own "could not be +# parsed" fallback, the one reader of ci-outcome JSON left in this pipeline. CASE="$TEMP_DIR/case-malformed-report" mkdir -p "$CASE/jobs" "$CASE/outcomes" "$CASE/work" write_jobs_fixture "$CASE/jobs/jobs.json" failure @@ -119,9 +121,30 @@ printf 'this is not json\n' > "$CASE/outcomes/glibc-17-debug-amd64.json" "$SCRIPT" 12345 "$CASE/work/summary.md" ) || fail "generate-test-summary.sh exited non-zero on a malformed outcome report" summary=$(cat "$CASE/work/summary.md") -echo "$summary" | grep -q "_unreadable outcome report_" \ +echo "$summary" | grep -q "could not be parsed" \ || fail "expected a malformed outcome report to be flagged unreadable, got: $summary" pass "a malformed outcome report is flagged unreadable rather than silently ignored" +# A run where every test job passed must not print an empty "Failed Jobs" +# section -- that guard is what stands between a green run and a stray, +# empty heading (or, on a bash whose indexed-array expansion under `set -u` +# minds an empty array, a hard failure). +CASE="$TEMP_DIR/case-all-green" +mkdir -p "$CASE/jobs" "$CASE/outcomes" "$CASE/work" +write_jobs_fixture "$CASE/jobs/jobs.json" success +( + cd "$CASE/work" + export GH_JOBS_FIXTURE="$CASE/jobs/jobs.json" GH_OUTCOME_FIXTURE_DIR="$CASE/outcomes" + export GITHUB_REPOSITORY="DataDog/java-profiler" GITHUB_SHA="deadbeefcafef00dfeedfacebeefcafebeefcafe" + "$SCRIPT" 12345 "$CASE/work/summary.md" +) || fail "generate-test-summary.sh exited non-zero on an all-passing run" +summary=$(cat "$CASE/work/summary.md") +if echo "$summary" | grep -q "Failed Jobs"; then + fail "an all-passing run must not print a Failed Jobs section, got: $summary" +fi +echo "$summary" | grep -q "All 1 test jobs passed" \ + || fail "expected the all-passed banner, got: $summary" +pass "an all-passing run prints no Failed Jobs section" + echo echo "All $TESTS generate-test-summary tests passed." diff --git a/.github/scripts/tests/test_quarantine.sh b/.github/scripts/tests/test_quarantine.sh index bb7610a9f4..6b102cbbf1 100755 --- a/.github/scripts/tests/test_quarantine.sh +++ b/.github/scripts/tests/test_quarantine.sh @@ -117,24 +117,38 @@ python3 "$SCRIPTS/quarantine.py" --list "$ROOT/ddprof-test/quarantine.txt" valid || fail "the committed quarantine list is invalid" pass "the committed quarantine list is valid" -echo "== quarantine.py match ==" +echo "== quarantine.find_entry (the rule the gating decision actually uses) ==" write_list "$LIST" "$(entry a.B.c PROF-1 "$(day_offset 30)" '*aarch64*')" -result=$(printf 'a.B.c\n' | python3 "$SCRIPTS/quarantine.py" --list "$LIST" match --cell "glibc-17-debug-aarch64") -echo "$result" | grep -q '"quarantined": \["a.B.c"\]' \ - || fail "expected a.B.c quarantined on an aarch64 cell, got: $result" +result=$(python3 -c " +import sys; sys.path.insert(0, '$SCRIPTS') +import quarantine +entries = quarantine.load('$LIST') +print('hit' if quarantine.find_entry(entries, 'a.B.c', 'glibc-17-debug-aarch64') else 'miss') +") +[ "$result" = "hit" ] || fail "expected a.B.c quarantined on an aarch64 cell, got: $result" pass "a cell glob matches the cells it names" -result=$(printf 'a.B.c\n' | python3 "$SCRIPTS/quarantine.py" --list "$LIST" match --cell "glibc-17-debug-amd64") -echo "$result" | grep -q '"gating": \["a.B.c"\]' \ - || fail "expected a.B.c gating on an amd64 cell, got: $result" +result=$(python3 -c " +import sys; sys.path.insert(0, '$SCRIPTS') +import quarantine +entries = quarantine.load('$LIST') +print('hit' if quarantine.find_entry(entries, 'a.B.c', 'glibc-17-debug-amd64') else 'miss') +") +[ "$result" = "miss" ] || fail "expected a.B.c gating (not quarantined) on an amd64 cell, got: $result" pass "a cell glob does not match other cells" write_list "$LIST" "$(entry 'a.B.*' PROF-1 "$(day_offset 30)")" -result=$(printf 'a.B.c\na.B.d\na.C.e\n' | python3 "$SCRIPTS/quarantine.py" --list "$LIST" match --cell "any") -echo "$result" | grep -q '"gating": \["a.C.e"\]' \ - || fail "expected only a.C.e to gate under a class wildcard, got: $result" +result=$(python3 -c " +import sys; sys.path.insert(0, '$SCRIPTS') +import quarantine +entries = quarantine.load('$LIST') +tests = ['a.B.c', 'a.B.d', 'a.C.e'] +gating = [t for t in tests if quarantine.find_entry(entries, t, 'any') is None] +print(','.join(gating)) +") +[ "$result" = "a.C.e" ] || fail "expected only a.C.e to gate under a class wildcard, got: $result" pass "a class wildcard covers that class only" echo "== gating: run_tests_with_retry.sh ==" @@ -327,6 +341,112 @@ set -e [ "$rc" -ne 0 ] || fail "a final attempt that named no test must not be excused by an earlier attempt's quarantined failure (got exit $rc)" pass "a final attempt naming no test is never excused by an earlier attempt's quarantine hit" +# Regression: a final attempt that crashes part-way through still writes JUnit +# XML for the tests it got to, and those all passed -- so it names no failure +# of its own and every aggregated failure is quarantined. Gradle blames the +# abort on the test task itself, so the non-test-task check cannot see it +# either. Only the attempt's own non-zero exit distinguishes this from the +# ordinary flaky-then-passed case, and it must stay red. +CASE="$TEMP_DIR/case-final-attempt-crashed-mid-run" +mkdir -p "$CASE" +cat > "$CASE/suite.sh" </dev/null || echo 0) + 1 )); echo \$n > .n +OUT=ddprof-test/build/test-results/testDebug +mkdir -p "\$OUT" +$(declare -f write_failure_xml) +$(declare -f write_pass_xml) +if [ "\$n" -eq 1 ]; then + write_failure_xml "\$OUT" "com.dd.WobblyTest" "sometimesFails" "got 2 samples, wanted 50" + exit 1 +fi +write_pass_xml "\$OUT" "com.dd.WobblyTest" "sometimesFails" +echo "# A fatal error has been detected by the Java Runtime Environment: SIGSEGV" +echo "Execution failed for task ':ddprof-test:test'." +echo "> Process 'Gradle Test Executor 3' finished with non-zero exit value 134" +exit 134 +EOS +chmod +x "$CASE/suite.sh" +write_list "$CASE/list.txt" "$(entry com.dd.WobblyTest.sometimesFails PROF-1 "$(day_offset 30)")" +set +e +output=$(cd "$CASE" && "$SCRIPTS/run_tests_with_retry.sh" --list list.txt "glibc-17-debug-amd64" -- ./suite.sh 2>&1) +rc=$? +set -e +[ "$rc" -ne 0 ] || fail "a final attempt that crashed after recording only passes must not be excused by quarantine (got exit $rc)" +pass "a final attempt that crashed mid-run is not read as a quarantined pass" + +# The counterpart: the same shape without the crash is the ordinary +# flaky-then-passed case a quarantine entry exists to excuse, and must be +# green. Without this the guard above could be satisfied by gating everything. +CASE="$TEMP_DIR/case-quarantined-flake-recovers" +mkdir -p "$CASE" +cat > "$CASE/suite.sh" </dev/null || echo 0) + 1 )); echo \$n > .n +OUT=ddprof-test/build/test-results/testDebug +mkdir -p "\$OUT" +$(declare -f write_failure_xml) +$(declare -f write_pass_xml) +if [ "\$n" -eq 1 ]; then + write_failure_xml "\$OUT" "com.dd.WobblyTest" "sometimesFails" "got 2 samples, wanted 50" + exit 1 +fi +write_pass_xml "\$OUT" "com.dd.WobblyTest" "sometimesFails" +exit 0 +EOS +chmod +x "$CASE/suite.sh" +write_list "$CASE/list.txt" "$(entry com.dd.WobblyTest.sometimesFails PROF-1 "$(day_offset 30)")" +set +e +output=$(cd "$CASE" && "$SCRIPTS/run_tests_with_retry.sh" --list list.txt "glibc-17-debug-amd64" -- ./suite.sh 2>&1) +rc=$? +set -e +[ "$rc" -eq 0 ] || fail "a quarantined flake that passed on retry must stay green (got exit $rc): $output" +pass "a quarantined flake that recovers on a clean retry is still excused" + +# ...but only when the evidence it rests on is complete. Docker writes the +# JUnit XML as root; if this user cannot take ownership of it, the snapshot is +# partial, and a partial snapshot is indistinguishable from an attempt whose +# missing tests all passed. make_results_readable() must report that rather +# than fail open, and the run must go red. +CASE="$TEMP_DIR/case-unreadable-results" +mkdir -p "$CASE/stub-bin" +cat > "$CASE/stub-bin/find" <<'EOS' +#!/usr/bin/env bash +for a in "$@"; do + if [ "$a" = "-user" ]; then echo "/root-owned/TEST-Foo.xml"; exit 0; fi +done +exec /usr/bin/find "$@" +EOS +cat > "$CASE/stub-bin/sudo" <<'EOS' +#!/usr/bin/env bash +exit 1 +EOS +chmod +x "$CASE/stub-bin/find" "$CASE/stub-bin/sudo" +cat > "$CASE/suite.sh" </dev/null || echo 0) + 1 )); echo \$n > .n +OUT=ddprof-test/build/test-results/testDebug +mkdir -p "\$OUT" +$(declare -f write_failure_xml) +$(declare -f write_pass_xml) +if [ "\$n" -eq 1 ]; then + write_failure_xml "\$OUT" "com.dd.WobblyTest" "sometimesFails" "got 2 samples, wanted 50" + exit 1 +fi +write_pass_xml "\$OUT" "com.dd.WobblyTest" "sometimesFails" +exit 0 +EOS +chmod +x "$CASE/suite.sh" +write_list "$CASE/list.txt" "$(entry com.dd.WobblyTest.sometimesFails PROF-1 "$(day_offset 30)")" +set +e +output=$(cd "$CASE" && PATH="$CASE/stub-bin:$PATH" "$SCRIPTS/run_tests_with_retry.sh" --list list.txt "glibc-17-debug-amd64" -- ./suite.sh 2>&1) +rc=$? +set -e +[ "$rc" -ne 0 ] || fail "results that could not be made readable must not be excused by quarantine (got exit $rc)" +echo "$output" | grep -q "suspect" \ + || fail "expected the suspect evidence to be named as the reason, got: $output" +pass "evidence that could not be made readable is never excused by the quarantine list" + # A test missing from the retry never re-ran, so it is not evidence of a flake. CASE="$TEMP_DIR/case-absent-is-not-passed" mkdir -p "$CASE/flake-evidence/attempt-1" "$CASE/flake-evidence/attempt-2" @@ -344,6 +464,35 @@ assert len(d['persistent']) == 1, d " "$CASE/out.json" || fail "absence from a later attempt was treated as a pass" pass "a test missing from the retry is not mistaken for a flake" +# A missing name or classname cannot be attributed to any real +# test; it must be skipped rather than counted as a failure or crashing the +# classifier, while a properly-identified failure alongside it still counts. +CASE="$TEMP_DIR/case-unnamed-testcase" +mkdir -p "$CASE/flake-evidence/attempt-1" +cat > "$CASE/flake-evidence/attempt-1/TEST-com.dd.Weird.xml" <<'EOS' + + + + + + + + + +EOS +write_list "$CASE/list.txt" +python3 "$SCRIPTS/flake_report.py" --list "$CASE/list.txt" report \ + --cell "glibc-17-debug-amd64" --evidence-dir "$CASE/flake-evidence" \ + --final-attempt 1 --out "$CASE/out.json" >/dev/null 2>&1 \ + || fail "a testcase with no name attribute must not crash the classifier" +python3 -c " +import json,sys +d = json.load(open(sys.argv[1])) +assert d['failure_count'] == 1, d +assert d['persistent'] and d['persistent'][0]['test'] == 'com.dd.Weird.realFailure', d +" "$CASE/out.json" || fail "the unnamed testcase was not ignored, or the real failure was missed" +pass "a testcase with no name attribute is ignored, not mistaken for a failure" + # A stray attempt-* directory must not abort classification, and attempt-1 # must still be read as the (only, and so final) real attempt. CASE="$TEMP_DIR/case-stray-attempt" @@ -394,11 +543,14 @@ echo "== flake_summary.py renders ==" CASE="$TEMP_DIR/case-summary" mkdir -p "$CASE/outcomes" cat > "$CASE/outcomes/glibc-17-debug-aarch64.json" <<'EOS' -{"cell": "glibc-17-debug-aarch64", "attempts": 2, "status": "fail", +{"cell": "glibc-17-debug-aarch64", "attempts": 2, "attempts_run": 2, "flaky": [{"test": "com.dd.WobblyTest.sometimesFails", "failed_attempts": [1], - "passed_attempts": [2], "message": "got 2 | wanted 50", + "message": "got 2 | wanted 50", "flaky": true, "quarantined": false, "ticket": null}], - "persistent": [], "quarantined": [], "gating_count": 1, "failure_count": 1} + "persistent": [], "quarantined": [], "gating_count": 1, "failure_count": 1, + "final_attempt_ran": true, "final_attempt_gating_count": 0, + "final_attempt_failure_count": 0, "other_task_failures": [], + "gates": true, "gate_reason": "1 un-quarantined failure(s)"} EOS summary=$(python3 "$SCRIPTS/flake_summary.py" --dir "$CASE/outcomes") \ || fail "flake_summary.py must render without error" diff --git a/.github/workflows/test_workflow.yml b/.github/workflows/test_workflow.yml index 3f5f055066..d9a13216dd 100644 --- a/.github/workflows/test_workflow.yml +++ b/.github/workflows/test_workflow.yml @@ -173,13 +173,19 @@ jobs: # The slow/e2e suite already runs the best part of an hour, so a # retry would risk the 180-minute job timeout, and it records - # failures without re-running them. That rationale does not hold - # under ASan: the retry above fires on an init abort that costs - # seconds, not a full slow run, so ASan keeps its second attempt - # even when slow. + # failures without re-running them. ASan keeps its second attempt + # even when slow, but only for the init abort above (0 named + # failures): MAX_FAILURES_TO_RETRY=0 stops a slow ASan run that + # actually failed named tests from being retried too, which would + # re-run the full slow suite a second time and risk that same + # timeout. export MAX_ATTEMPTS=2 - if [[ "${{ inputs.slow_tests }}" == "true" && "${{ matrix.config }}" != "asan" ]]; then - export MAX_ATTEMPTS=1 + if [[ "${{ inputs.slow_tests }}" == "true" ]]; then + if [[ "${{ matrix.config }}" == "asan" ]]; then + export MAX_FAILURES_TO_RETRY=0 + else + export MAX_ATTEMPTS=1 + fi fi .github/scripts/run_tests_with_retry.sh \ @@ -532,13 +538,19 @@ jobs: # The slow/e2e suite already runs the best part of an hour, so a # retry would risk the 180-minute job timeout, and it records - # failures without re-running them. That rationale does not hold - # under ASan: the retry above fires on an init abort that costs - # seconds, not a full slow run, so ASan keeps its second attempt - # even when slow. + # failures without re-running them. ASan keeps its second attempt + # even when slow, but only for the init abort above (0 named + # failures): MAX_FAILURES_TO_RETRY=0 stops a slow ASan run that + # actually failed named tests from being retried too, which would + # re-run the full slow suite a second time and risk that same + # timeout. export MAX_ATTEMPTS=2 - if [[ "${{ inputs.slow_tests }}" == "true" && "${{ matrix.config }}" != "asan" ]]; then - export MAX_ATTEMPTS=1 + if [[ "${{ inputs.slow_tests }}" == "true" ]]; then + if [[ "${{ matrix.config }}" == "asan" ]]; then + export MAX_FAILURES_TO_RETRY=0 + else + export MAX_ATTEMPTS=1 + fi fi .github/scripts/run_tests_with_retry.sh \ diff --git a/ddprof-test/quarantine.txt b/ddprof-test/quarantine.txt index fb5f42c362..74504f057c 100644 --- a/ddprof-test/quarantine.txt +++ b/ddprof-test/quarantine.txt @@ -14,9 +14,12 @@ # quarantined is a decision somebody renews rather than the # default. 90 days is the usual span. # cells Comma-separated globs against the cell name -# (---), e.g. "*aarch64*" or -# "musl-*,*-asan-*". Leave as "-" to quarantine everywhere; prefer -# narrowing it, so the same test breaking elsewhere still gates. +# (---[-slow]) -- the slow/e2e suite gets +# its own cells, suffixed "-slow", distinct from the regular +# suite's -- e.g. "*aarch64*", "musl-*,*-asan-*", or "*-slow" to +# target only the slow suite. Leave as "-" to quarantine +# everywhere; prefer narrowing it, so the same test breaking +# elsewhere still gates. # reason Free text — what is unreliable and how often. Last field, so it # may contain anything but "|". # From e93cad96f63e46b3d55ac1dbbb02ef3259c8d00b Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Tue, 15 Sep 2026 11:31:53 +0200 Subject: [PATCH 5/9] ci: cover musl, and close four holes in the quarantine gating 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/ -- 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 ---[-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) --- .github/scripts/flake_report.py | 30 ++--- .github/scripts/flake_summary.py | 7 +- .github/scripts/quarantine.py | 26 ++++- .github/scripts/run_tests_with_retry.sh | 2 +- .github/scripts/tests/test_quarantine.sh | 104 ++++++++++++++++++ .../datadoghq/profiler/ProfilerTestPlugin.kt | 6 + .../profiler/test/ProfilerTestRunner.java | 18 +++ gradle/libs.versions.toml | 3 +- 8 files changed, 178 insertions(+), 18 deletions(-) diff --git a/.github/scripts/flake_report.py b/.github/scripts/flake_report.py index efaccbc7af..1d9f3ac552 100755 --- a/.github/scripts/flake_report.py +++ b/.github/scripts/flake_report.py @@ -196,14 +196,17 @@ def cmd_report(args): # outright (zero failures of its own) is # the ordinary flaky-then-fixed case and # must not gate. - # final attempt exited -> gate. It ran, recorded results, and - # non-zero having named named no failure of its own, yet the - # no failure of its own command still failed: the JVM aborted - # part-way through, so the tests it never - # reached are absent from the XML rather - # than passing. Gradle blames the crash on - # the test task itself, so the non-test - # task check above cannot see it. + # final attempt exited -> gate, regardless of how many of its own + # non-zero failures were named and quarantined. + # The JVM can abort part-way through + # after naming one real (quarantined) + # failure, so the tests it never reached + # are absent from the XML rather than + # passing; a quarantined name or two must + # not paper over that. Gradle blames the + # crash on the test task itself, so the + # non-test task check above cannot see + # it. # no failure named -> no opinion; the caller keeps its own # exit code (a compile error or a dead # runner is nothing to do with @@ -231,13 +234,14 @@ def cmd_report(args): gates = True gate_reason = "all failing tests are quarantined, but the build also failed in {}".format( ", ".join(other_task_failures)) - elif args.final_attempt_exit_code not in (None, 0) and not final_attempt_failure_count: + elif args.final_attempt_exit_code not in (None, 0): gates = True gate_reason = ( - "the final attempt named no failure of its own yet exited {}; " - "the run was cut short rather than passing, so the tests missing " - "from its results cannot be read as quarantined" - ).format(args.final_attempt_exit_code) + "the final attempt named {} failure(s) of its own (all " + "quarantined) yet exited {}; the run was cut short rather than " + "cleanly passing, so the tests missing from its results cannot " + "be read as quarantined" + ).format(final_attempt_failure_count, args.final_attempt_exit_code) else: gates = False gate_reason = "all {} failing test(s) are quarantined".format(len(results)) diff --git a/.github/scripts/flake_summary.py b/.github/scripts/flake_summary.py index 52c5a86e29..05afffaf91 100755 --- a/.github/scripts/flake_summary.py +++ b/.github/scripts/flake_summary.py @@ -167,8 +167,13 @@ def cells_glob(cells): Every shared axis narrows the glob further: a test failing only on musl+aarch64 gets `*musl*aarch64*` rather than the wider `*aarch64*` (which would also cover glibc aarch64). + + Cell names are `---[-slow]`. `*`-joining is + order-sensitive, so the axes here must be listed in that same left-to-right + order (libc, config, arch, suite suffix) -- axes out of order yields a glob + fnmatch can never match against the very cells it was derived from. """ - axes = ["aarch64", "amd64", "musl", "glibc", "asan", "tsan", "slow"] + axes = ["glibc", "musl", "debug", "release", "asan", "tsan", "amd64", "aarch64", "slow"] shared = [axis for axis in axes if all(axis in c for c in cells)] if not shared: return None diff --git a/.github/scripts/quarantine.py b/.github/scripts/quarantine.py index f5a7890b2a..b9b2459d8e 100755 --- a/.github/scripts/quarantine.py +++ b/.github/scripts/quarantine.py @@ -149,14 +149,36 @@ def covers(entry, test_id): return test_id == pattern +def is_expired(entry, today=None): + """True once this entry's review_by date has passed. + + validate-quarantine (quarantine.py's own CLI) only runs from ci.yml, but + find_entry() is called from every workflow that reuses run_tests_with_retry.sh + (nightly.yml, release-validated.yml included). Enforcing expiry here, at + match time, means a stale mute cannot keep quarantining a failure just + because the workflow that hit it never runs the separate validator. + """ + review_by = entry.get("review_by", "") + if not DATE_RE.match(review_by): + return False + try: + due = datetime.date.fromisoformat(review_by) + except ValueError: + return False + return due < (today or datetime.date.today()) + + def find_entry(entries, test_id, cell): """The first entry quarantining this test on this cell, or None. Every caller that decides whether a failure gates goes through here, so the - matching rule cannot drift between the subcommand and flake_report.py. + matching rule cannot drift between the subcommand and flake_report.py. An + expired entry is treated as absent rather than as a hit, so it can never + excuse a failure outside the PR CI that happens to run validate-quarantine. """ return next( - (e for e in entries if covers(e, test_id) and applies_to(e, cell)), + (e for e in entries + if covers(e, test_id) and applies_to(e, cell) and not is_expired(e)), None, ) diff --git a/.github/scripts/run_tests_with_retry.sh b/.github/scripts/run_tests_with_retry.sh index 0fea0325ba..dc289e192a 100755 --- a/.github/scripts/run_tests_with_retry.sh +++ b/.github/scripts/run_tests_with_retry.sh @@ -113,7 +113,7 @@ snapshot() { mkdir -p "$dest" if [ -d "$RESULTS_DIR" ]; then cp -r "$RESULTS_DIR"/. "$dest"/ \ - || echo "::warning::Could not snapshot ${RESULTS_DIR} for attempt ${attempt}; flake classification for this cell will be incomplete" + || { EVIDENCE_SUSPECT=1; echo "::warning::Could not snapshot ${RESULTS_DIR} for attempt ${attempt}; flake classification for this cell will be incomplete"; } fi } diff --git a/.github/scripts/tests/test_quarantine.sh b/.github/scripts/tests/test_quarantine.sh index 6b102cbbf1..337c4767fb 100755 --- a/.github/scripts/tests/test_quarantine.sh +++ b/.github/scripts/tests/test_quarantine.sh @@ -151,6 +151,21 @@ print(','.join(gating)) [ "$result" = "a.C.e" ] || fail "expected only a.C.e to gate under a class wildcard, got: $result" pass "a class wildcard covers that class only" +# validate-quarantine only runs from ci.yml, but find_entry() is the function +# every workflow's flake_report.py invocation actually calls -- nightly.yml +# and release-validated.yml reuse the retry/quarantine machinery without ever +# running validate-quarantine, so an expired entry must stop matching here +# too, not just be caught by the separate CLI check above. +write_list "$LIST" "$(entry a.B.c PROF-1 "$(day_offset -1)")" +result=$(python3 -c " +import sys; sys.path.insert(0, '$SCRIPTS') +import quarantine +entries = quarantine.load('$LIST') +print('hit' if quarantine.find_entry(entries, 'a.B.c', 'any') else 'miss') +") +[ "$result" = "miss" ] || fail "expected an expired entry to gate rather than match, got: $result" +pass "find_entry() treats an expired entry as absent, not as a hit" + echo "== gating: run_tests_with_retry.sh ==" # A suite that fails one test on the first attempt and passes on the second. @@ -403,6 +418,36 @@ set -e [ "$rc" -eq 0 ] || fail "a quarantined flake that passed on retry must stay green (got exit $rc): $output" pass "a quarantined flake that recovers on a clean retry is still excused" +# A final attempt can name only quarantined failures and still exit non-zero +# -- a JVM abort partway through, after writing XML for the one test it +# reached. The tests it never got to are missing from the XML, not passing; +# a quarantined name or two must not paper over that crash. +CASE="$TEMP_DIR/case-final-attempt-quarantined-failure-plus-crash" +mkdir -p "$CASE" +cat > "$CASE/suite.sh" </dev/null || echo 0) + 1 )); echo \$n > .n +OUT=ddprof-test/build/test-results/testDebug +mkdir -p "\$OUT" +$(declare -f write_failure_xml) +write_failure_xml "\$OUT" "com.dd.WobblyTest" "sometimesFails" "got 2 samples, wanted 50" +if [ "\$n" -gt 1 ]; then + echo "# A fatal error has been detected by the Java Runtime Environment: SIGSEGV" + echo "Execution failed for task ':ddprof-test:test'." + echo "> Process 'Gradle Test Executor 3' finished with non-zero exit value 134" + exit 134 +fi +exit 1 +EOS +chmod +x "$CASE/suite.sh" +write_list "$CASE/list.txt" "$(entry com.dd.WobblyTest.sometimesFails PROF-1 "$(day_offset 30)")" +set +e +output=$(cd "$CASE" && "$SCRIPTS/run_tests_with_retry.sh" --list list.txt "glibc-17-debug-amd64" -- ./suite.sh 2>&1) +rc=$? +set -e +[ "$rc" -ne 0 ] || fail "a final attempt that crashed after naming only a quarantined failure must not be excused (got exit $rc)" +pass "a final attempt naming a quarantined failure is still gated if it also crashed" + # ...but only when the evidence it rests on is complete. Docker writes the # JUnit XML as root; if this user cannot take ownership of it, the snapshot is # partial, and a partial snapshot is indistinguishable from an attempt whose @@ -447,6 +492,39 @@ echo "$output" | grep -q "suspect" \ || fail "expected the suspect evidence to be named as the reason, got: $output" pass "evidence that could not be made readable is never excused by the quarantine list" +# A `cp -r` that fails partway through RESULTS_DIR must poison the evidence +# the same way an unreadable results directory does above -- even when the +# one failure it *did* manage to copy is quarantined, the files it could not +# copy are missing from this attempt's snapshot, not passing. Stubbed like +# the find/sudo cases above rather than chmod'd, since a root-run CI job +# would not actually be denied read access by chmod. +CASE="$TEMP_DIR/case-partial-snapshot-copy" +mkdir -p "$CASE/stub-bin" +cat > "$CASE/stub-bin/cp" <<'EOS' +#!/usr/bin/env bash +exit 1 +EOS +chmod +x "$CASE/stub-bin/cp" +cat > "$CASE/suite.sh" </dev/null || echo 0) + 1 )); echo \$n > .n +OUT=ddprof-test/build/test-results/testDebug +mkdir -p "\$OUT" +$(declare -f write_failure_xml) +write_failure_xml "\$OUT" "com.dd.WobblyTest" "sometimesFails" "got 2 samples, wanted 50" +exit 1 +EOS +chmod +x "$CASE/suite.sh" +write_list "$CASE/list.txt" "$(entry com.dd.WobblyTest.sometimesFails PROF-1 "$(day_offset 30)")" +set +e +output=$(cd "$CASE" && PATH="$CASE/stub-bin:$PATH" "$SCRIPTS/run_tests_with_retry.sh" --list list.txt "glibc-17-debug-amd64" -- ./suite.sh 2>&1) +rc=$? +set -e +[ "$rc" -ne 0 ] || fail "a failed snapshot copy must not be excused even though the named failure is quarantined (got exit $rc)" +echo "$output" | grep -q "suspect" \ + || fail "expected the suspect evidence to be named as the reason, got: $output" +pass "a failed/partial snapshot copy sets EVIDENCE_SUSPECT and forces gating" + # A test missing from the retry never re-ran, so it is not evidence of a flake. CASE="$TEMP_DIR/case-absent-is-not-passed" mkdir -p "$CASE/flake-evidence/attempt-1" "$CASE/flake-evidence/attempt-2" @@ -562,5 +640,31 @@ echo "$summary" | grep -q 'got 2 \\| wanted 50' \ || fail "expected the pipe in the message to be escaped, got: $summary" pass "the PR summary renders the flaky table and a proposal" +echo "== flake_summary.cells_glob ==" + +# Every glob cells_glob() proposes must actually fnmatch the cells it was +# derived from -- a glob built with axes in the wrong order (or missing an +# axis real cells vary on, like config) looks plausible but can never match, +# silently proposing a quarantine entry that quarantines nothing. +python3 -c " +import fnmatch, sys +sys.path.insert(0, '$SCRIPTS') +import flake_summary as fs + +cases = [ + ['glibc-17-debug-amd64', 'glibc-21-debug-amd64'], + ['musl-17-release-aarch64', 'musl-11-release-aarch64'], + ['glibc-17-debug-amd64-slow'], + ['glibc-17-asan-amd64', 'musl-17-asan-amd64'], +] +for cells in cases: + globs = fs.cells_glob(cells) or [] + for cell in cells: + assert any(fnmatch.fnmatch(cell, g) for g in globs), ( + 'cells_glob(%r) = %r does not match %r' % (cells, globs, cell)) +print('ok') +" | grep -q '^ok$' || fail "a cells_glob() proposal did not fnmatch the cells it was derived from" +pass "every cells_glob() proposal fnmatches the cells it was derived from" + echo echo "All $TESTS quarantine tests passed." diff --git a/build-logic/conventions/src/main/kotlin/com/datadoghq/profiler/ProfilerTestPlugin.kt b/build-logic/conventions/src/main/kotlin/com/datadoghq/profiler/ProfilerTestPlugin.kt index e0b9691433..b7fbae4632 100644 --- a/build-logic/conventions/src/main/kotlin/com/datadoghq/profiler/ProfilerTestPlugin.kt +++ b/build-logic/conventions/src/main/kotlin/com/datadoghq/profiler/ProfilerTestPlugin.kt @@ -368,6 +368,12 @@ class ProfilerTestPlugin : Plugin { allArgs.add("-D$key=$value") } + // Same build/test-results/ layout Gradle's own Test task uses, + // so run_tests_with_retry.sh's TEST-*.xml scan finds musl's results the + // same way it finds every other cell's. + val reportsDir = project.layout.buildDirectory.dir("test-results/$taskName").get().asFile + allArgs.add("-Dtest.reportsDir=${reportsDir.absolutePath}") + // UNIFIED INTERFACE: Test filter from -Ptests property val testsFilter = project.findProperty("tests") as String? if (testsFilter != null) { diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/test/ProfilerTestRunner.java b/ddprof-test/src/test/java/com/datadoghq/profiler/test/ProfilerTestRunner.java index ff7c0a2e68..29376b2e24 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/test/ProfilerTestRunner.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/test/ProfilerTestRunner.java @@ -18,8 +18,11 @@ import org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder; import org.junit.platform.launcher.core.LauncherFactory; import org.junit.platform.launcher.listeners.SummaryGeneratingListener; +import org.junit.platform.reporting.legacy.xml.LegacyXmlReportGeneratingListener; import java.io.PrintWriter; +import java.nio.file.Path; +import java.nio.file.Paths; import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; @@ -41,6 +44,10 @@ * - -Dtest.filter=*.Pattern* - Pattern matching on class names * - -Dtest.tags.include=tag1,tag2 - Only run tests tagged with one of these tags * - -Dtest.tags.exclude=tag1,tag2 - Skip tests tagged with any of these tags + * - -Dtest.reportsDir=path - Write JUnit XML reports here (same TEST-*.xml format + * Gradle's Test task produces), so flake_report.py can + * classify musl failures the same way it classifies every + * other cell */ public class ProfilerTestRunner { public static void main(String[] args) { @@ -132,6 +139,17 @@ private static void runTests() { SummaryGeneratingListener listener = new SummaryGeneratingListener(); launcher.registerTestExecutionListeners(new GradleStyleTestListener(), listener); + // Without this, musl runs produce no TEST-*.xml at all: flake_report.py has + // nothing to read, so every failure on musl is invisible to flake + // classification and quarantine, and a passing musl attempt looks + // indistinguishable from one that never ran. + String reportsDir = System.getProperty("test.reportsDir"); + if (reportsDir != null && !reportsDir.isEmpty()) { + Path reportsPath = Paths.get(reportsDir); + reportsPath.toFile().mkdirs(); + launcher.registerTestExecutionListeners(new LegacyXmlReportGeneratingListener(reportsPath, new PrintWriter(System.err))); + } + // Execute tests launcher.execute(request); diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 1f8c21b5dd..b3efc0d0f1 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -39,6 +39,7 @@ junit-api = { module = "org.junit.jupiter:junit-jupiter-api", version.ref = "jun junit-engine = { module = "org.junit.jupiter:junit-jupiter-engine", version.ref = "junit" } junit-params = { module = "org.junit.jupiter:junit-jupiter-params", version.ref = "junit" } junit-platform-launcher = { module = "org.junit.platform:junit-platform-launcher", version.ref = "junit-platform" } +junit-platform-reporting = { module = "org.junit.platform:junit-platform-reporting", version.ref = "junit-platform" } junit-pioneer = { module = "org.junit-pioneer:junit-pioneer", version.ref = "junit-pioneer" } # Logging @@ -66,7 +67,7 @@ jmh-annprocess = { module = "org.openjdk.jmh:jmh-generator-annprocess", version. [bundles] # Core testing framework (JUnit + logging + launcher API for custom test runner) -testing = ["junit-api", "junit-engine", "junit-params", "junit-platform-launcher", "junit-pioneer", "slf4j-simple"] +testing = ["junit-api", "junit-engine", "junit-params", "junit-platform-launcher", "junit-platform-reporting", "junit-pioneer", "slf4j-simple"] # Profiler runtime dependencies (JFR analysis + compression) profiler-runtime = ["jmc-flightrecorder", "jol-core", "lz4", "snappy", "zstd"] From c50f8be5d46add29fb99bc2ee4b695bdca2ec71d Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Tue, 15 Sep 2026 13:15:17 +0200 Subject: [PATCH 6/9] ci: let a quarantined failure excuse the exit code it caused 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) --- .github/scripts/flake_report.py | 83 +++++++++++++++++++----- .github/scripts/tests/test_quarantine.sh | 76 ++++++++++++++++++++++ 2 files changed, 142 insertions(+), 17 deletions(-) diff --git a/.github/scripts/flake_report.py b/.github/scripts/flake_report.py index 1d9f3ac552..d13f2d63ab 100755 --- a/.github/scripts/flake_report.py +++ b/.github/scripts/flake_report.py @@ -110,6 +110,16 @@ def cmd_count(args): _NON_TEST_TASK_FAILURE_RE = re.compile(r"Execution failed for task '([^']+)'") +# Gradle reporting that a forked test JVM died, and the JVM's own crash banner. +# An ordinary test failure makes Gradle exit non-zero too, so the exit code by +# 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" + r"|A fatal error has been detected by the Java Runtime Environment" + r"|hs_err_pid" +) + def non_test_task_failures(log_path, test_task_pattern): """Gradle task names blamed for a failure, other than the test task itself. @@ -129,6 +139,24 @@ def non_test_task_failures(log_path, test_task_pattern): return sorted(found) +def cut_short_marker(log_path): + """The log line fragment showing this invocation was cut short, or None. + + A missing or unreadable log yields None, matching non_test_task_failures() + above: with no log there is no evidence either way, and the alternative + would be to gate every quarantined failure on the strength of a file the + caller happened not to pass. + """ + if not log_path or not os.path.isfile(log_path): + return None + with open(log_path, errors="replace") as handle: + for line in handle: + m = _CUT_SHORT_RE.search(line) + if m: + return m.group(0) + return None + + def cmd_report(args): attempts = collect_attempts(args.evidence_dir) ran = len(attempts) @@ -175,6 +203,19 @@ def cmd_report(args): other_task_failures = non_test_task_failures(args.attempt_log, args.test_task_pattern) + # Two independent signs that the final attempt stopped early rather than + # running to completion and failing tests: Gradle saying so in the log, and + # the attempt having reached fewer tests than another attempt managed. The + # latter needs more than one attempt to compare against, which slow suites + # (MAX_ATTEMPTS=1) do not have, so the log is the primary signal. + final_attempt_cut_short = cut_short_marker(args.attempt_log) + observed_shortfall = None + if final_attempt_ran and ran > 1: + best_observed = max(len(seen) for _, seen, _ in attempts) + final_observed = len(final[1]) + if final_observed < best_observed: + observed_shortfall = (final_observed, best_observed) + # The gating verdict, owned here rather than re-derived by the caller from # raw counts: three independent readers of this file re-deciding the same # thing is how a schema change turns into shotgun surgery. @@ -196,17 +237,17 @@ def cmd_report(args): # outright (zero failures of its own) is # the ordinary flaky-then-fixed case and # must not gate. - # final attempt exited -> gate, regardless of how many of its own - # non-zero failures were named and quarantined. - # The JVM can abort part-way through - # after naming one real (quarantined) - # failure, so the tests it never reached - # are absent from the XML rather than - # passing; a quarantined name or two must - # not paper over that. Gradle blames the - # crash on the test task itself, so the - # non-test task check above cannot see - # it. + # final attempt exited -> gate only with evidence that it was cut + # non-zero and was cut short 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 all by itself, so treating + # every non-zero exit as a crash would + # gate the ordinary case the list exists + # to excuse. Gradle blames a crash on the + # test task itself, so the non-test task + # check above cannot see it. # no failure named -> no opinion; the caller keeps its own # exit code (a compile error or a dead # runner is nothing to do with @@ -234,14 +275,20 @@ def cmd_report(args): gates = True gate_reason = "all failing tests are quarantined, but the build also failed in {}".format( ", ".join(other_task_failures)) - elif args.final_attempt_exit_code not in (None, 0): + elif args.final_attempt_exit_code not in (None, 0) and final_attempt_cut_short: + gates = True + gate_reason = ( + "the final attempt exited {} and its log shows the run was cut " + "short ({!r}), so the tests missing from its results cannot be " + "read as quarantined" + ).format(args.final_attempt_exit_code, final_attempt_cut_short) + elif args.final_attempt_exit_code not in (None, 0) and observed_shortfall: gates = True gate_reason = ( - "the final attempt named {} failure(s) of its own (all " - "quarantined) yet exited {}; the run was cut short rather than " - "cleanly passing, so the tests missing from its results cannot " - "be read as quarantined" - ).format(final_attempt_failure_count, args.final_attempt_exit_code) + "the final attempt exited {} having reached only {} of the {} " + "tests another attempt ran, so it stopped early and the tests " + "missing from its results cannot be read as quarantined" + ).format(args.final_attempt_exit_code, *observed_shortfall) else: gates = False gate_reason = "all {} failing test(s) are quarantined".format(len(results)) @@ -262,6 +309,8 @@ def cmd_report(args): "final_attempt_gating_count": final_attempt_gating_count, "final_attempt_failure_count": final_attempt_failure_count, "other_task_failures": other_task_failures, + "final_attempt_cut_short": final_attempt_cut_short, + "final_attempt_observed_shortfall": observed_shortfall, "final_attempt_exit_code": args.final_attempt_exit_code, "gates": gates, "gate_reason": gate_reason, diff --git a/.github/scripts/tests/test_quarantine.sh b/.github/scripts/tests/test_quarantine.sh index 337c4767fb..36bdb3a0fe 100755 --- a/.github/scripts/tests/test_quarantine.sh +++ b/.github/scripts/tests/test_quarantine.sh @@ -590,6 +590,82 @@ assert d['persistent'] and d['persistent'][0]['test'] == 'com.dd.WobblyTest.some " "$CASE/out.json" || fail "attempt-1 was not read back despite the stray attempt-tmp" pass "attempt-1 is still read as evidence while the stray directory is skipped" +# The ordinary quarantine case, and the one the exit-code guard must not eat: +# a quarantined test fails on the final attempt, so Gradle exits non-zero +# *because of that very test*. No test passed --final-attempt-exit-code before, +# so it defaulted to None and this shape went untested while the guard gated +# every one of them. +CASE="$TEMP_DIR/case-quarantined-failure-exit-nonzero" +mkdir -p "$CASE/flake-evidence/attempt-1" +write_failure_xml "$CASE/flake-evidence/attempt-1" "com.dd.WobblyTest" "sometimesFails" "boom" +cat > "$CASE/attempt.log" <<'EOS' +> Task :ddprof-test:testDebug FAILED +Execution failed for task ':ddprof-test:testDebug'. +> There were failing tests. +EOS +write_list "$CASE/list.txt" "$(entry com.dd.WobblyTest.sometimesFails PROF-1 "$(day_offset 30)")" +python3 "$SCRIPTS/flake_report.py" --list "$CASE/list.txt" report \ + --cell "glibc-17-debug-amd64" --evidence-dir "$CASE/flake-evidence" \ + --final-attempt 1 --attempt-log "$CASE/attempt.log" \ + --final-attempt-exit-code 1 --test-task-pattern test \ + --out "$CASE/out.json" >/dev/null 2>&1 +python3 -c " +import json,sys +d = json.load(open(sys.argv[1])) +assert d['gates'] is False, 'a quarantined test failing on the final attempt must not gate: %r' % d['gate_reason'] +assert len(d['quarantined']) == 1, d +" "$CASE/out.json" || fail "the non-zero exit caused by the quarantined test's own failure was read as a crash" +pass "a quarantined failure still excuses the non-zero exit it caused" + +# The shape the guard exists for: the same all-quarantined failure list, but +# Gradle reports the test JVM died, so tests it never reached are absent from +# the XML rather than passing. +CASE="$TEMP_DIR/case-quarantined-but-cut-short" +mkdir -p "$CASE/flake-evidence/attempt-1" +write_failure_xml "$CASE/flake-evidence/attempt-1" "com.dd.WobblyTest" "sometimesFails" "boom" +cat > "$CASE/attempt.log" <<'EOS' +> Task :ddprof-test:testDebug FAILED +Process 'Gradle Test Executor 1' finished with non-zero exit value 134 +EOS +write_list "$CASE/list.txt" "$(entry com.dd.WobblyTest.sometimesFails PROF-1 "$(day_offset 30)")" +python3 "$SCRIPTS/flake_report.py" --list "$CASE/list.txt" report \ + --cell "glibc-17-debug-amd64" --evidence-dir "$CASE/flake-evidence" \ + --final-attempt 1 --attempt-log "$CASE/attempt.log" \ + --final-attempt-exit-code 1 --test-task-pattern test \ + --out "$CASE/out.json" >/dev/null 2>&1 +python3 -c " +import json,sys +d = json.load(open(sys.argv[1])) +assert d['gates'] is True, 'a cut-short final attempt must gate even with every named failure quarantined: %r' % d['gate_reason'] +assert d['final_attempt_cut_short'], d +" "$CASE/out.json" || fail "a dead test JVM was excused by the quarantine list" +pass "a cut-short final attempt gates despite its failures being quarantined" + +# Same intent, without the log saying so: the final attempt reached fewer tests +# than an earlier one managed, so it stopped early. +CASE="$TEMP_DIR/case-quarantined-but-short-run" +mkdir -p "$CASE/flake-evidence/attempt-1" "$CASE/flake-evidence/attempt-2" +write_failure_xml "$CASE/flake-evidence/attempt-1" "com.dd.WobblyTest" "sometimesFails" "boom" +write_pass_xml "$CASE/flake-evidence/attempt-1" "com.dd.OtherTest" "stable" +write_failure_xml "$CASE/flake-evidence/attempt-2" "com.dd.WobblyTest" "sometimesFails" "boom" +cat > "$CASE/attempt.log" <<'EOS' +> Task :ddprof-test:testDebug FAILED +Execution failed for task ':ddprof-test:testDebug'. +EOS +write_list "$CASE/list.txt" "$(entry com.dd.WobblyTest.sometimesFails PROF-1 "$(day_offset 30)")" +python3 "$SCRIPTS/flake_report.py" --list "$CASE/list.txt" report \ + --cell "glibc-17-debug-amd64" --evidence-dir "$CASE/flake-evidence" \ + --final-attempt 2 --attempt-log "$CASE/attempt.log" \ + --final-attempt-exit-code 1 --test-task-pattern test \ + --out "$CASE/out.json" >/dev/null 2>&1 +python3 -c " +import json,sys +d = json.load(open(sys.argv[1])) +assert d['gates'] is True, 'a final attempt that reached fewer tests must gate: %r' % d['gate_reason'] +assert d['final_attempt_observed_shortfall'], d +" "$CASE/out.json" || fail "a final attempt that stopped early was excused by the quarantine list" +pass "a final attempt reaching fewer tests than an earlier one gates" + echo "== validate rejects unmatchable cell globs ==" write_list "$LIST" "$(entry a.B.c PROF-1 "$(day_offset 30)" '*arm64*')" From 35dbbe2b55a5db8ec73bee4cfbace314f6d3f35d Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Tue, 15 Sep 2026 13:15:40 +0200 Subject: [PATCH 7/9] ci: match the test ids JUnit actually writes 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 ".*" 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 .* -- 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) --- .github/scripts/flake_summary.py | 6 +++ .github/scripts/quarantine.py | 33 ++++++++++++++ .github/scripts/tests/test_quarantine.sh | 56 ++++++++++++++++++++++++ ddprof-test/quarantine.txt | 10 ++++- 4 files changed, 104 insertions(+), 1 deletion(-) diff --git a/.github/scripts/flake_summary.py b/.github/scripts/flake_summary.py index 05afffaf91..53f2d28fa6 100755 --- a/.github/scripts/flake_summary.py +++ b/.github/scripts/flake_summary.py @@ -54,6 +54,12 @@ def sanitize_quarantine_test_pattern(test_id): instead, which is still an exact, matchable pattern and rendering-safe as is (it contains no character _SAFE_TEST_ID_RE would touch). """ + # A plain JUnit 5 method arrives as `Class.method()`. Those parentheses are + # the only unsafe characters in it, and quarantine.covers() normalises them + # away, so proposing the documented `Class.method` keeps the entry precise + # rather than muting the whole class. + if test_id.endswith("()") and not _SAFE_TEST_ID_RE.search(test_id[:-2]): + return test_id[:-2] if not _SAFE_TEST_ID_RE.search(test_id): return test_id classname = test_id.rsplit(".", 1)[0] diff --git a/.github/scripts/quarantine.py b/.github/scripts/quarantine.py index b9b2459d8e..8011a0f19c 100755 --- a/.github/scripts/quarantine.py +++ b/.github/scripts/quarantine.py @@ -142,8 +142,25 @@ def applies_to(entry, cell): return any(fnmatch.fnmatch(cell, g) for g in globs) +_INVOCATION_INDEX_RE = re.compile(r"^\[\d+\]$") + + +def normalise_test_id(test_id): + """A JUnit XML test id reduced to the shape entries are written in. + + Gradle writes a JUnit 5 @Test method as `name="method()"`, so the id built + from the report is `Class.method()`, while quarantine.txt documents -- and + a human writes -- `Class.method`. Stripping the parentheses here, in the + one function every caller's match goes through, keeps the documented shape + matching the id JUnit actually produces while reports and annotations go + on showing the real name. + """ + return test_id[:-2] if test_id.endswith("()") else test_id + + def covers(entry, test_id): pattern = entry["test"] + test_id = normalise_test_id(test_id) if pattern.endswith(".*"): return test_id.startswith(pattern[:-1]) return test_id == pattern @@ -245,6 +262,22 @@ def complain(line, message): "'.*', so this would silently quarantine nothing" ).format(entry["test"])) + if entry["test"].endswith("()"): + complain(line, ( + "test '{}' carries the parentheses JUnit puts in its XML; " + "entries are written as ., and covers() " + "normalises the report's id to that shape -- drop the '()'" + ).format(entry["test"])) + + if _INVOCATION_INDEX_RE.match(entry["test"].rsplit(".", 1)[-1]): + complain(line, ( + "test '{}' names an invocation index. @ParameterizedTest and " + "@RetryingTest invocations appear in the XML as '[1]', '[2]' " + "with no method name at all, so the index identifies neither " + "the method nor a stable case -- quarantine the class with " + "'.*' instead" + ).format(entry["test"])) + if entry["added"] and DATE_RE.match(entry["added"]): try: datetime.date.fromisoformat(entry["added"]) diff --git a/.github/scripts/tests/test_quarantine.sh b/.github/scripts/tests/test_quarantine.sh index 36bdb3a0fe..6faf4b4401 100755 --- a/.github/scripts/tests/test_quarantine.sh +++ b/.github/scripts/tests/test_quarantine.sh @@ -666,6 +666,62 @@ assert d['final_attempt_observed_shortfall'], d " "$CASE/out.json" || fail "a final attempt that stopped early was excused by the quarantine list" pass "a final attempt reaching fewer tests than an earlier one gates" +# Gradle writes a JUnit 5 @Test method as name="method()", so the id built from +# the report is Class.method() while an entry is written Class.method. Without +# normalisation covers() (exact equality) matches nothing and the quarantine +# silently excuses nothing, with validate still reporting the list as valid. +CASE="$TEMP_DIR/case-method-parens" +mkdir -p "$CASE/flake-evidence/attempt-1" +cat > "$CASE/flake-evidence/attempt-1/TEST-com.dd.WobblyTest.xml" <<'EOS' + + + + + + +EOS +write_list "$CASE/list.txt" "$(entry com.dd.WobblyTest.sometimesFails PROF-1 "$(day_offset 30)")" +python3 "$SCRIPTS/flake_report.py" --list "$CASE/list.txt" report \ + --cell "glibc-17-debug-amd64" --evidence-dir "$CASE/flake-evidence" \ + --final-attempt 1 --out "$CASE/out.json" >/dev/null 2>&1 +python3 -c " +import json,sys +d = json.load(open(sys.argv[1])) +assert len(d['quarantined']) == 1, 'an entry written as documented must match the id JUnit produces: %r' % d +assert d['quarantined'][0]['test'] == 'com.dd.WobblyTest.sometimesFails()', d +assert d['gates'] is False, d['gate_reason'] +" "$CASE/out.json" || fail "the documented . form did not match Gradle's method() id" +pass "an entry written without parentheses matches JUnit's method() id" + +# The paste-ready proposal must be precise for such a method, not widened to +# the whole class: the parens are the only unsafe characters and covers() +# normalises them away. +CASE="$TEMP_DIR/case-proposal-parens" +python3 -c " +import sys +sys.path.insert(0, '$SCRIPTS') +import flake_summary +got = flake_summary.sanitize_quarantine_test_pattern('com.dd.WobblyTest.sometimesFails()') +assert got == 'com.dd.WobblyTest.sometimesFails', got +idx = flake_summary.sanitize_quarantine_test_pattern('com.dd.WobblyTest.[1]') +assert idx == 'com.dd.WobblyTest.*', idx +" || fail "the proposal for a method() id was not the documented form, or an indexed invocation was not widened class-wide" +pass "a method() id proposes the documented form; an indexed invocation proposes the class" + +echo "== validate rejects unmatchable test patterns ==" + +write_list "$LIST" "$(entry 'com.dd.WobblyTest.sometimesFails()' PROF-1 "$(day_offset 30)")" +if python3 "$SCRIPTS/quarantine.py" --list "$LIST" validate >/dev/null 2>&1; then + fail "a test field carrying JUnit's parentheses should be rejected" +fi +pass "a test pattern written with parentheses is rejected" + +write_list "$LIST" "$(entry 'com.dd.WobblyTest.[1]' PROF-1 "$(day_offset 30)")" +if python3 "$SCRIPTS/quarantine.py" --list "$LIST" validate >/dev/null 2>&1; then + fail "a test field naming an invocation index should be rejected" +fi +pass "a test pattern naming an invocation index is rejected" + echo "== validate rejects unmatchable cell globs ==" write_list "$LIST" "$(entry a.B.c PROF-1 "$(day_offset 30)" '*arm64*')" diff --git a/ddprof-test/quarantine.txt b/ddprof-test/quarantine.txt index 74504f057c..7fbd2121da 100644 --- a/ddprof-test/quarantine.txt +++ b/ddprof-test/quarantine.txt @@ -5,8 +5,16 @@ # # test | ticket | added | review_by | cells | reason # -# test Fully qualified .. A trailing ".*" covers every +# test Fully qualified ., written without the +# parentheses JUnit puts in its own XML report -- CI normalises +# the report's id to this shape. A trailing ".*" covers every # method in the class. +# +# @ParameterizedTest and @RetryingTest invocations cannot be +# named individually: the XML records them as "[1]", "[2]" with +# no method name at all, so the index identifies neither the +# method nor a stable case. Quarantine those class-wide with +# ".*"; CI's suggested line already does. # ticket PROF-. Required — a quarantine without a ticket is just # a test nobody runs. # added YYYY-MM-DD, the day it went in. From 4d7e6a8bcc41ed0fa5730620ef85ec95f371f90d Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Tue, 15 Sep 2026 13:32:18 +0200 Subject: [PATCH 8/9] ci: close the paths where the flake machinery failed open 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) --- .github/scripts/flake_report.py | 36 ++- .github/scripts/flake_summary.py | 81 +++++- .github/scripts/prepare_reports.sh | 6 +- .github/scripts/quarantine.py | 57 +++- .github/scripts/run_tests_with_retry.sh | 26 +- .../tests/test_generate_test_summary.sh | 21 ++ .github/scripts/tests/test_prepare_reports.sh | 52 ++++ .github/scripts/tests/test_quarantine.sh | 255 +++++++++++++++++- .github/workflows/ci.yml | 1 + .../profiler/test/ProfilerTestRunner.java | 18 +- 10 files changed, 505 insertions(+), 48 deletions(-) create mode 100755 .github/scripts/tests/test_prepare_reports.sh diff --git a/.github/scripts/flake_report.py b/.github/scripts/flake_report.py index d13f2d63ab..ece0e44388 100755 --- a/.github/scripts/flake_report.py +++ b/.github/scripts/flake_report.py @@ -252,18 +252,23 @@ def cmd_report(args): # exit code (a compile error or a dead # runner is nothing to do with # quarantine). - if args.evidence_suspect: - gates = True - gate_reason = ( - "flake evidence for this cell is suspect (the results directory " - "could not be reliably cleared between attempts), so a prior " - "attempt's results may be mistaken for the final attempt's own" - ) - elif gating: + if gating: gates = True gate_reason = "{} un-quarantined failure(s)".format(len(gating)) elif results: - if not final_attempt_ran or final_attempt_gating_count != 0: + # Suspect evidence belongs here, inside the branch that has something + # to excuse: it is a reason to distrust the quarantine excuse, not a + # test failure of its own. A run where everything passed but the + # results tree could not be cleared has nothing to excuse, so its own + # exit code stands. + if args.evidence_suspect: + gates = True + gate_reason = ( + "flake evidence for this cell is suspect (the results directory " + "could not be reliably cleared between attempts), so a prior " + "attempt's results may be mistaken for the final attempt's own" + ) + elif not final_attempt_ran or final_attempt_gating_count != 0: gates = True gate_reason = ( "the final attempt did not itself pass with only quarantined " @@ -300,8 +305,19 @@ def cmd_report(args): "cell": args.cell, "attempts": ran, "attempts_run": args.final_attempt, + # A single attempt cannot distinguish flaky from broken: passed_in is + # empty for every failure because nothing re-ran. Calling those + # "persistent" would assert something the run never measured, so they + # get their own bucket and their own proposals. "flaky": [r for r in results if r["flaky"] and not r["quarantined"]], - "persistent": [r for r in results if not r["flaky"] and not r["quarantined"]], + "persistent": [ + r for r in results + if not r["flaky"] and not r["quarantined"] and ran > 1 + ], + "unclassified": [ + r for r in results + if not r["flaky"] and not r["quarantined"] and ran <= 1 + ], "quarantined": [r for r in results if r["quarantined"]], "gating_count": len(gating), "failure_count": len(results), diff --git a/.github/scripts/flake_summary.py b/.github/scripts/flake_summary.py index 53f2d28fa6..f7429c15cf 100755 --- a/.github/scripts/flake_summary.py +++ b/.github/scripts/flake_summary.py @@ -31,11 +31,9 @@ # a fence-breaking ``` sequence into that render. _SAFE_TEST_ID_RE = re.compile(r"[^A-Za-z0-9_.$-]") -# The one place a failure message is truncated for display. flake_report.py -# stores messages at a wider cap (200 chars) for anyone reading the raw JSON; -# every renderer of this data (this module's tables, generate-test-summary.sh's -# per-job table) uses this same, narrower display width so the same failure -# does not render at two different lengths in one PR comment. +# The display width for a failure message in this module's tables. +# flake_report.py stores messages at a wider cap (200 chars) for anyone reading +# the raw JSON. MESSAGE_DISPLAY_WIDTH = 120 @@ -174,16 +172,53 @@ def cells_glob(cells): musl+aarch64 gets `*musl*aarch64*` rather than the wider `*aarch64*` (which would also cover glibc aarch64). - Cell names are `---[-slow]`. `*`-joining is - order-sensitive, so the axes here must be listed in that same left-to-right - order (libc, config, arch, suite suffix) -- axes out of order yields a glob - fnmatch can never match against the very cells it was derived from. + Cell names are `---[-slow]`, so the axes are read + positionally out of that grammar rather than matched against a hardcoded + token list. A list would silently omit whichever axis nobody thought of -- + the JDK (which the matrix varies along most) and the regular/slow suffix + were both missing, so a flake seen only on `glibc-8-j9-debug-amd64` + proposed `*glibc*debug*amd64*` and quarantined it on all 13 JDKs and on the + slow suite too. + + A field the cells disagree on becomes `*`; one they share is kept + literally. The suffix is an axis in its own right: a proposal derived from + regular cells ends in the arch so it cannot also match that cell's `-slow` + twin. """ - axes = ["glibc", "musl", "debug", "release", "asan", "tsan", "amd64", "aarch64", "slow"] - shared = [axis for axis in axes if all(axis in c for c in cells)] - if not shared: + fields = [c.split("-") for c in cells] + # A cell that does not parse as ---[-slow] (a jdk + # like "8-j9" makes that five or six fields) is not something to guess at. + widths = {len(f) for f in fields} + if len(widths) != 1: return None - return ["*" + "*".join(shared) + "*"] + width = widths.pop() + if width < 4: + return None + shared = [ + fields[0][i] if all(f[i] == fields[0][i] for f in fields) else "*" + for i in range(width) + ] + if all(part == "*" for part in shared): + return None + glob = "-".join(shared) + # Anchored at the end so a regular-suite proposal cannot match `-slow`; + # leading `*` only if the first field itself is unconstrained. + return [glob if shared[0] != "*" else "*" + glob.lstrip("*")] + + +def widened_note(test_id, pattern): + """A warning line when the proposed pattern covers more than was observed. + + covers() understands an exact id or a class-wide `.*` and nothing else, so + an id it cannot express exactly (a parameterized invocation index) can only + be proposed class-wide. That mutes every test in the class, which is a + different decision from the one the evidence supports -- so it is stated + rather than left for the reviewer to notice. + """ + if pattern.endswith(".*") and not test_id.endswith(".*"): + return ("# WIDENED: observed `{}`, which covers() cannot match exactly; " + "this entry mutes the whole class".format(test_id)) + return None def render_proposals(flaky, proposal_limit=25): @@ -214,8 +249,12 @@ def render_proposals(flaky, proposal_limit=25): info["message"] or "intermittent failure", ", ".join(sorted(set(info["cells"]))[:4]), )).replace("|", "/") + pattern = sanitize_quarantine_test_pattern(test_id) + note = widened_note(test_id, pattern) + if note: + out.append(note) out.append(quarantine.format_entry( - sanitize_quarantine_test_pattern(test_id), + pattern, "PROF-XXXXX", today.isoformat(), review_by, @@ -252,6 +291,7 @@ def main(): flaky = group_by_test(reports, "flaky") persistent = group_by_test(reports, "persistent") + unclassified = group_by_test(reports, "unclassified") quarantined = group_by_test(reports, "quarantined") out = [] @@ -276,6 +316,19 @@ def main(): out.append("") out.extend(render_table(persistent)) out.append("") + if unclassified: + out.append("### :grey_question: Failing tests — flakiness not measured") + out.append("") + out.extend(render_table(unclassified)) + out.append("") + out.append( + "**These fail the build.** The suite was not retried (slow suites run " + "once), so whether they are flaky or broken was never measured — the " + "entry below is offered on the same terms as a flake's, and the " + "judgement is still yours." + ) + out.append("") + out.extend(render_proposals(unclassified)) if quarantined: out.append("### :mute: Quarantined failures — not gating") out.append("") diff --git a/.github/scripts/prepare_reports.sh b/.github/scripts/prepare_reports.sh index e016a46da6..b64dd69c2a 100755 --- a/.github/scripts/prepare_reports.sh +++ b/.github/scripts/prepare_reports.sh @@ -20,7 +20,11 @@ cp -r ddprof-test/build/reports/tests test-reports/tests || true # on its first attempt (skipped as a needless copy with no other attempt to # compare against) -- copy build/test-results directly only then, so a green # run still ships its JUnit XML. -if [ -z "$(find flake-evidence -mindepth 1 -maxdepth 1 -name 'attempt-*' 2>/dev/null)" ]; then +# Keyed on real evidence rather than on the attempt-* directory: snapshot() +# creates that with mkdir -p *before* the copy that may fail, so an empty or +# partial snapshot would otherwise skip this fallback and ship an artifact with +# no JUnit XML in it at all. +if [ -z "$(find flake-evidence -name 'TEST-*.xml' 2>/dev/null | head -1)" ]; then cp -r ddprof-test/build/test-results test-reports/test-results || true fi cp -r flake-evidence test-reports/flake-evidence || true diff --git a/.github/scripts/quarantine.py b/.github/scripts/quarantine.py index 8011a0f19c..fb048073f1 100755 --- a/.github/scripts/quarantine.py +++ b/.github/scripts/quarantine.py @@ -55,10 +55,18 @@ # cell globs could both match the same real cell. Wide enough to catch a glob # written against any axis (jdk, config, the libc/arch pair, or the slow/regular # suite suffix) without having to enumerate the workflow's actual, ever-growing -# matrix. Deliberately over-inclusive (e.g. jdk variants like "8-j9" beyond the -# base list below): a synthetic cell that never occurs for real only makes -# overlap detection more conservative, never less. -_SYNTHETIC_JDKS = ("8", "8-graal", "11", "17", "17-graal", "21", "25") +# matrix. Over-inclusive on purpose: a synthetic cell that never occurs for +# real only makes overlap detection more conservative, never less. Under- +# inclusive is the dangerous direction -- cells_overlap() fails closed for a +# glob matching nothing synthetic, so a JDK variant missing from here makes two +# genuinely disjoint entries look like duplicates and fail validation. +_SYNTHETIC_JDK_BASES = ("8", "11", "17", "21", "25") +_SYNTHETIC_JDK_SUFFIXES = ("", "-orcl", "-j9", "-ibm", "-graal") +_SYNTHETIC_JDKS = tuple( + base + suffix + for base in _SYNTHETIC_JDK_BASES + for suffix in _SYNTHETIC_JDK_SUFFIXES +) _SYNTHETIC_CONFIGS = ("debug", "release", "asan", "tsan") _SYNTHETIC_SUITE_SUFFIXES = ("", "-slow") SYNTHETIC_CELLS = tuple( @@ -162,7 +170,15 @@ def covers(entry, test_id): pattern = entry["test"] test_id = normalise_test_id(test_id) if pattern.endswith(".*"): - return test_id.startswith(pattern[:-1]) + # "covers every method in the class", per quarantine.txt -- so the + # remainder after the class name must be a single method segment. An + # unbounded prefix match would make "com.datadoghq.profiler.*" suspend + # the merge gate for the entire repository from one validate-clean + # line. + prefix = pattern[:-1] + if not test_id.startswith(prefix): + return False + return "." not in test_id[len(prefix):] return test_id == pattern @@ -177,11 +193,15 @@ def is_expired(entry, today=None): """ review_by = entry.get("review_by", "") if not DATE_RE.match(review_by): - return False + # Blank or not a date at all. parse() accepts both, and validate() + # only runs in PR CI, so treating an unreadable expiry as "never + # expires" would let the one entry nobody can review outlive every + # entry that can be. An expiry that cannot be read has passed. + return True try: due = datetime.date.fromisoformat(review_by) except ValueError: - return False + return True return due < (today or datetime.date.today()) @@ -253,15 +273,26 @@ def complain(line, message): if entry[field] and not DATE_RE.match(entry[field]): complain(line, "{} '{}' is not YYYY-MM-DD".format(field, entry[field])) - if entry["test"] and BAD_TEST_WILDCARD_RE.search( - entry["test"][:-2] if entry["test"].endswith(".*") else entry["test"] - ): + stem = entry["test"][:-2] if entry["test"].endswith(".*") else entry["test"] + if entry["test"] and (not stem or BAD_TEST_WILDCARD_RE.search(stem)): complain(line, ( - "test pattern '{}' has a wildcard outside a single trailing " - "'.*'; covers() only understands an exact id or a class-wide " - "'.*', so this would silently quarantine nothing" + "test pattern '{}' is not an exact id or a class-wide '.*'; " + "covers() understands nothing else, so this would silently " + "quarantine nothing" ).format(entry["test"])) + # ".*" is not a class. covers() scopes a trailing '.*' to one + # class's methods, so a package-level pattern quarantines nothing -- + # while looking like it quarantines a great deal. + if entry["test"].endswith(".*") and stem: + last = stem.rsplit(".", 1)[-1] + if last and not last[:1].isupper(): + complain(line, ( + "test pattern '{}' reads as a package, not a class: a " + "trailing '.*' covers the methods of one class, so this " + "matches nothing. Name the class, or list its tests" + ).format(entry["test"])) + if entry["test"].endswith("()"): complain(line, ( "test '{}' carries the parentheses JUnit puts in its XML; " diff --git a/.github/scripts/run_tests_with_retry.sh b/.github/scripts/run_tests_with_retry.sh index dc289e192a..a788ab0536 100755 --- a/.github/scripts/run_tests_with_retry.sh +++ b/.github/scripts/run_tests_with_retry.sh @@ -38,6 +38,13 @@ fi CELL="${1:?usage: run_tests_with_retry.sh [--list ] -- }" shift [ "${1:-}" = "--" ] && shift +# Without a command the pipeline below degrades to a bare redirection, exits 0, +# records no attempt, and the cell goes green having run no test at all. Every +# other unexamined-pass path in this script fails loudly; so does this one. +if [ "$#" -eq 0 ]; then + echo "::error::no command given: run_tests_with_retry.sh [--list ] -- " + exit 1 +fi MAX_ATTEMPTS="${MAX_ATTEMPTS:-2}" MAX_FAILURES_TO_RETRY="${MAX_FAILURES_TO_RETRY:-3}" @@ -109,7 +116,15 @@ snapshot() { # so read access has to be taken again here or the cp below fails and the # cell silently loses its flake evidence. make_results_readable || EVIDENCE_SUSPECT=1 - rm -rf "$dest" || echo "::warning::Could not clear ${dest}; attempt ${attempt} evidence may be stale" + if ! rm -rf "$dest"; then + # cp -r below merges into whatever survives, so this attempt's evidence + # becomes a union with an earlier one's. attempt_results() then reports + # tests as observed-and-passed that this attempt never ran, which is how a + # persistent failure acquires a "flaky" label and a paste-ready quarantine + # entry that buries a real defect. + echo "::warning::Could not clear ${dest}; attempt ${attempt} evidence may be merged with an earlier attempt's" + EVIDENCE_SUSPECT=1 + fi mkdir -p "$dest" if [ -d "$RESULTS_DIR" ]; then cp -r "$RESULTS_DIR"/. "$dest"/ \ @@ -123,8 +138,13 @@ TEST_TASK_PATTERN="${TEST_TASK_PATTERN:-:ddprof-test:test}" # Self-contained state: a leftover attempt-2 from an earlier run on a reused # workspace would be read back as this run's evidence, inflating the attempt -# count and importing failures that never happened here. -rm -rf "$EVIDENCE_DIR" "$(dirname "$OUTCOME_FILE")" +# count and importing failures that never happened here. Root-owned leftovers +# from a Docker-run cell are exactly how that happens, so a failure to clear +# is recorded rather than discarded. +if ! rm -rf "$EVIDENCE_DIR" "$(dirname "$OUTCOME_FILE")"; then + echo "::warning::Could not clear ${EVIDENCE_DIR}; this run may inherit an earlier run's attempts as its own evidence" + EVIDENCE_SUSPECT=1 +fi EXIT_CODE=1 # A single, per-attempt-truncated log: only the final attempt's is ever read diff --git a/.github/scripts/tests/test_generate_test_summary.sh b/.github/scripts/tests/test_generate_test_summary.sh index 594d960ae3..e424dbf100 100755 --- a/.github/scripts/tests/test_generate_test_summary.sh +++ b/.github/scripts/tests/test_generate_test_summary.sh @@ -146,5 +146,26 @@ echo "$summary" | grep -q "All 1 test jobs passed" \ || fail "expected the all-passed banner, got: $summary" pass "an all-passing run prints no Failed Jobs section" +# The job-name regex and test_workflow.yml's `name:` expressions are a contract +# with no enforcement: a non-matching job is skipped silently, so a rename +# renders a summary reporting zero test jobs while the run stays green. Read +# the real workflow rather than a fabricated name, so drift fails here. +pattern=$(sed -n "s/.*test_job_pattern='\(.*\)'.*/\1/p" "$SCRIPT") +[ -n "$pattern" ] || fail "could not read test_job_pattern out of generate-test-summary.sh" +names=$(grep -hE "^ *name: test-linux" "$ROOT/.github/workflows/test_workflow.yml") +[ -n "$names" ] || fail "no test-linux job names found in test_workflow.yml" +count=0 +while IFS= read -r line; do + rendered=${line#*name: } + rendered=${rendered//'${{ matrix.java_version }}'/17} + rendered=${rendered//'${{ matrix.config }}'/debug} + rendered=${rendered//"\${{ inputs.slow_tests && 'slow' || 'regular' }}"/regular} + [[ "$rendered" =~ $pattern ]] \ + || fail "job name '$rendered' does not match generate-test-summary.sh's test_job_pattern" + count=$((count + 1)) +done <<< "$names" +[ "$count" -eq 4 ] || fail "expected 4 test-linux jobs in test_workflow.yml, matched $count" +pass "every test-linux job name in test_workflow.yml matches the summary's regex" + echo echo "All $TESTS generate-test-summary tests passed." diff --git a/.github/scripts/tests/test_prepare_reports.sh b/.github/scripts/tests/test_prepare_reports.sh new file mode 100755 index 0000000000..fe3d5c8ba4 --- /dev/null +++ b/.github/scripts/tests/test_prepare_reports.sh @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +# Hermetic checks for prepare_reports.sh's artifact staging. +# +# The one thing that must never happen quietly: shipping a test-reports +# artifact with no JUnit XML in it, which is what made every failed job render +# "No detailed failure information available". +set -uo pipefail + +ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")/../../.." && pwd) +SCRIPT="$ROOT/.github/scripts/prepare_reports.sh" +TESTS=0 +TEMP_DIR=$(mktemp -d) +trap 'rm -rf "$TEMP_DIR"' EXIT + +pass() { TESTS=$((TESTS + 1)); echo " ok: $1"; } +fail() { echo "FAIL: $1" >&2; exit 1; } + +write_xml() { + mkdir -p "$1" + cat > "$1/TEST-com.dd.Some.xml" <<'EOS' + + + + +EOS +} + +echo "== prepare_reports.sh stages JUnit XML ==" + +# snapshot() creates attempt- with mkdir -p *before* the copy that may fail, +# so an empty or partial snapshot must still fall back to build/test-results. +CASE="$TEMP_DIR/case-empty-snapshot" +mkdir -p "$CASE/flake-evidence/attempt-1" "$CASE/build/logs" +write_xml "$CASE/ddprof-test/build/test-results/testDebug" +( cd "$CASE" && "$SCRIPT" >/dev/null 2>&1 ) +[ -n "$(find "$CASE/test-reports" -name 'TEST-*.xml' 2>/dev/null | head -1)" ] \ + || fail "an empty attempt-* snapshot must still ship build/test-results, found none in test-reports" +pass "an empty snapshot falls back to build/test-results" + +# With real evidence in flake-evidence there is nothing to fall back to: the +# snapshot already holds what build/test-results holds. +CASE="$TEMP_DIR/case-real-snapshot" +mkdir -p "$CASE/build/logs" +write_xml "$CASE/flake-evidence/attempt-1" +write_xml "$CASE/ddprof-test/build/test-results/testDebug" +( cd "$CASE" && "$SCRIPT" >/dev/null 2>&1 ) +[ -n "$(find "$CASE/test-reports" -name 'TEST-*.xml' 2>/dev/null | head -1)" ] \ + || fail "a populated snapshot must still reach the artifact" +pass "a populated snapshot is shipped as-is" + +echo +echo "All $TESTS prepare-reports tests passed." diff --git a/.github/scripts/tests/test_quarantine.sh b/.github/scripts/tests/test_quarantine.sh index 6faf4b4401..b3adaf2e02 100755 --- a/.github/scripts/tests/test_quarantine.sh +++ b/.github/scripts/tests/test_quarantine.sh @@ -214,7 +214,7 @@ python3 -c " import json,sys d = json.load(open(sys.argv[1])) assert d['gating_count'] == 0 and d['failure_count'] == 0, d -assert not d['flaky'] and not d['persistent'], d +assert not d['flaky'] and not d['persistent'] and not d['unclassified'], d " "$CASE/ci-outcome/glibc-17-debug-amd64.json" || fail "a clean run was not reported as clean" pass "a suite that passes on the first attempt is green and reports no failures" @@ -521,9 +521,43 @@ output=$(cd "$CASE" && PATH="$CASE/stub-bin:$PATH" "$SCRIPTS/run_tests_with_retr rc=$? set -e [ "$rc" -ne 0 ] || fail "a failed snapshot copy must not be excused even though the named failure is quarantined (got exit $rc)" -echo "$output" | grep -q "suspect" \ - || fail "expected the suspect evidence to be named as the reason, got: $output" -pass "a failed/partial snapshot copy sets EVIDENCE_SUSPECT and forces gating" +echo "$output" | grep -q "Could not snapshot" \ + || fail "expected the lost evidence to be reported, got: $output" +pass "a failed snapshot copy leaves nothing to excuse, so the command's own failure stands" + +# Suspect evidence is a reason to distrust a quarantine excuse, not a failure +# of its own. A suite that passed has nothing to excuse, so an unreadable +# results tree must not turn its exit 0 into a red job. +CASE="$TEMP_DIR/case-suspect-evidence-all-passed" +mkdir -p "$CASE/stub-bin" +cat > "$CASE/stub-bin/find" <<'EOS' +#!/usr/bin/env bash +for a in "$@"; do + if [ "$a" = "-user" ]; then echo "/root-owned/TEST-Foo.xml"; exit 0; fi +done +exec /usr/bin/find "$@" +EOS +cat > "$CASE/stub-bin/sudo" <<'EOS' +#!/usr/bin/env bash +exit 1 +EOS +chmod +x "$CASE/stub-bin/find" "$CASE/stub-bin/sudo" +cat > "$CASE/suite.sh" <&1) +rc=$? +set -e +[ "$rc" -eq 0 ] || fail "suspect evidence must not fail a run in which every test passed (got exit $rc): $output" +pass "suspect evidence does not redden a run with nothing to excuse" # A test missing from the retry never re-ran, so it is not evidence of a flake. CASE="$TEMP_DIR/case-absent-is-not-passed" @@ -567,7 +601,7 @@ python3 -c " import json,sys d = json.load(open(sys.argv[1])) assert d['failure_count'] == 1, d -assert d['persistent'] and d['persistent'][0]['test'] == 'com.dd.Weird.realFailure', d +assert d['unclassified'] and d['unclassified'][0]['test'] == 'com.dd.Weird.realFailure', d " "$CASE/out.json" || fail "the unnamed testcase was not ignored, or the real failure was missed" pass "a testcase with no name attribute is ignored, not mistaken for a failure" @@ -586,7 +620,7 @@ python3 -c " import json,sys d = json.load(open(sys.argv[1])) assert d['attempts'] == 1, d -assert d['persistent'] and d['persistent'][0]['test'] == 'com.dd.WobblyTest.sometimesFails', d +assert d['unclassified'] and d['unclassified'][0]['test'] == 'com.dd.WobblyTest.sometimesFails', d " "$CASE/out.json" || fail "attempt-1 was not read back despite the stray attempt-tmp" pass "attempt-1 is still read as evidence while the stray directory is skipped" @@ -708,6 +742,215 @@ assert idx == 'com.dd.WobblyTest.*', idx " || fail "the proposal for a method() id was not the documented form, or an indexed invocation was not widened class-wide" pass "a method() id proposes the documented form; an indexed invocation proposes the class" +# The other-task guard, asserted on the verdict rather than on stdout: the +# previous check grepped for a task name the fixture echoes itself, which +# filter_gradle_log.py passes through verbatim, so deleting the guard left the +# test green. Exit 0 from the fixture isolates this from the exit-code branch. +CASE="$TEMP_DIR/case-other-task-guard" +mkdir -p "$CASE/flake-evidence/attempt-1" +write_failure_xml "$CASE/flake-evidence/attempt-1" "com.dd.WobblyTest" "sometimesFails" "boom" +cat > "$CASE/attempt.log" <<'EOS' +> Task :ddprof-lib:verifyNative FAILED +Execution failed for task ':ddprof-lib:verifyNative'. +EOS +write_list "$CASE/list.txt" "$(entry com.dd.WobblyTest.sometimesFails PROF-1 "$(day_offset 30)")" +python3 "$SCRIPTS/flake_report.py" --list "$CASE/list.txt" report \ + --cell "glibc-17-debug-amd64" --evidence-dir "$CASE/flake-evidence" \ + --final-attempt 1 --attempt-log "$CASE/attempt.log" \ + --final-attempt-exit-code 0 --test-task-pattern ":ddprof-test:test" \ + --out "$CASE/out.json" >/dev/null 2>&1 +python3 -c " +import json,sys +d = json.load(open(sys.argv[1])) +assert d['gates'] is True, 'a non-test task failure must gate even with every test quarantined: %r' % d['gate_reason'] +assert 'the build also failed in' in (d['gate_reason'] or ''), d['gate_reason'] +assert d['other_task_failures'] == [':ddprof-lib:verifyNative'], d['other_task_failures'] +" "$CASE/out.json" || fail "the non-test-task guard did not gate, or did not name the task" +pass "a build failure outside the test task is never excused by the list" + +# b-12, hermetically: suspect evidence with no failure recorded has nothing to +# excuse, so the classifier must hold no opinion and let the command's own exit +# code stand. +CASE="$TEMP_DIR/case-suspect-no-failures" +mkdir -p "$CASE/flake-evidence/attempt-1" +write_pass_xml "$CASE/flake-evidence/attempt-1" "com.dd.SteadyTest" "alwaysPasses" +write_list "$CASE/list.txt" +python3 "$SCRIPTS/flake_report.py" --list "$CASE/list.txt" report \ + --cell "glibc-17-debug-amd64" --evidence-dir "$CASE/flake-evidence" \ + --final-attempt 1 --evidence-suspect --out "$CASE/out.json" >/dev/null 2>&1 +python3 -c " +import json,sys +d = json.load(open(sys.argv[1])) +assert d['gates'] is None, 'suspect evidence with no failure must not gate: %r' % d['gate_reason'] +" "$CASE/out.json" || fail "suspect evidence gated a run in which nothing failed" +pass "suspect evidence holds no opinion when there is no failure to excuse" + +# s-22: a single attempt cannot tell flaky from broken, so the failure must not +# also appear as persistent -- that label asserts a measurement never made. +CASE="$TEMP_DIR/case-single-attempt-not-persistent" +mkdir -p "$CASE/flake-evidence/attempt-1" +write_failure_xml "$CASE/flake-evidence/attempt-1" "com.dd.SlowTest" "onlyRunOnce" "boom" +write_list "$CASE/list.txt" +python3 "$SCRIPTS/flake_report.py" --list "$CASE/list.txt" report \ + --cell "glibc-17-debug-amd64-slow" --evidence-dir "$CASE/flake-evidence" \ + --final-attempt 1 --out "$CASE/out.json" >/dev/null 2>&1 +python3 -c " +import json,sys +d = json.load(open(sys.argv[1])) +assert not d['persistent'], 'a single-attempt failure must not be labelled persistent: %r' % d['persistent'] +assert len(d['unclassified']) == 1, d +" "$CASE/out.json" || fail "a single-attempt failure was labelled broken rather than unclassified" +pass "a single-attempt failure is unclassified, not persistent" + +# s-19, through the renderer: the WIDENED marker has to reach the rendered +# proposal, not merely exist as a helper. +python3 -c " +import sys +sys.path.insert(0, '$SCRIPTS') +from flake_summary import render_proposals +out = '\n'.join(render_proposals({'com.dd.WobblyTest.[1]': {'message': 'boom', 'cells': ['glibc-17-debug-amd64']}})) +assert 'WIDENED' in out, 'the rendered proposal does not mark the widening:\n' + out +assert 'com.dd.WobblyTest.*' in out, out +" || fail "a class-wide proposal was rendered without its WIDENED marker" +pass "the rendered proposal carries the WIDENED marker" + +# g-14: no command at all must fail loudly rather than record a green cell that +# ran nothing. +CASE="$TEMP_DIR/case-no-command" +mkdir -p "$CASE" +write_list "$CASE/list.txt" +set +e +output=$(cd "$CASE" && "$SCRIPTS/run_tests_with_retry.sh" --list list.txt "glibc-17-debug-amd64" -- 2>&1) +rc=$? +set -e +[ "$rc" -ne 0 ] || fail "an empty command list must not exit 0 (got $rc): $output" +echo "$output" | grep -q "no command given" \ + || fail "expected an explicit error about the missing command, got: $output" +pass "an empty command list fails loudly instead of passing without running a test" + +# s-9: a destination that cannot be cleared makes cp merge stale evidence into +# this attempt's, which is how a persistent failure acquires a flaky label. +CASE="$TEMP_DIR/case-dest-clear-fails" +mkdir -p "$CASE/stub-bin" +cat > "$CASE/stub-bin/rm" <<'EOS' +#!/usr/bin/env bash +for a in "$@"; do + case "$a" in *flake-evidence/attempt-*) exit 1 ;; esac +done +exec /bin/rm "$@" +EOS +chmod +x "$CASE/stub-bin/rm" +cat > "$CASE/suite.sh" <&1) +rc=$? +set -e +[ "$rc" -ne 0 ] || fail "evidence that may be merged with an earlier attempt's must not be excused (got exit $rc): $output" +echo "$output" | grep -q "merged with an earlier attempt" \ + || fail "expected the merged-evidence warning, got: $output" +pass "a destination that cannot be cleared poisons the evidence rather than being excused" + +echo "== quarantine.py fails closed ==" + +# An unreadable review_by must stop excusing rather than excuse forever: +# parse() accepts a blank or non-date value, and validate() only runs in PR CI. +for bad in "" "not-a-date" "2026-13-45"; do + python3 -c " +import sys +sys.path.insert(0, '$SCRIPTS') +import quarantine +entry = {'test': 'a.B.c', 'ticket': 'PROF-1', 'added': '2026-01-01', + 'review_by': '$bad', 'cells': [], '_line': 1} +assert quarantine.is_expired(entry), 'review_by %r must count as expired' % '$bad' +assert quarantine.find_entry([entry], 'a.B.c', 'glibc-17-debug-amd64') is None, \ + 'an entry with an unreadable review_by must not excuse anything' +" || fail "an unreadable review_by (${bad:-}) was treated as never expiring" +done +pass "an unreadable review_by counts as expired, not as eternal" + +# A trailing '.*' covers one class's methods, as documented -- not a package. +python3 -c " +import sys +sys.path.insert(0, '$SCRIPTS') +import quarantine +pkg = {'test': 'com.datadoghq.profiler.*'} +assert not quarantine.covers(pkg, 'com.datadoghq.profiler.cpu.CpuTest.sampling'), \ + 'a package-level pattern must not quarantine a whole subtree' +cls = {'test': 'com.datadoghq.profiler.cpu.CpuTest.*'} +assert quarantine.covers(cls, 'com.datadoghq.profiler.cpu.CpuTest.sampling') +assert quarantine.covers(cls, 'com.datadoghq.profiler.cpu.CpuTest.sampling()') +assert not quarantine.covers(cls, 'com.datadoghq.profiler.cpu.CpuTest.Inner.sampling') +" || fail "a trailing '.*' is not scoped to one class's methods" +pass "a trailing '.*' covers one class's methods, not a package" + +write_list "$LIST" "$(entry 'com.datadoghq.profiler.*' PROF-1 "$(day_offset 30)")" +if python3 "$SCRIPTS/quarantine.py" --list "$LIST" validate >/dev/null 2>&1; then + fail "a package-level '.*' pattern should be rejected" +fi +pass "a package-level '.*' pattern is rejected" + +write_list "$LIST" "$(entry '.*' PROF-1 "$(day_offset 30)")" +if python3 "$SCRIPTS/quarantine.py" --list "$LIST" validate >/dev/null 2>&1; then + fail "a bare '.*' pattern should be rejected" +fi +pass "a bare '.*' pattern is rejected" + +# cells_overlap() fails closed for a glob matching nothing synthetic, so a JDK +# variant missing from the synthetic universe makes disjoint entries look like +# duplicates. +write_list "$LIST" \ + "$(entry a.B.c PROF-1 "$(day_offset 30)" '*17-j9*')" \ + "$(entry a.B.c PROF-2 "$(day_offset 30)" '*21-graal*')" +python3 "$SCRIPTS/quarantine.py" --list "$LIST" validate >/dev/null 2>&1 \ + || fail "two entries on genuinely disjoint JDK variants must validate" +pass "disjoint JDK-variant cells are not mistaken for duplicates" + +echo "== cells_glob narrows on every axis ==" + +python3 -c " +import sys +sys.path.insert(0, '$SCRIPTS') +import fnmatch +from flake_summary import cells_glob +# One cell: every axis is shared, so nothing else may match. +g = cells_glob(['glibc-8-j9-debug-amd64'])[0] +assert fnmatch.fnmatch('glibc-8-j9-debug-amd64', g), g +assert not fnmatch.fnmatch('glibc-17-debug-amd64', g), 'JDK axis not narrowed: %s' % g +# Differing only in JDK: that field opens up, the rest stays put. +g = cells_glob(['glibc-8-debug-amd64', 'glibc-17-debug-amd64'])[0] +assert fnmatch.fnmatch('glibc-8-debug-amd64', g) and fnmatch.fnmatch('glibc-17-debug-amd64', g), g +assert not fnmatch.fnmatch('musl-8-debug-amd64', g), 'libc axis not narrowed: %s' % g +assert not fnmatch.fnmatch('glibc-8-release-amd64', g), 'config axis not narrowed: %s' % g +assert not fnmatch.fnmatch('glibc-8-debug-aarch64', g), 'arch axis not narrowed: %s' % g +assert not fnmatch.fnmatch('glibc-8-debug-amd64-slow', g), 'suite suffix not narrowed: %s' % g +" || fail "cells_glob does not narrow on the JDK or the slow/regular axis" +pass "a proposal excludes cells differing only in JDK or in the slow suffix" + +echo "== a widened proposal says so ==" + +python3 -c " +import sys +sys.path.insert(0, '$SCRIPTS') +from flake_summary import widened_note, sanitize_quarantine_test_pattern +idx = 'com.dd.WobblyTest.[1]' +pat = sanitize_quarantine_test_pattern(idx) +assert pat == 'com.dd.WobblyTest.*', pat +note = widened_note(idx, pat) +assert note and 'WIDENED' in note and idx in note, note +exact = 'com.dd.WobblyTest.sometimesFails()' +assert widened_note(exact, sanitize_quarantine_test_pattern(exact)) is None +" || fail "a class-wide proposal for an inexpressible id carries no warning" +pass "a proposal widened to the class is marked as widened" + echo "== validate rejects unmatchable test patterns ==" write_list "$LIST" "$(entry 'com.dd.WobblyTest.sometimesFails()' PROF-1 "$(day_offset 30)")" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 69990a911e..39d79e4f36 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -67,6 +67,7 @@ jobs: python3 .github/scripts/quarantine.py validate .github/scripts/tests/test_quarantine.sh .github/scripts/tests/test_generate_test_summary.sh + .github/scripts/tests/test_prepare_reports.sh check-for-pr: runs-on: ubuntu-latest diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/test/ProfilerTestRunner.java b/ddprof-test/src/test/java/com/datadoghq/profiler/test/ProfilerTestRunner.java index 29376b2e24..d03991d52a 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/test/ProfilerTestRunner.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/test/ProfilerTestRunner.java @@ -20,7 +20,9 @@ import org.junit.platform.launcher.listeners.SummaryGeneratingListener; import org.junit.platform.reporting.legacy.xml.LegacyXmlReportGeneratingListener; +import java.io.IOException; import java.io.PrintWriter; +import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; import java.util.Optional; @@ -146,7 +148,21 @@ private static void runTests() { String reportsDir = System.getProperty("test.reportsDir"); if (reportsDir != null && !reportsDir.isEmpty()) { Path reportsPath = Paths.get(reportsDir); - reportsPath.toFile().mkdirs(); + try { + Files.createDirectories(reportsPath); + } catch (IOException e) { + // Silently producing no XML is the one outcome that must not + // happen: flake_report.py would read zero observed tests for + // this cell, which is indistinguishable from a cell that was + // never retried, so a failure here would disable flake + // classification and quarantine for musl without a trace. + System.err.println("::error::cannot write JUnit reports to " + reportsPath + ": " + e); + System.exit(1); + } + if (!Files.isDirectory(reportsPath)) { + System.err.println("::error::JUnit report directory is not a directory: " + reportsPath); + System.exit(1); + } launcher.registerTestExecutionListeners(new LegacyXmlReportGeneratingListener(reportsPath, new PrintWriter(System.err))); } From 5f01c39cc06c5b1f1a653b4a53a82e9b4f32cf98 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Tue, 15 Sep 2026 13:54:07 +0200 Subject: [PATCH 9/9] ci: ignore the CI scripts' bytecode 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) --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index ee257fca7c..38ff268849 100644 --- a/.gitignore +++ b/.gitignore @@ -45,6 +45,9 @@ doc/temp/ /flake-evidence/ /ci-outcome/ +# Bytecode from the CI scripts, left by running them or their tests locally +__pycache__/ + # CLAUDE.md is auto-generated from AGENTS.md bootstrap instructions CLAUDE.md