diff --git a/build/RetryLedger.cs b/build/RetryLedger.cs index 63a444b9c..342c1764e 100644 --- a/build/RetryLedger.cs +++ b/build/RetryLedger.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.IO; using System.Linq; using System.Text; @@ -68,7 +68,8 @@ void recordLedger(string projectName, string framework, SupervisorResults result AbortReason = results.AbortReason, // Bounded by the budget itself: the point of the list is to name the suspects, not to // reproduce the log. - FlakyTests = results.PassedOnRetry.Select(x => x.DisplayName).Take(MaxRetriesPerRun).ToArray() + FlakyTests = results.PassedOnRetry.Select(x => x.DisplayName).Take(MaxRetriesPerRun).ToArray(), + FlakyFailures = results.PassedOnRetry.Take(MaxRetriesPerRun).Select(firstFailure).ToArray() }; writeLedgerFile(entry); @@ -76,6 +77,63 @@ void recordLedger(string projectName, string framework, SupervisorResults result annotate(entry); } + /// + /// Why a retried test failed the first time. + /// + /// The ledger named the flaky test but threw away the reason, and Bobcat's own log keeps + /// only the [FLAKY] ... passed on attempt 2 line for a test that eventually passed — the + /// first attempt's failure appears nowhere at all. That is the difference between a lead and a + /// name. Two of the four flaky tests standing on 2026-08-03 were undiagnosable for exactly this + /// reason, and the note left on multi_tenancy_through_virtual_hosts records its next step + /// as "dump the tracked session on the FIRST attempt rather than infer from the assertion" — + /// which is this. GH-3763, GH-3787. + /// + /// Takes the earliest attempt that did not succeed rather than Attempts[0]: the + /// ordering of the collection is Bobcat's business, and AttemptNumber is the field that + /// actually states it. + /// + /// The reason comes from the attempt's Outcome — the test's own error — and + /// deliberately not from Disposition.Reason, which is the supervisor's rationale + /// for retrying ("a failure is retried in a fresh process, within the budget, to separate flaky + /// from broken") and is the same sentence for every retry in the repository. That distinction is + /// the whole point of this field: one of them identifies the test's problem, the other only + /// restates the retry policy. + /// + static FlakyFailure firstFailure(TestReport report) + { + var failed = report.Attempts? + .Where(x => !x.Succeeded) + .OrderBy(x => x.AttemptNumber) + .FirstOrDefault(); + + var outcome = failed?.Outcome; + + return new FlakyFailure + { + Test = report.DisplayName, + Attempt = failed?.AttemptNumber ?? 0, + ErrorType = string.IsNullOrWhiteSpace(outcome?.ErrorType) ? null : outcome.ErrorType, + // One line and bounded: this is a lead to open the log with, not a copy of it. A + // multi-line reason would also break the ::warning workflow command downstream. + Reason = condense(outcome?.ErrorMessage) + }; + } + + /// + /// Collapses a failure reason to a single bounded line, or null when there is nothing to say. + /// + static string condense(string reason) + { + if (string.IsNullOrWhiteSpace(reason)) return null; + + var flattened = string.Join(" ", reason.Split(['\r', '\n'], StringSplitOptions.RemoveEmptyEntries) + .Select(x => x.Trim()) + .Where(x => x.Length > 0)); + + const int limit = 400; + return flattened.Length <= limit ? flattened : flattened[..limit] + "…"; + } + static void writeLedgerFile(LedgerEntry entry) { try @@ -133,7 +191,22 @@ static void appendStepSummary(LedgerEntry entry) builder.AppendLine(); builder.AppendLine("
Tests that only passed on a retry"); builder.AppendLine(); - foreach (var test in entry.FlakyTests) builder.AppendLine($"- `{test}`"); + + // Keyed off FlakyFailures where it has something to add, so the reason sits with the + // name rather than in a log nobody opens. + var reasons = entry.FlakyFailures.ToDictionary(x => x.Test, x => x); + foreach (var test in entry.FlakyTests) + { + builder.AppendLine($"- `{test}`"); + + if (!reasons.TryGetValue(test, out var failure)) continue; + if (failure.ErrorType is null && failure.Reason is null) continue; + + var label = failure.ErrorType is null ? "" : $"**{failure.ErrorType}**"; + var detail = failure.Reason is null ? "" : $" — {failure.Reason}"; + builder.AppendLine($" - attempt {failure.Attempt}: {label}{detail}"); + } + builder.AppendLine(); builder.AppendLine("
"); } @@ -194,5 +267,28 @@ class LedgerEntry public int WorkerFaults { get; init; } public string AbortReason { get; init; } public string[] FlakyTests { get; init; } = []; + + /// + /// The same tests as , each with the reason its first attempt failed. + /// Additive on purpose: stays a plain string array because + /// build/flakiness-report.sh flattens it with map(.FlakyTests[]), and that jq must keep + /// working against ledgers written by older runs. + /// + public FlakyFailure[] FlakyFailures { get; init; } = []; + } + + /// + /// One retried test and why it failed before it passed. See . + /// + class FlakyFailure + { + public string Test { get; init; } + public int Attempt { get; init; } + + /// Exception type name from the failing attempt, e.g. ShouldAssertException. + public string ErrorType { get; init; } + + /// The failing attempt's own error message, flattened to one bounded line. + public string Reason { get; init; } } } diff --git a/build/flakiness-report.sh b/build/flakiness-report.sh index aa973a55f..588db1eec 100755 --- a/build/flakiness-report.sh +++ b/build/flakiness-report.sh @@ -52,6 +52,9 @@ if [ "$(jq 'length' "${output_dir}/entries.json")" != "0" ]; then fi fi +# flakyFailures carries the first-attempt failure reason alongside the name. The `// []` matters: +# ledgers written before that field existed do not have it, and the baseline is downloaded from an +# EARLIER run's artifact, so without the default one older baseline nulls the whole roll-up. jq '[group_by(.Job)[] | { job: .[0].Job, tests: (map(.Tests) | add), @@ -59,7 +62,8 @@ jq '[group_by(.Job)[] | { passedOnRetry: (map(.PassedOnRetry) | add), failed: (map(.Failed) | add), indeterminate: (map(.Indeterminate) | add), - flakyTests: (map(.FlakyTests[]) | unique) + flakyTests: (map(.FlakyTests[]) | unique), + flakyFailures: (map(.FlakyFailures // [] | .[]) | unique) }] | sort_by(-.retries, .job)' \ "${output_dir}/entries.json" > "${output_dir}/aggregate.json" @@ -194,7 +198,18 @@ delta_for() { if [ "${total_retries}" != "0" ]; then echo "
Tests that only passed on a retry" echo - jq -r '.[] | select(.retries > 0) | "- **\(.job)**", (.flakyTests[] | " - `\(.)`")' \ + # Each test is followed by why its first attempt failed, when the ledger recorded one. A name + # alone says which test to look at; the reason is what makes it possible to act without + # reproducing the whole job. + jq -r ' + def reason_for($t): [.flakyFailures[]? | select(.Test == $t)] | first; + .[] | select(.retries > 0) + | "- **\(.job)**", + (.flakyTests[] as $t + | " - `\($t)`", + (reason_for($t) + | select(. != null and ((.ErrorType // .Reason) != null)) + | " - attempt \(.Attempt): \(.ErrorType // "")\(if .Reason then " — \(.Reason)" else "" end)"))' \ "${output_dir}/aggregate.json" echo echo "
"