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