From 85ef1742733045b6c72347d2f0525fb8ee310bbd Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Wed, 2 Sep 2026 20:42:45 +0200 Subject: [PATCH] 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