From 611463de39f66884aec408faed9c6e681947ef8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Thu, 30 Jul 2026 12:31:37 +0200 Subject: [PATCH 01/11] Align CTRF retry reporting with ctrf-io/ctrf#58 The CTRF maintainers confirmed the retry model our engine already emits: `retryAttempts[]` is the attempt history *preceding* the final attempt (attempts 1..N-1, initial execution included), the final attempt is excluded because its outcome is the test object, and `retries` equals `retryAttempts.length` so the final attempt is `retries + 1`. Documented that confirmation where the shape is produced; no behavior change. They also answered the cross-process (`--retry-failed-tests`) question: the per-attempt documents and any document merged from them describe the same logical run and should share a `runId` while each keeps its own `reportId`, and the merged test object should carry the final attempt's `duration`. - Emit `runId` (CTRF 5.4), resolved from a shared logical-run id (`TESTINGPLATFORM_LOGICAL_RUN_ID`, then the `dotnet test` execution id, then a fresh id) so every process of one run agrees on it. - Seed that variable in `RetryOrchestrator` so all attempts inherit it, preserving an id an outer orchestrator already set. - `CtrfReportMerger` carries a `runId` over when every input agrees, and still derives its own `reportId`. - Add an opt-in `CtrfMergeMode.CollapseRetryAttempts` merge mode: last attempt wins, earlier attempts (including in-process ones already nested by an input) flatten into `retryAttempts[]` renumbered 1..N-1, `retries`/`flaky` are recomputed, and the row keeps the final attempt's duration. Concatenation stays the default because MTP UIDs are only unique within an assembly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ad2ea8a-3349-4979-b27c-dd52bb3b7190 --- .../CtrfMergeMode.cs | 25 ++ .../CtrfReportEngine.JsonSerializer.cs | 27 ++ .../CtrfReportEngine.JsonTestWriter.cs | 3 + .../CtrfReportEngine.TestCollapsing.cs | 5 + .../CtrfReportMerger.cs | 282 +++++++++++++++++- .../InternalAPI/InternalAPI.Unshipped.txt | 5 + ...osoft.Testing.Extensions.CtrfReport.csproj | 1 + .../RetryOrchestrator.cs | 10 + .../Helpers/EnvironmentVariableConstants.cs | 6 + .../CtrfReportTests.cs | 6 + .../CtrfReportEngineTests.cs | 47 +++ .../CtrfReportMergerTests.cs | 251 ++++++++++++++++ 12 files changed, 657 insertions(+), 11 deletions(-) create mode 100644 src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfMergeMode.cs diff --git a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfMergeMode.cs b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfMergeMode.cs new file mode 100644 index 0000000000..4f33704d09 --- /dev/null +++ b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfMergeMode.cs @@ -0,0 +1,25 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace Microsoft.Testing.Extensions.CtrfReport; + +/// +/// Controls how combines the tests[] arrays of its inputs. +/// +internal enum CtrfMergeMode +{ + /// + /// Concatenates the inputs, which is correct when they describe disjoint sets of tests (the shard or + /// per-module case). This is the default: MTP test UIDs are only unique WITHIN an assembly, so collapsing + /// by identity across modules would fuse same-named tests from different assemblies. + /// + Concatenate, + + /// + /// Folds rows describing the same logical test into one, which is correct when the inputs are successive + /// attempts of the same test module (--retry-failed-tests): the last attempt wins and earlier ones + /// become its retryAttempts[]. Inputs MUST be supplied in attempt order, and MUST come from the same + /// module for identities to be comparable. + /// + CollapseRetryAttempts, +} diff --git a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.JsonSerializer.cs b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.JsonSerializer.cs index 63d59b36cf..35f49fe61c 100644 --- a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.JsonSerializer.cs +++ b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.JsonSerializer.cs @@ -4,6 +4,7 @@ using System.Text.Json; using Microsoft.Testing.Platform; +using Microsoft.Testing.Platform.Helpers; namespace Microsoft.Testing.Extensions.CtrfReport; @@ -68,6 +69,12 @@ private byte[] BuildCtrfJson(CapturedTestResult[] results, DateTimeOffset finish // Bump this constant whenever we update against a newer schema revision. writer.WriteString("specVersion", CtrfSpecVersion); writer.WriteString("reportId", Guid.NewGuid().ToString("D")); + // CTRF 5.4 (`runId`): identifies the logical run this document belongs to. A logical run can span + // several documents — the modules of one `dotnet test` invocation, or the successive processes of + // `--retry-failed-tests`, where each attempt writes its own document. ctrf-io/ctrf#58 confirmed that + // those per-execution documents (and any document merged from them) SHOULD share a `runId` while each + // keeps its own `reportId`. When nothing correlated this process, it is the whole logical run. + writer.WriteString("runId", ResolveRunId()); writer.WriteString("timestamp", finishTime.ToString("O", CultureInfo.InvariantCulture)); writer.WriteString( "generatedBy", @@ -152,4 +159,24 @@ private byte[] BuildCtrfJson(CapturedTestResult[] results, DateTimeOffset finish return ms.ToArray(); } + + /// + /// Resolves the CTRF runId: the id of the logical run this document belongs to. + /// + /// + /// The retry orchestrator seeds TESTINGPLATFORM_LOGICAL_RUN_ID before launching its attempts, so every + /// attempt process stamps the same value; dotnet test already correlates the modules of one invocation + /// through its execution id, which serves the same purpose when no orchestrator is involved. Falling back to a + /// fresh id keeps the field a valid non-empty string for a standalone run, which is its own logical run. + /// + private string ResolveRunId() + { + string? runId = _environment.GetEnvironmentVariable(EnvironmentVariableConstants.TESTINGPLATFORM_LOGICAL_RUN_ID); + if (RoslynString.IsNullOrEmpty(runId)) + { + runId = _environment.GetEnvironmentVariable(EnvironmentVariableConstants.TESTINGPLATFORM_DOTNETTEST_EXECUTIONID); + } + + return RoslynString.IsNullOrEmpty(runId) ? Guid.NewGuid().ToString("D") : runId; + } } diff --git a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.JsonTestWriter.cs b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.JsonTestWriter.cs index 4e38a72332..fdd706f33c 100644 --- a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.JsonTestWriter.cs +++ b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.JsonTestWriter.cs @@ -77,6 +77,9 @@ private static void WriteTest(Utf8JsonWriter writer, CollapsedTestResult c) if (c.PriorAttempts.Count > 0) { + // CTRF 9.20/9.21 as clarified by ctrf-io/ctrf#58: `retries` is the number of + // re-executions, which equals `retryAttempts.length` because the array holds + // attempts 1..N-1 only — so the final attempt's number is `retries + 1`. writer.WriteNumber("retries", c.PriorAttempts.Count); writer.WritePropertyName("retryAttempts"); writer.WriteStartArray(); diff --git a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.TestCollapsing.cs b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.TestCollapsing.cs index 7de772747a..ad54060c00 100644 --- a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.TestCollapsing.cs +++ b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.TestCollapsing.cs @@ -56,6 +56,11 @@ private static List CollapseAttempts(CapturedTestResult[] r // For each UID, group all captures in arrival order: the latest entry becomes the // final test record, earlier entries become `retryAttempts[]`. Preserves the // insertion order of first-seen UIDs in the output (stable across runs). + // + // ctrf-io/ctrf#58 confirmed this is the intended CTRF model: `retryAttempts[]` + // is the attempt history PRECEDING the final attempt (attempts 1..N-1, initial + // execution included), and the final attempt is excluded because its outcome and + // diagnostics are carried by the test object itself. var byUid = new Dictionary(StringComparer.Ordinal); var collapsed = new List(results.Length); foreach (CapturedTestResult r in results) diff --git a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs index e38a870766..2ec881dbff 100644 --- a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs +++ b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs @@ -15,9 +15,10 @@ namespace Microsoft.Testing.Extensions.CtrfReport; /// This is a pure, invocation-agnostic JSON-level merge (no I/O, no clock) that mirrors the /// TRX and JUnit mergers, demonstrating that the same post-processing shape fits a JSON format: /// -/// results.tests[] arrays are concatenated as-is. +/// results.tests[] arrays are concatenated as-is, unless asks for successive attempts of the same test to be folded into one row. /// results.summary counters are re-derived by counting the merged tests[] (so summary.tests always matches the array length); start/stop use the earliest/latest across inputs, duration is the resulting span. /// reportFormat and specVersion are taken from the first report; reportId is derived deterministically from the inputs, so identical inputs reproduce the same id (RFC 018 idempotency). +/// runId is carried over when every input agrees on one, because the merged document describes the same logical run as its inputs while remaining a distinct artifact with its own reportId (see ctrf-io/ctrf#58). /// tool keeps a concrete identity only when every input reported the exact same tool object; otherwise (inputs disagree or any input omits it) a neutral merger identity is used, so one framework is not attributed to another's tests. /// environment keeps the first report's shared fields, but module-specific values under extra (testApplication, exitCode) are dropped rather than presented as describing all merged modules. /// @@ -32,7 +33,28 @@ internal static class CtrfReportMerger // not by any input report. private const string GeneratedByName = "Microsoft.Testing.Extensions.CtrfReport"; + // Fields a CTRF retry attempt object (section 11) shares with a test object and that carry over verbatim when + // a non-final attempt is folded into 'retryAttempts[]'. 'attempt' and 'status' are handled separately because + // they are required, and 'attemptId' is listed here because an input that assigned one keeps it. + private static readonly string[] RetryAttemptFields = + [ + "attemptId", + "duration", + "message", + "trace", + "line", + "snippet", + "stdout", + "stderr", + "start", + "stop", + "attachments", + ]; + internal static string Merge(IReadOnlyList inputReports) + => Merge(inputReports, CtrfMergeMode.Concatenate); + + internal static string Merge(IReadOnlyList inputReports, CtrfMergeMode mode) { if (inputReports is null) { @@ -64,6 +86,10 @@ internal static string Merge(IReadOnlyList inputReports) JsonNode? firstTool = null; int reportCount = 0; + // The merged document belongs to the same logical run as its inputs only when they all belong to the + // same one, so a run id is carried over only when every input reported the very same value. + var distinctRunIds = new HashSet(StringComparer.Ordinal); + // Collect each input's environment so shared fields can be retained and module- or agent-specific // ones (values that differ across inputs) dropped, rather than attributing the first report's // environment to every merged test. @@ -94,6 +120,11 @@ internal static string Merge(IReadOnlyList inputReports) reportCount++; acceptedReports.Add(reportJson); + distinctRunIds.Add( + root["runId"] is JsonValue runIdValue && runIdValue.TryGetValue(out string? runIdText) && !RoslynString.IsNullOrEmpty(runIdText) + ? runIdText + : string.Empty); + if (root["results"]?["environment"] is JsonObject environment) { environments.Add(environment); @@ -157,11 +188,18 @@ internal static string Merge(IReadOnlyList inputReports) long startMs = earliestStart ?? 0; long stopMs = latestStop ?? startMs; + // In retry mode the inputs are successive attempts of the same suite, so the same logical test can + // appear in several of them. Collapse those repeats into one row before counting, otherwise the same + // test would be reported (and counted) several times. + JsonArray tests = mode == CtrfMergeMode.CollapseRetryAttempts + ? CollapseRetryAttempts(mergedTests) + : mergedTests; + // Counters are derived from the merged tests[] rather than trusting each input's summary, so // summary.tests always equals the array length even when an input omitted or under-reported // its summary. long passed = 0, failed = 0, skipped = 0, pending = 0, other = 0, flaky = 0; - foreach (JsonNode? test in mergedTests) + foreach (JsonNode? test in tests) { if (test is null) { @@ -185,7 +223,7 @@ internal static string Merge(IReadOnlyList inputReports) var summaryObject = new JsonObject { - ["tests"] = mergedTests.Count, + ["tests"] = tests.Count, ["passed"] = passed, ["failed"] = failed, ["skipped"] = skipped, @@ -221,21 +259,32 @@ internal static string Merge(IReadOnlyList inputReports) resultsObject["environment"] = commonEnvironment; } - resultsObject["tests"] = mergedTests; + resultsObject["tests"] = tests; var merged = new JsonObject { ["reportFormat"] = first["reportFormat"]?.DeepClone() ?? "CTRF", ["specVersion"] = first["specVersion"]?.DeepClone() ?? "0.0.0", ["reportId"] = CreateDeterministicReportId(acceptedReports), - ["timestamp"] = DateTimeOffset.FromUnixTimeMilliseconds(stopMs).ToString("O", CultureInfo.InvariantCulture), - // The merged document is produced by this merger, not by any input, so stamp its own identity - // rather than carrying the first input's 'generatedBy' (which could report a different producer - // or version when merging reports from different tool versions). - ["generatedBy"] = GeneratedByName, - ["results"] = resultsObject, }; + // A merged document is a new artifact (hence its own reportId) but it still describes the same logical + // run as the documents it was built from, so it carries their runId — provided they all agree on one. + // Inputs that disagree, or an input with no runId, contribute the empty sentinel and suppress the field + // rather than picking an arbitrary run to represent the whole merge. + if (distinctRunIds.Count == 1 && distinctRunIds.First() is { Length: > 0 } sharedRunId) + { + merged["runId"] = sharedRunId; + } + + merged["timestamp"] = DateTimeOffset.FromUnixTimeMilliseconds(stopMs).ToString("O", CultureInfo.InvariantCulture); + + // The merged document is produced by this merger, not by any input, so stamp its own identity + // rather than carrying the first input's 'generatedBy' (which could report a different producer + // or version when merging reports from different tool versions). + merged["generatedBy"] = GeneratedByName; + merged["results"] = resultsObject; + return merged.ToJsonString(new JsonSerializerOptions { WriteIndented = true }); } @@ -243,6 +292,13 @@ internal static async Task MergeToFileAsync( IReadOnlyList inputPaths, string outputPath, CancellationToken cancellationToken) + => await MergeToFileAsync(inputPaths, outputPath, CtrfMergeMode.Concatenate, cancellationToken).ConfigureAwait(false); + + internal static async Task MergeToFileAsync( + IReadOnlyList inputPaths, + string outputPath, + CtrfMergeMode mode, + CancellationToken cancellationToken) { if (inputPaths is null) { @@ -276,7 +332,7 @@ internal static async Task MergeToFileAsync( #endif } - string merged = Merge(reports); + string merged = Merge(reports, mode); string? outputDirectory = Path.GetDirectoryName(outputPath); if (!RoslynString.IsNullOrEmpty(outputDirectory)) @@ -297,6 +353,210 @@ await MergeOutputFileHelper.WriteViaTemporarySiblingAsync(outputPath, async temp }).ConfigureAwait(false); } + /// + /// Folds successive attempts of the same logical test — the tests[] rows contributed, in attempt order, by + /// the per-attempt documents of one orchestrated retry run — into a single row per test. + /// + /// + /// The retry model confirmed in ctrf-io/ctrf#58: the last attempt's outcome IS the test object, and + /// retryAttempts[] holds attempts 1..N-1 (the initial execution plus any earlier retries), + /// numbered from 1, so retries == retryAttempts.length and the final attempt is retries + 1. + /// The test object therefore keeps the FINAL attempt's duration (and start/stop), rather + /// than a sum across attempts. Attempts an input already recorded in its own retryAttempts[] (in-process + /// retries within a single attempt process) are flattened into the same history so no execution is lost. + /// + private static JsonArray CollapseRetryAttempts(JsonArray tests) + { + var slots = new List<(JsonNode Final, List Priors)>(); + var byIdentity = new Dictionary(StringComparer.Ordinal); + + foreach (JsonNode? test in tests) + { + if (test is null) + { + continue; + } + + // A row we cannot identify gets its own slot: fusing unrelated rows would lose results, whereas an + // uncollapsed duplicate is merely redundant. + if (GetTestIdentity(test) is not string identity) + { + slots.Add((test, [])); + continue; + } + + if (byIdentity.TryGetValue(identity, out int index)) + { + (JsonNode previousFinal, List priors) = slots[index]; + priors.Add(previousFinal); + slots[index] = (test, priors); + } + else + { + byIdentity.Add(identity, slots.Count); + slots.Add((test, [])); + } + } + + var collapsed = new JsonArray(); + foreach ((JsonNode final, List priors) in slots) + { + collapsed.Add(BuildCollapsedTest(final, priors)); + } + + return collapsed; + } + + /// + /// Computes the key identifying the logical test a row describes, preferring the CTRF testId, then the + /// producer-supplied extra.uid, and finally the suite path plus name. Returns + /// when the row carries none of them. + /// + private static string? GetTestIdentity(JsonNode test) + { + if (test["testId"] is JsonValue testIdValue && testIdValue.TryGetValue(out string? testId) && !RoslynString.IsNullOrEmpty(testId)) + { + return $"testId\u001f{testId}"; + } + + if (test["extra"]?["uid"] is JsonValue uidValue && uidValue.TryGetValue(out string? uid) && !RoslynString.IsNullOrEmpty(uid)) + { + return $"uid\u001f{uid}"; + } + + if (test["name"] is not JsonValue nameValue || !nameValue.TryGetValue(out string? name) || RoslynString.IsNullOrEmpty(name)) + { + return null; + } + + var identity = new StringBuilder("name"); + if (test["suite"] is JsonArray suite) + { + foreach (JsonNode? segment in suite) + { + identity.Append('\u001f').Append((string?)segment); + } + } + + return identity.Append('\u001f').Append(name).ToString(); + } + + private static JsonNode BuildCollapsedTest(JsonNode final, List priors) + { + var collapsed = (JsonObject)final.DeepClone(); + if (priors.Count == 0) + { + return collapsed; + } + + var history = new JsonArray(); + foreach (JsonNode prior in priors) + { + AppendAttempts(history, prior); + } + + // In-process retries observed by the final attempt precede its own outcome in the history. + if (collapsed["retryAttempts"] is JsonArray finalAttempts) + { + AppendNestedAttempts(history, finalAttempts); + } + + bool anyFailed = false; + for (int i = 0; i < history.Count; i++) + { + if (history[i] is not JsonObject attempt) + { + continue; + } + + attempt["attempt"] = i + 1; + anyFailed |= (string?)attempt["status"] == "failed"; + } + + collapsed["retryAttempts"] = history; + collapsed["retries"] = history.Count; + + // CTRF 9.22: flaky only when the FINAL status is passed after at least one failed attempt. Recomputed + // (rather than inherited) because an input's own flag only describes the attempt that produced it. + if ((string?)collapsed["status"] == "passed" && anyFailed) + { + collapsed["flaky"] = true; + } + else + { + collapsed.Remove("flaky"); + } + + return collapsed; + } + + /// + /// Appends the executions a non-final attempt row represents — the attempts it already nested, then its own + /// outcome — to the retry history being built. + /// + private static void AppendAttempts(JsonArray history, JsonNode test) + { + if (test["retryAttempts"] is JsonArray nested) + { + AppendNestedAttempts(history, nested); + } + + history.Add(ToRetryAttempt(test)); + } + + /// + /// Copies retry attempt objects an input already recorded into the history being built, skipping entries that + /// are not objects: a element could not describe an execution and is not a valid retry + /// attempt object. + /// + private static void AppendNestedAttempts(JsonArray history, JsonArray nested) + { + foreach (JsonNode? attempt in nested) + { + if (attempt is JsonObject attemptObject) + { + history.Add(attemptObject.DeepClone()); + } + } + } + + /// + /// Projects a test row onto the retry attempt object of CTRF section 11, which allows a narrower set of + /// fields than a test: everything else (name, suite, tags, labels, ...) either belongs to the collapsed test + /// object or, like rawStatus, moves under extra, the only permitted extension point. + /// + private static JsonNode ToRetryAttempt(JsonNode test) + { + // 'attempt' is assigned by the caller, once the position of this execution in the history is known. + var attempt = new JsonObject + { + ["attempt"] = 1, + ["status"] = test["status"]?.DeepClone() ?? "other", + }; + + foreach (string field in RetryAttemptFields) + { + if (test[field] is JsonNode value) + { + attempt[field] = value.DeepClone(); + } + } + + JsonObject? extra = test["extra"] is JsonObject testExtra ? (JsonObject)testExtra.DeepClone() : null; + if (test["rawStatus"] is JsonNode rawStatus) + { + extra ??= []; + extra["rawStatus"] = rawStatus.DeepClone(); + } + + if (extra is { Count: > 0 }) + { + attempt["extra"] = extra; + } + + return attempt; + } + private static bool TryReadLong(JsonNode summary, string propertyName, out long value) { value = 0; diff --git a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/InternalAPI/InternalAPI.Unshipped.txt b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/InternalAPI/InternalAPI.Unshipped.txt index 925dc5999d..65be134020 100644 --- a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/InternalAPI/InternalAPI.Unshipped.txt +++ b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/InternalAPI/InternalAPI.Unshipped.txt @@ -1,7 +1,12 @@ #nullable enable virtual Microsoft.Testing.Extensions.ReportGeneratorBase.ArtifactKind.get -> string? +Microsoft.Testing.Extensions.CtrfReport.CtrfMergeMode +Microsoft.Testing.Extensions.CtrfReport.CtrfMergeMode.CollapseRetryAttempts = 1 -> Microsoft.Testing.Extensions.CtrfReport.CtrfMergeMode +Microsoft.Testing.Extensions.CtrfReport.CtrfMergeMode.Concatenate = 0 -> Microsoft.Testing.Extensions.CtrfReport.CtrfMergeMode Microsoft.Testing.Extensions.CtrfReport.CtrfReportMerger static Microsoft.Testing.Extensions.CtrfReport.CtrfReportMerger.Merge(System.Collections.Generic.IReadOnlyList! inputReports) -> string! +static Microsoft.Testing.Extensions.CtrfReport.CtrfReportMerger.Merge(System.Collections.Generic.IReadOnlyList! inputReports, Microsoft.Testing.Extensions.CtrfReport.CtrfMergeMode mode) -> string! +static Microsoft.Testing.Extensions.CtrfReport.CtrfReportMerger.MergeToFileAsync(System.Collections.Generic.IReadOnlyList! inputPaths, string! outputPath, Microsoft.Testing.Extensions.CtrfReport.CtrfMergeMode mode, System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.Task! static Microsoft.Testing.Extensions.CtrfReport.CtrfReportMerger.MergeToFileAsync(System.Collections.Generic.IReadOnlyList! inputPaths, string! outputPath, System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.Task! static Microsoft.Testing.Platform.Resources.PlatformResources.NamedPipeDirectoryNotWritableErrorMessage.get -> string! static Microsoft.Testing.Platform.Resources.PlatformResources.NamedPipePathTooLongErrorMessage.get -> string! diff --git a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/Microsoft.Testing.Extensions.CtrfReport.csproj b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/Microsoft.Testing.Extensions.CtrfReport.csproj index a9f7c6dccd..f65438334b 100644 --- a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/Microsoft.Testing.Extensions.CtrfReport.csproj +++ b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/Microsoft.Testing.Extensions.CtrfReport.csproj @@ -52,6 +52,7 @@ This package extends Microsoft Testing Platform to produce test reports in the C + diff --git a/src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs b/src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs index 247c50e215..80e4686512 100644 --- a/src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs +++ b/src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs @@ -2,6 +2,7 @@ // Licensed under dual-license. See LICENSE.PLATFORMTOOLS.txt file in the project root for full license information. using Microsoft.Testing.Extensions.Policy.Resources; +using Microsoft.Testing.Platform; using Microsoft.Testing.Platform.CommandLine; using Microsoft.Testing.Platform.Configurations; using Microsoft.Testing.Platform.Extensions.OutputDevice; @@ -84,6 +85,15 @@ public async Task OrchestrateTestHostExecutionAsync(CancellationToken cance environment.SetEnvironmentVariable(EnvironmentVariableConstants.TESTINGPLATFORM_TRX_TESTRUN_ID, Guid.NewGuid().ToString("N")); + // Every attempt is a separate process writing its own report, but together they are one logical run. + // Formats that can express that (CTRF 'runId') need all attempts to agree on a single id, so seed one + // here and let the launched test hosts inherit it. An id set by an outer orchestrator (for example + // 'dotnet test' correlating several modules) already covers these attempts, so it is preserved. + if (RoslynString.IsNullOrEmpty(environment.GetEnvironmentVariable(EnvironmentVariableConstants.TESTINGPLATFORM_LOGICAL_RUN_ID))) + { + environment.SetEnvironmentVariable(EnvironmentVariableConstants.TESTINGPLATFORM_LOGICAL_RUN_ID, Guid.NewGuid().ToString("N")); + } + ILogger logger = _serviceProvider.GetLoggerFactory().CreateLogger(); IConfiguration configuration = _serviceProvider.GetConfiguration(); diff --git a/src/Platform/Microsoft.Testing.Platform/Helpers/EnvironmentVariableConstants.cs b/src/Platform/Microsoft.Testing.Platform/Helpers/EnvironmentVariableConstants.cs index 83eed5246e..185c6b1312 100644 --- a/src/Platform/Microsoft.Testing.Platform/Helpers/EnvironmentVariableConstants.cs +++ b/src/Platform/Microsoft.Testing.Platform/Helpers/EnvironmentVariableConstants.cs @@ -58,6 +58,12 @@ internal static class EnvironmentVariableConstants // Unhandled Exception public const string TESTINGPLATFORM_EXIT_PROCESS_ON_UNHANDLED_EXCEPTION = nameof(TESTINGPLATFORM_EXIT_PROCESS_ON_UNHANDLED_EXCEPTION); + // Correlates every process that takes part in the same logical test run: the successive attempts launched by + // the retry orchestrator, or the modules of a single 'dotnet test' invocation. Report formats that model a + // logical run spanning several documents (CTRF 'runId') surface this value so a consumer can tie those + // documents back together. It may also be set externally to correlate shards running on different machines. + public const string TESTINGPLATFORM_LOGICAL_RUN_ID = nameof(TESTINGPLATFORM_LOGICAL_RUN_ID); + // Trx public const string TESTINGPLATFORM_TRX_TESTRUN_ID = nameof(TESTINGPLATFORM_TRX_TESTRUN_ID); } diff --git a/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/CtrfReportTests.cs b/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/CtrfReportTests.cs index 48891a2b24..1d862aaee4 100644 --- a/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/CtrfReportTests.cs +++ b/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/CtrfReportTests.cs @@ -135,6 +135,10 @@ private static void AssertCtrfReportShape(string filePath) // - FlakyTest (retried) → first attempt failed, retried successfully: // final status: "passed", retries: 1, // retryAttempts[].status: "failed", flaky: true + // + // The retry shape follows the model confirmed in ctrf-io/ctrf#58: `retryAttempts[]` + // holds attempts 1..N-1 (the final attempt's outcome IS the test object), so + // `retries` equals `retryAttempts.length` and the final attempt is `retries + 1`. string actual = File.ReadAllText(filePath); string normalized = NormalizeCtrfReport(actual); @@ -143,6 +147,7 @@ private static void AssertCtrfReportShape(string filePath) "reportFormat": "CTRF", "specVersion": "0.0.0", "reportId": "", + "runId": "", "timestamp": "", "generatedBy": "Microsoft.Testing.Extensions.CtrfReport@", "results": { @@ -228,6 +233,7 @@ private static string NormalizeCtrfReport(string actual) // anchored, but runtime-variable values are folded into stable tokens. string normalized = actual; normalized = Regex.Replace(normalized, @"""reportId"": ""[^""]+""", @"""reportId"": """""); + normalized = Regex.Replace(normalized, @"""runId"": ""[^""]+""", @"""runId"": """""); normalized = Regex.Replace(normalized, @"""timestamp"": ""[^""]+""", @"""timestamp"": """""); normalized = Regex.Replace(normalized, @"""generatedBy"": ""Microsoft\.Testing\.Extensions\.CtrfReport@[^""]+""", @"""generatedBy"": ""Microsoft.Testing.Extensions.CtrfReport@"""); normalized = Regex.Replace(normalized, @"""start"": \d+", @"""start"": "); diff --git a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportEngineTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportEngineTests.cs index d2b780131e..54401573c8 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportEngineTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportEngineTests.cs @@ -830,6 +830,53 @@ public async Task GenerateReportAsync_SingleLineOutput_EmitsOneArrayEntry() Assert.AreEqual("only-line", stdout[0].GetString()); } + [TestMethod] + public async Task GenerateReportAsync_RunId_UsesTheSharedLogicalRunId() + { + // ctrf-io/ctrf#58: every process of one logical run (the attempts of --retry-failed-tests, the modules + // of one dotnet test invocation) must stamp the same runId, so consumers can tie those documents back + // together. The orchestrator publishes that id through this environment variable. + using var memoryStream = new MemoryFileStream(); + CtrfReportEngine engine = CreateEngine(memoryStream); + _ = _environmentMock.Setup(x => x.GetEnvironmentVariable("TESTINGPLATFORM_LOGICAL_RUN_ID")).Returns("run-42"); + + await engine.GenerateReportAsync([Captured("p1", "Passing test", "passed")]); + + using var document = JsonDocument.Parse(memoryStream.GetUtf8Content()); + Assert.AreEqual("run-42", document.RootElement.GetProperty("runId").GetString()); + } + + [TestMethod] + public async Task GenerateReportAsync_RunId_FallsBackToTheDotnetTestExecutionId() + { + using var memoryStream = new MemoryFileStream(); + CtrfReportEngine engine = CreateEngine(memoryStream); + _ = _environmentMock.Setup(x => x.GetEnvironmentVariable("TESTINGPLATFORM_LOGICAL_RUN_ID")).Returns((string?)null); + _ = _environmentMock.Setup(x => x.GetEnvironmentVariable("TESTINGPLATFORM_DOTNETTEST_EXECUTIONID")).Returns("execution-7"); + + await engine.GenerateReportAsync([Captured("p1", "Passing test", "passed")]); + + using var document = JsonDocument.Parse(memoryStream.GetUtf8Content()); + Assert.AreEqual("execution-7", document.RootElement.GetProperty("runId").GetString()); + } + + [TestMethod] + public async Task GenerateReportAsync_RunId_IsGenerated_WhenNothingCorrelatedTheProcess() + { + // A standalone run is its own logical run, and CTRF requires runId to be a non-empty string when present. + using var memoryStream = new MemoryFileStream(); + CtrfReportEngine engine = CreateEngine(memoryStream); + _ = _environmentMock.Setup(x => x.GetEnvironmentVariable("TESTINGPLATFORM_LOGICAL_RUN_ID")).Returns((string?)null); + _ = _environmentMock.Setup(x => x.GetEnvironmentVariable("TESTINGPLATFORM_DOTNETTEST_EXECUTIONID")).Returns((string?)null); + + await engine.GenerateReportAsync([Captured("p1", "Passing test", "passed")]); + + using var document = JsonDocument.Parse(memoryStream.GetUtf8Content()); + string runId = document.RootElement.GetProperty("runId").GetString()!; + Assert.IsTrue(Guid.TryParse(runId, out _), $"Expected a generated id, got '{runId}'."); + Assert.AreNotEqual(document.RootElement.GetProperty("reportId").GetString(), runId); + } + private CtrfReportEngine CreateEngine(MemoryFileStream stream) { _ = _fileSystem.Setup(x => x.ExistFile(It.IsAny())).Returns(false); diff --git a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs index 7efdd64aba..ef71412787 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs @@ -490,6 +490,7 @@ private static string BuildReport( string toolName = "MSTest", string? toolVersion = null, string osPlatform = "test", + string? runId = null, IEnumerable? testEntries = null) { var testArray = new JsonArray(); @@ -542,9 +543,259 @@ private static string BuildReport( }, }; + if (runId is not null) + { + report["runId"] = runId; + } + return report.ToJsonString(); } + [TestMethod] + public void Merge_CarriesRunId_WhenEveryInputReportsTheSameOne() + { + // Per-attempt (and per-shard) documents of one logical run share a runId; the merged document + // describes that same logical run, so it keeps the id while getting its own reportId. + string a = BuildReport(runId: "run-42", testEntries: [Test("a", "passed")]); + string b = BuildReport(runId: "run-42", testEntries: [Test("b", "passed")]); + + JsonNode merged = JsonNode.Parse(CtrfReportMerger.Merge([a, b]))!; + + Assert.AreEqual("run-42", (string?)merged["runId"]); + Assert.AreNotEqual("run-42", (string?)merged["reportId"]); + Assert.AreNotEqual((string?)JsonNode.Parse(a)!["reportId"], (string?)merged["reportId"]); + } + + [TestMethod] + public void Merge_OmitsRunId_WhenInputsBelongToDifferentRuns() + { + string a = BuildReport(runId: "run-1"); + string b = BuildReport(runId: "run-2"); + + JsonNode merged = JsonNode.Parse(CtrfReportMerger.Merge([a, b]))!; + + Assert.IsNull(merged["runId"]); + } + + [TestMethod] + public void Merge_OmitsRunId_WhenAnInputHasNone() + { + // An input with no runId may or may not belong to the same run, so claiming the known one would + // assert a correlation the inputs do not support. + string a = BuildReport(runId: "run-1"); + string b = BuildReport(); + + JsonNode merged = JsonNode.Parse(CtrfReportMerger.Merge([a, b]))!; + + Assert.IsNull(merged["runId"]); + } + + [TestMethod] + public void Merge_DefaultMode_DoesNotCollapseRepeatedTests() + { + // Cross-module merges must keep every row: MTP UIDs are only unique within an assembly, so two + // same-named tests can legitimately come from different modules. + string attempt1 = BuildReport(testEntries: [Attempt("t", "failed", uid: "u1")]); + string attempt2 = BuildReport(testEntries: [Attempt("t", "passed", uid: "u1")]); + + JsonNode results = JsonNode.Parse(CtrfReportMerger.Merge([attempt1, attempt2]))!["results"]!; + + Assert.HasCount(2, (JsonArray)results["tests"]!); + Assert.AreEqual(2, (long)results["summary"]!["tests"]!); + } + + [TestMethod] + public void Merge_CollapseRetryAttempts_FoldsEarlierAttemptsIntoRetryHistory() + { + // Three per-attempt documents of one orchestrated retry run: attempts 1 and 2 failed, attempt 3 passed. + string attempt1 = BuildReport(testEntries: [Attempt("flaky test", "failed", uid: "u1", duration: 120, message: "boom 1")]); + string attempt2 = BuildReport(testEntries: [Attempt("flaky test", "failed", uid: "u1", duration: 130, message: "boom 2")]); + string attempt3 = BuildReport(testEntries: [Attempt("flaky test", "passed", uid: "u1", duration: 140)]); + + JsonNode results = JsonNode.Parse(CtrfReportMerger.Merge([attempt1, attempt2, attempt3], CtrfMergeMode.CollapseRetryAttempts))!["results"]!; + + var tests = (JsonArray)results["tests"]!; + Assert.HasCount(1, tests); + + JsonNode test = tests[0]!; + Assert.AreEqual("passed", (string?)test["status"]); + Assert.IsTrue((bool)test["flaky"]!); + + // ctrf-io/ctrf#58: retryAttempts holds attempts 1..N-1, so retries == retryAttempts.length and the + // final attempt is retries + 1. + var retryAttempts = (JsonArray)test["retryAttempts"]!; + Assert.HasCount(2, retryAttempts); + Assert.AreEqual(2, (long)test["retries"]!); + Assert.AreEqual(1, (long)retryAttempts[0]!["attempt"]!); + Assert.AreEqual(2, (long)retryAttempts[1]!["attempt"]!); + Assert.AreEqual("boom 1", (string?)retryAttempts[0]!["message"]); + Assert.AreEqual("boom 2", (string?)retryAttempts[1]!["message"]); + + // The test object carries the FINAL attempt's duration, not the sum across attempts. + Assert.AreEqual(140, (long)test["duration"]!); + Assert.AreEqual(120, (long)retryAttempts[0]!["duration"]!); + } + + [TestMethod] + public void Merge_CollapseRetryAttempts_CountsLogicalTestsOnce() + { + // The scenario from ctrf-io/ctrf#58: a 4-test suite where each attempt re-runs only what failed. + // The merged document describes the logical run, so it reports 4 tests, not the 8 executions. + string attempt1 = BuildReport(testEntries: + [ + Attempt("ok1", "passed", uid: "u1"), + Attempt("ok2", "passed", uid: "u2"), + Attempt("recovers", "failed", uid: "u3"), + Attempt("always fails", "failed", uid: "u4"), + ]); + string attempt2 = BuildReport(testEntries: + [ + Attempt("recovers", "passed", uid: "u3"), + Attempt("always fails", "failed", uid: "u4"), + ]); + string attempt3 = BuildReport(testEntries: [Attempt("always fails", "failed", uid: "u4")]); + string attempt4 = BuildReport(testEntries: [Attempt("always fails", "failed", uid: "u4")]); + + JsonNode results = JsonNode.Parse( + CtrfReportMerger.Merge([attempt1, attempt2, attempt3, attempt4], CtrfMergeMode.CollapseRetryAttempts))!["results"]!; + + JsonNode summary = results["summary"]!; + Assert.AreEqual(4, (long)summary["tests"]!); + Assert.AreEqual(3, (long)summary["passed"]!); + Assert.AreEqual(1, (long)summary["failed"]!); + Assert.AreEqual(1, (long)summary["flaky"]!); + Assert.HasCount(4, (JsonArray)results["tests"]!); + + JsonNode alwaysFails = ((JsonArray)results["tests"]!).Single(t => (string?)t!["name"] == "always fails")!; + Assert.AreEqual("failed", (string?)alwaysFails["status"]); + Assert.AreEqual(3, (long)alwaysFails["retries"]!); + Assert.IsNull(alwaysFails["flaky"]); + + JsonNode neverRetried = ((JsonArray)results["tests"]!).Single(t => (string?)t!["name"] == "ok1")!; + Assert.IsNull(neverRetried["retries"]); + Assert.IsNull(neverRetried["retryAttempts"]); + } + + [TestMethod] + public void Merge_CollapseRetryAttempts_FlattensInProcessRetriesOfEachAttempt() + { + // An attempt process can itself have retried the test in-process; those executions are already in its + // retryAttempts[] and must keep their place in the merged history instead of being dropped. + JsonObject firstAttempt = Attempt("t", "failed", uid: "u1", message: "second execution"); + firstAttempt["retryAttempts"] = new JsonArray(new JsonObject + { + ["attempt"] = 1, + ["status"] = "failed", + ["message"] = "first execution", + }); + + string attempt1 = BuildReport(testEntries: [firstAttempt]); + string attempt2 = BuildReport(testEntries: [Attempt("t", "passed", uid: "u1")]); + + JsonNode test = ((JsonArray)JsonNode.Parse( + CtrfReportMerger.Merge([attempt1, attempt2], CtrfMergeMode.CollapseRetryAttempts))!["results"]!["tests"]!)[0]!; + + var retryAttempts = (JsonArray)test["retryAttempts"]!; + Assert.HasCount(2, retryAttempts); + Assert.AreEqual(3, (long)test["retries"]! + 1, "The final attempt is retries + 1."); + Assert.AreEqual("first execution", (string?)retryAttempts[0]!["message"]); + Assert.AreEqual("second execution", (string?)retryAttempts[1]!["message"]); + Assert.AreEqual(1, (long)retryAttempts[0]!["attempt"]!); + Assert.AreEqual(2, (long)retryAttempts[1]!["attempt"]!); + } + + [TestMethod] + public void Merge_CollapseRetryAttempts_ProjectsAttemptsOntoRetryAttemptShape() + { + // CTRF section 11 forbids unknown fields on a retry attempt, so test-only fields must not leak into it; + // rawStatus has no attempt-level slot and moves under 'extra', the only permitted extension point. + JsonObject failing = Attempt("t", "failed", uid: "u1", message: "boom"); + failing["rawStatus"] = "timedOut"; + failing["suite"] = new JsonArray("NS", "C"); + failing["tags"] = new JsonArray("slow"); + failing["trace"] = "at X()"; + + string attempt1 = BuildReport(testEntries: [failing]); + string attempt2 = BuildReport(testEntries: [Attempt("t", "passed", uid: "u1")]); + + JsonNode test = ((JsonArray)JsonNode.Parse( + CtrfReportMerger.Merge([attempt1, attempt2], CtrfMergeMode.CollapseRetryAttempts))!["results"]!["tests"]!)[0]!; + + JsonNode retryAttempt = ((JsonArray)test["retryAttempts"]!)[0]!; + Assert.AreEqual("failed", (string?)retryAttempt["status"]); + Assert.AreEqual("boom", (string?)retryAttempt["message"]); + Assert.AreEqual("at X()", (string?)retryAttempt["trace"]); + Assert.AreEqual("timedOut", (string?)retryAttempt["extra"]!["rawStatus"]); + Assert.AreEqual("u1", (string?)retryAttempt["extra"]!["uid"]); + Assert.IsNull(retryAttempt["name"]); + Assert.IsNull(retryAttempt["suite"]); + Assert.IsNull(retryAttempt["tags"]); + Assert.IsNull(retryAttempt["rawStatus"]); + } + + [TestMethod] + public void Merge_CollapseRetryAttempts_DropsStaleFlakyFlag_WhenFinalAttemptFails() + { + // An input's own flaky flag only describes the attempt that produced it; a later failure means the + // logical test is not flaky (CTRF 9.22 requires the FINAL status to be passed). + JsonObject recovered = Attempt("t", "passed", uid: "u1"); + recovered["flaky"] = true; + + string attempt1 = BuildReport(testEntries: [recovered]); + string attempt2 = BuildReport(testEntries: [Attempt("t", "failed", uid: "u1")]); + + JsonNode results = JsonNode.Parse( + CtrfReportMerger.Merge([attempt1, attempt2], CtrfMergeMode.CollapseRetryAttempts))!["results"]!; + + JsonNode test = ((JsonArray)results["tests"]!)[0]!; + Assert.AreEqual("failed", (string?)test["status"]); + Assert.IsNull(test["flaky"]); + Assert.AreEqual(0, (long)results["summary"]!["flaky"]!); + } + + [TestMethod] + public void Merge_CollapseRetryAttempts_UsesNameAndSuite_WhenNoIdentifierIsAvailable() + { + // Without testId or extra.uid, the suite path plus name is the only identity available, and two tests + // that only share a name must not be fused. + static JsonObject Named(string name, string status, string suite) + { + JsonObject test = Test(name, status); + test["suite"] = new JsonArray(suite); + return test; + } + + string attempt1 = BuildReport(testEntries: [Named("t", "failed", "A"), Named("t", "failed", "B")]); + string attempt2 = BuildReport(testEntries: [Named("t", "passed", "A"), Named("t", "failed", "B")]); + + var tests = (JsonArray)JsonNode.Parse( + CtrfReportMerger.Merge([attempt1, attempt2], CtrfMergeMode.CollapseRetryAttempts))!["results"]!["tests"]!; + + Assert.HasCount(2, tests); + Assert.AreEqual("passed", (string?)tests[0]!["status"]); + Assert.AreEqual("failed", (string?)tests[1]!["status"]); + Assert.AreEqual(1, (long)tests[0]!["retries"]!); + Assert.AreEqual(1, (long)tests[1]!["retries"]!); + } + + private static JsonObject Attempt(string name, string status, string uid, long duration = 1, string? message = null) + { + var test = new JsonObject + { + ["name"] = name, + ["status"] = status, + ["duration"] = duration, + ["extra"] = new JsonObject { ["uid"] = uid }, + }; + + if (message is not null) + { + test["message"] = message; + } + + return test; + } + [TestMethod] public void Merge_WhenAnInputHasNoEnvironment_DropsEnvironment() { From 4f6725ea51971c169609d7fbc3eaa699e54d0f66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Thu, 30 Jul 2026 14:10:37 +0200 Subject: [PATCH 02/11] Address review findings on CTRF runId and retry merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correct the logical-run scope, and harden the merger against malformed input. `runId` scope. Four comments claimed the `dotnet test` execution id correlates the modules of one invocation. It does not: it is minted per root test application and propagated only to that process tree, so sibling modules of a multi-project run legitimately get different ids (docs/mstest-runner-protocol/004-protocol-dotnet-test-pipe.md). The comments now describe the real scope and state that correlating modules or machines requires setting TESTINGPLATFORM_LOGICAL_RUN_ID explicitly. `RetryOrchestrator` now mirrors the engine's resolution chain (`existing ?? execution id ?? fresh`), so a retried module stays part of its own execution tree's run instead of forming a separate one, and uses the same `"D"` GUID format as the engine. Malformed `tests[]` rows. A non-object element (a bare string, or a JSON null left by a lazy producer) threw `InvalidOperationException` from four places, three of them on the pre-existing `Concatenate` path: the summary counter loop, `TryReadLong` in the ingestion loop, `GetTestIdentity`'s `extra` lookup, and the collapse helpers. Such rows are now rejected at ingestion — carrying one through would turn a defect localized to one input into a schema-invalid merged document, whereas dropping it matches how a whole non-CTRF input is already rejected. With `tests[]` filtered once at the boundary, the collapse helpers take `JsonObject` and the invariant is enforced by the type system rather than by repeated guards. `TryReadLong` keeps its guard for a non-object `results.summary`. Tests. Add an acceptance test asserting that the per-attempt CTRF documents of one `--retry-failed-tests` run share a `runId` and carry distinct `reportId`s — the only test that covers the cross-process contract, since the engine unit tests mock `IEnvironment`. Add merger tests for malformed rows (both modes) and for a non-object `extra`. Each new test was mutation-verified to fail when the production logic it pins is reverted. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ad2ea8a-3349-4979-b27c-dd52bb3b7190 --- .../CtrfReportEngine.JsonSerializer.cs | 20 ++--- .../CtrfReportMerger.cs | 81 ++++++++++++------- .../RetryOrchestrator.cs | 14 +++- .../Helpers/EnvironmentVariableConstants.cs | 12 ++- .../RetryFailedTestsTests.cs | 60 +++++++++++++- .../CtrfReportEngineTests.cs | 10 ++- .../CtrfReportMergerTests.cs | 60 ++++++++++++++ 7 files changed, 205 insertions(+), 52 deletions(-) diff --git a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.JsonSerializer.cs b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.JsonSerializer.cs index 35f49fe61c..a19159605a 100644 --- a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.JsonSerializer.cs +++ b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.JsonSerializer.cs @@ -70,10 +70,9 @@ private byte[] BuildCtrfJson(CapturedTestResult[] results, DateTimeOffset finish writer.WriteString("specVersion", CtrfSpecVersion); writer.WriteString("reportId", Guid.NewGuid().ToString("D")); // CTRF 5.4 (`runId`): identifies the logical run this document belongs to. A logical run can span - // several documents — the modules of one `dotnet test` invocation, or the successive processes of - // `--retry-failed-tests`, where each attempt writes its own document. ctrf-io/ctrf#58 confirmed that - // those per-execution documents (and any document merged from them) SHOULD share a `runId` while each - // keeps its own `reportId`. When nothing correlated this process, it is the whole logical run. + // several documents — most notably the successive processes of `--retry-failed-tests`, where each + // attempt writes its own document. ctrf-io/ctrf#58 confirmed that those per-execution documents (and + // any document merged from them) SHOULD share a `runId` while each keeps its own `reportId`. writer.WriteString("runId", ResolveRunId()); writer.WriteString("timestamp", finishTime.ToString("O", CultureInfo.InvariantCulture)); writer.WriteString( @@ -164,10 +163,13 @@ private byte[] BuildCtrfJson(CapturedTestResult[] results, DateTimeOffset finish /// Resolves the CTRF runId: the id of the logical run this document belongs to. /// /// - /// The retry orchestrator seeds TESTINGPLATFORM_LOGICAL_RUN_ID before launching its attempts, so every - /// attempt process stamps the same value; dotnet test already correlates the modules of one invocation - /// through its execution id, which serves the same purpose when no orchestrator is involved. Falling back to a - /// fresh id keeps the field a valid non-empty string for a standalone run, which is its own logical run. + /// The retry orchestrator sets TESTINGPLATFORM_LOGICAL_RUN_ID before launching its attempts, so every + /// attempt process stamps the same value; a CI job can set it too, to correlate documents this process cannot + /// know about (the modules of a multi-project run, or shards on different machines). Failing that, the + /// dotnet test execution id identifies this test application's own process tree — note it is per root + /// test application, NOT per dotnet test invocation, so sibling modules legitimately get distinct ids + /// (see docs/mstest-runner-protocol/004-protocol-dotnet-test-pipe.md). A fresh id is the last resort: + /// an uncorrelated run is a logical run of its own, and CTRF requires the field to be a non-empty string. /// private string ResolveRunId() { @@ -177,6 +179,6 @@ private string ResolveRunId() runId = _environment.GetEnvironmentVariable(EnvironmentVariableConstants.TESTINGPLATFORM_DOTNETTEST_EXECUTIONID); } - return RoslynString.IsNullOrEmpty(runId) ? Guid.NewGuid().ToString("D") : runId; + return RoslynString.IsNullOrEmpty(runId) ? Guid.NewGuid().ToString("D") : runId!; } } diff --git a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs index 2ec881dbff..0defed0db8 100644 --- a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs +++ b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs @@ -135,22 +135,29 @@ internal static string Merge(IReadOnlyList inputReports, CtrfMergeMode m { foreach (JsonNode? test in testArray) { - mergedTests.Add(test?.DeepClone()); + // Only a Test object belongs in tests[]: the CTRF schema types the array's items as objects + // with required members, so carrying a malformed element (a bare string, or a JSON null left + // by a lazy producer) through would turn a defect localized to one input into an invalid + // MERGED document — the artifact consumers actually read. This mirrors the check above, where + // an input that fails the CTRF shape test is rejected outright rather than passed along. + if (test is not JsonObject testObject) + { + continue; + } + + mergedTests.Add(testObject.DeepClone()); // Fall back to per-test timing so a summary-less input (which the merger explicitly // supports) still contributes to the merged min/max instead of being dropped or // forcing the merged timestamp back to the Unix epoch. - if (test is not null) + if (TryReadLong(testObject, "start", out long testStart)) + { + earliestStart = Min(earliestStart, testStart); + } + + if (TryReadLong(testObject, "stop", out long testStop)) { - if (TryReadLong(test, "start", out long testStart)) - { - earliestStart = Min(earliestStart, testStart); - } - - if (TryReadLong(test, "stop", out long testStop)) - { - latestStop = Max(latestStop, testStop); - } + latestStop = Max(latestStop, testStop); } } } @@ -197,16 +204,16 @@ internal static string Merge(IReadOnlyList inputReports, CtrfMergeMode m // Counters are derived from the merged tests[] rather than trusting each input's summary, so // summary.tests always equals the array length even when an input omitted or under-reported - // its summary. + // its summary. Every element is a Test object: non-objects were rejected during ingestion. long passed = 0, failed = 0, skipped = 0, pending = 0, other = 0, flaky = 0; foreach (JsonNode? test in tests) { - if (test is null) + if (test is not JsonObject testObject) { continue; } - switch ((string?)test["status"]) + switch ((string?)testObject["status"]) { case "passed": passed++; break; case "failed": failed++; break; @@ -215,7 +222,7 @@ internal static string Merge(IReadOnlyList inputReports, CtrfMergeMode m default: other++; break; } - if (test["flaky"] is JsonValue flakyValue && flakyValue.TryGetValue(out bool isFlaky) && isFlaky) + if (testObject["flaky"] is JsonValue flakyValue && flakyValue.TryGetValue(out bool isFlaky) && isFlaky) { flaky++; } @@ -367,39 +374,40 @@ await MergeOutputFileHelper.WriteViaTemporarySiblingAsync(outputPath, async temp /// private static JsonArray CollapseRetryAttempts(JsonArray tests) { - var slots = new List<(JsonNode Final, List Priors)>(); + var slots = new List<(JsonObject Final, List Priors)>(); var byIdentity = new Dictionary(StringComparer.Ordinal); foreach (JsonNode? test in tests) { - if (test is null) + // Non-objects were already rejected during ingestion; this only re-establishes the type. + if (test is not JsonObject testObject) { continue; } // A row we cannot identify gets its own slot: fusing unrelated rows would lose results, whereas an // uncollapsed duplicate is merely redundant. - if (GetTestIdentity(test) is not string identity) + if (GetTestIdentity(testObject) is not string identity) { - slots.Add((test, [])); + slots.Add((testObject, [])); continue; } if (byIdentity.TryGetValue(identity, out int index)) { - (JsonNode previousFinal, List priors) = slots[index]; + (JsonObject previousFinal, List priors) = slots[index]; priors.Add(previousFinal); - slots[index] = (test, priors); + slots[index] = (testObject, priors); } else { byIdentity.Add(identity, slots.Count); - slots.Add((test, [])); + slots.Add((testObject, [])); } } var collapsed = new JsonArray(); - foreach ((JsonNode final, List priors) in slots) + foreach ((JsonObject final, List priors) in slots) { collapsed.Add(BuildCollapsedTest(final, priors)); } @@ -412,14 +420,19 @@ private static JsonArray CollapseRetryAttempts(JsonArray tests) /// producer-supplied extra.uid, and finally the suite path plus name. Returns /// when the row carries none of them. /// - private static string? GetTestIdentity(JsonNode test) + private static string? GetTestIdentity(JsonObject test) { if (test["testId"] is JsonValue testIdValue && testIdValue.TryGetValue(out string? testId) && !RoslynString.IsNullOrEmpty(testId)) { return $"testId\u001f{testId}"; } - if (test["extra"]?["uid"] is JsonValue uidValue && uidValue.TryGetValue(out string? uid) && !RoslynString.IsNullOrEmpty(uid)) + // `extra` is free-form, so a foreign producer may well have written a string or an array there; indexing + // anything but an object by property name throws. + if (test["extra"] is JsonObject extra + && extra["uid"] is JsonValue uidValue + && uidValue.TryGetValue(out string? uid) + && !RoslynString.IsNullOrEmpty(uid)) { return $"uid\u001f{uid}"; } @@ -441,7 +454,7 @@ private static JsonArray CollapseRetryAttempts(JsonArray tests) return identity.Append('\u001f').Append(name).ToString(); } - private static JsonNode BuildCollapsedTest(JsonNode final, List priors) + private static JsonNode BuildCollapsedTest(JsonObject final, List priors) { var collapsed = (JsonObject)final.DeepClone(); if (priors.Count == 0) @@ -450,7 +463,7 @@ private static JsonNode BuildCollapsedTest(JsonNode final, List priors } var history = new JsonArray(); - foreach (JsonNode prior in priors) + foreach (JsonObject prior in priors) { AppendAttempts(history, prior); } @@ -494,7 +507,7 @@ private static JsonNode BuildCollapsedTest(JsonNode final, List priors /// Appends the executions a non-final attempt row represents — the attempts it already nested, then its own /// outcome — to the retry history being built. /// - private static void AppendAttempts(JsonArray history, JsonNode test) + private static void AppendAttempts(JsonArray history, JsonObject test) { if (test["retryAttempts"] is JsonArray nested) { @@ -525,7 +538,7 @@ private static void AppendNestedAttempts(JsonArray history, JsonArray nested) /// fields than a test: everything else (name, suite, tags, labels, ...) either belongs to the collapsed test /// object or, like rawStatus, moves under extra, the only permitted extension point. /// - private static JsonNode ToRetryAttempt(JsonNode test) + private static JsonNode ToRetryAttempt(JsonObject test) { // 'attempt' is assigned by the caller, once the position of this execution in the history is known. var attempt = new JsonObject @@ -557,10 +570,16 @@ private static JsonNode ToRetryAttempt(JsonNode test) return attempt; } - private static bool TryReadLong(JsonNode summary, string propertyName, out long value) + /// + /// Reads an integral property from a JSON node, tolerating anything the node may actually be. The node comes + /// straight out of an untrusted document — results.summary is not guaranteed to be an object — so a + /// non-object must read as "absent" rather than throw: indexing a by property name is + /// an . + /// + private static bool TryReadLong(JsonNode node, string propertyName, out long value) { value = 0; - if (summary[propertyName] is not JsonValue jsonValue) + if (node is not JsonObject jsonObject || jsonObject[propertyName] is not JsonValue jsonValue) { return false; } diff --git a/src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs b/src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs index 80e4686512..427a589a1b 100644 --- a/src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs +++ b/src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs @@ -86,12 +86,18 @@ public async Task OrchestrateTestHostExecutionAsync(CancellationToken cance environment.SetEnvironmentVariable(EnvironmentVariableConstants.TESTINGPLATFORM_TRX_TESTRUN_ID, Guid.NewGuid().ToString("N")); // Every attempt is a separate process writing its own report, but together they are one logical run. - // Formats that can express that (CTRF 'runId') need all attempts to agree on a single id, so seed one - // here and let the launched test hosts inherit it. An id set by an outer orchestrator (for example - // 'dotnet test' correlating several modules) already covers these attempts, so it is preserved. + // Formats that can express that (CTRF 'runId') need all attempts to agree on a single id, so establish + // one here and let the launched test hosts inherit it. Preference order: + // - an id already set explicitly (a CI job correlating several modules or machines) is kept; + // - otherwise the dotnet test execution id, which already identifies THIS test application's process + // tree, so the attempts stay part of that run instead of forming a separate one; + // - otherwise a fresh id, because a standalone retried run is its own logical run. if (RoslynString.IsNullOrEmpty(environment.GetEnvironmentVariable(EnvironmentVariableConstants.TESTINGPLATFORM_LOGICAL_RUN_ID))) { - environment.SetEnvironmentVariable(EnvironmentVariableConstants.TESTINGPLATFORM_LOGICAL_RUN_ID, Guid.NewGuid().ToString("N")); + string? executionId = environment.GetEnvironmentVariable(EnvironmentVariableConstants.TESTINGPLATFORM_DOTNETTEST_EXECUTIONID); + environment.SetEnvironmentVariable( + EnvironmentVariableConstants.TESTINGPLATFORM_LOGICAL_RUN_ID, + RoslynString.IsNullOrEmpty(executionId) ? Guid.NewGuid().ToString("D") : executionId!); } ILogger logger = _serviceProvider.GetLoggerFactory().CreateLogger(); diff --git a/src/Platform/Microsoft.Testing.Platform/Helpers/EnvironmentVariableConstants.cs b/src/Platform/Microsoft.Testing.Platform/Helpers/EnvironmentVariableConstants.cs index 185c6b1312..f33e36a843 100644 --- a/src/Platform/Microsoft.Testing.Platform/Helpers/EnvironmentVariableConstants.cs +++ b/src/Platform/Microsoft.Testing.Platform/Helpers/EnvironmentVariableConstants.cs @@ -58,10 +58,14 @@ internal static class EnvironmentVariableConstants // Unhandled Exception public const string TESTINGPLATFORM_EXIT_PROCESS_ON_UNHANDLED_EXCEPTION = nameof(TESTINGPLATFORM_EXIT_PROCESS_ON_UNHANDLED_EXCEPTION); - // Correlates every process that takes part in the same logical test run: the successive attempts launched by - // the retry orchestrator, or the modules of a single 'dotnet test' invocation. Report formats that model a - // logical run spanning several documents (CTRF 'runId') surface this value so a consumer can tie those - // documents back together. It may also be set externally to correlate shards running on different machines. + // Correlates the processes that make up one logical test run, so report formats that can describe a run + // spanning several documents (CTRF 'runId') can tie those documents back together. The retry orchestrator + // sets it before launching its attempts, and any process tree started from there inherits it. + // + // NOTE: a multi-project 'dotnet test' does NOT set this. Each module is a separate root test application + // with its own execution id (see docs/mstest-runner-protocol/004-protocol-dotnet-test-pipe.md), so its + // modules are distinct execution trees. Correlating them — or correlating shards running on different + // machines — requires setting this variable explicitly before launching them. public const string TESTINGPLATFORM_LOGICAL_RUN_ID = nameof(TESTINGPLATFORM_LOGICAL_RUN_ID); // Trx diff --git a/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/RetryFailedTestsTests.cs b/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/RetryFailedTestsTests.cs index e1b87d9ab7..05f22ed05d 100644 --- a/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/RetryFailedTestsTests.cs +++ b/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/RetryFailedTestsTests.cs @@ -551,6 +551,59 @@ public async Task RetryFailedTests_WithMinimumExpectedTests_StripsThresholdOnRet testHostResult.AssertOutputDoesNotContain("Minimum expected tests policy violation"); } + [TestMethod] + [DynamicData(nameof(TargetFrameworks.NetForDynamicData), typeof(TargetFrameworks))] + public async Task RetryFailedTests_CtrfReports_ShareRunIdButNotReportId(string tfm) + { + // Each attempt is a separate process that writes its own CTRF document, but together they are one + // logical run. Per ctrf-io/ctrf#58 those documents SHOULD share a `runId` while each stays a distinct + // artifact with its own `reportId`. This is the only test that exercises the cross-process contract: + // the engine unit tests mock IEnvironment, so they cannot observe the orchestrator's seeding. + var testHost = TestInfrastructure.TestHost.LocateFrom(AssetFixture.TargetAssetPath, AssetName, tfm); + string resultDirectory = Path.Combine(testHost.DirectoryName, Guid.NewGuid().ToString("N")); + + // METHOD1=1 makes TestMethod1 fail on the first attempt and pass on the second, so exactly two + // attempts run and each writes a CTRF report. + TestHostResult testHostResult = await testHost.ExecuteAsync( + $"--retry-failed-tests 3 --report-ctrf --results-directory {resultDirectory}", + new() + { + { EnvironmentVariableConstants.TESTINGPLATFORM_TELEMETRY_OPTOUT, "1" }, + { "METHOD1", "1" }, + { "RESULTDIR", resultDirectory }, + }, + cancellationToken: TestContext.CancellationToken); + + testHostResult.AssertExitCodeIs(ExitCode.Success); + testHostResult.AssertOutputContains("Retry summary: Passed! after 2/4 attempts"); + + // Attempts 1..N-1 stay under Retries//; only the final attempt's report is moved to the top level. + string[] ctrfFiles = + [ + .. Directory.GetFiles(resultDirectory, "*.ctrf.json", SearchOption.AllDirectories).OrderBy(f => f, StringComparer.Ordinal), + ]; + Assert.HasCount(2, ctrfFiles, $"Expected one CTRF report per attempt.{Environment.NewLine}{string.Join(Environment.NewLine, ctrfFiles)}"); + + string[] runIds = [.. ctrfFiles.Select(f => ReadRequiredStringProperty(f, "runId"))]; + string[] reportIds = [.. ctrfFiles.Select(f => ReadRequiredStringProperty(f, "reportId"))]; + + Assert.AreEqual(runIds[0], runIds[1], "Both attempts belong to the same logical run, so they must share a runId."); + Assert.AreNotEqual(reportIds[0], reportIds[1], "Each attempt is a distinct artifact, so it must have its own reportId."); + Assert.AreNotEqual(runIds[0], reportIds[0], "runId and reportId identify different things and must not be the same value."); + } + + private static string ReadRequiredStringProperty(string filePath, string propertyName) + { + using var document = System.Text.Json.JsonDocument.Parse(File.ReadAllText(filePath)); + Assert.IsTrue( + document.RootElement.TryGetProperty(propertyName, out System.Text.Json.JsonElement value), + $"'{propertyName}' is missing from '{filePath}'."); + + string? text = value.GetString(); + Assert.IsFalse(string.IsNullOrEmpty(text), $"'{propertyName}' must be a non-empty string in '{filePath}'."); + return text!; + } + public sealed class TestAssetFixture() : TestAssetFixtureBase() { public string TargetAssetPath => GetAssetPath(AssetName); @@ -558,7 +611,8 @@ public sealed class TestAssetFixture() : TestAssetFixtureBase() public override (string ID, string Name, string Code) GetAssetsToGenerate() => (AssetName, AssetName, TestCode .PatchTargetFrameworks(TargetFrameworks.All) - .PatchCodeWithReplace("$MicrosoftTestingPlatformVersion$", MicrosoftTestingPlatformVersion)); + .PatchCodeWithReplace("$MicrosoftTestingPlatformVersion$", MicrosoftTestingPlatformVersion) + .PatchCodeWithReplace("$MicrosoftTestingExtensionsCtrfReportVersion$", MicrosoftTestingExtensionsCtrfReportVersion)); private const string TestCode = """ #file RetryFailedTests.csproj @@ -574,6 +628,7 @@ public override (string ID, string Name, string Code) GetAssetsToGenerate() => ( + @@ -611,6 +666,9 @@ public static async Task Main(string[] args) (_,__) => new DummyTestFramework()); builder.AddCrashDumpProvider(); builder.AddTrxReportProvider(); +#pragma warning disable TPEXP // Type is for evaluation purposes only and is subject to change or removal in future updates. + builder.AddCtrfReportProvider(); +#pragma warning restore TPEXP builder.AddRetryProvider(); builder.AddMSBuild(); builder.AddTreeNodeFilterService(treeNodeFilterExtension); diff --git a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportEngineTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportEngineTests.cs index 54401573c8..e06892b89a 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportEngineTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportEngineTests.cs @@ -833,12 +833,13 @@ public async Task GenerateReportAsync_SingleLineOutput_EmitsOneArrayEntry() [TestMethod] public async Task GenerateReportAsync_RunId_UsesTheSharedLogicalRunId() { - // ctrf-io/ctrf#58: every process of one logical run (the attempts of --retry-failed-tests, the modules - // of one dotnet test invocation) must stamp the same runId, so consumers can tie those documents back - // together. The orchestrator publishes that id through this environment variable. + // ctrf-io/ctrf#58: every process of one logical run — notably the successive attempts of + // --retry-failed-tests — must stamp the same runId so consumers can tie those documents back + // together. The retry orchestrator publishes that id through this environment variable. using var memoryStream = new MemoryFileStream(); CtrfReportEngine engine = CreateEngine(memoryStream); _ = _environmentMock.Setup(x => x.GetEnvironmentVariable("TESTINGPLATFORM_LOGICAL_RUN_ID")).Returns("run-42"); + _ = _environmentMock.Setup(x => x.GetEnvironmentVariable("TESTINGPLATFORM_DOTNETTEST_EXECUTIONID")).Returns("execution-7"); await engine.GenerateReportAsync([Captured("p1", "Passing test", "passed")]); @@ -849,6 +850,9 @@ public async Task GenerateReportAsync_RunId_UsesTheSharedLogicalRunId() [TestMethod] public async Task GenerateReportAsync_RunId_FallsBackToTheDotnetTestExecutionId() { + // The execution id identifies this test application's own process tree. It is per root test application, + // not per 'dotnet test' invocation, so it correlates a module with its child processes — not with the + // sibling modules of a multi-project run, which legitimately report different logical runs. using var memoryStream = new MemoryFileStream(); CtrfReportEngine engine = CreateEngine(memoryStream); _ = _environmentMock.Setup(x => x.GetEnvironmentVariable("TESTINGPLATFORM_LOGICAL_RUN_ID")).Returns((string?)null); diff --git a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs index ef71412787..d76dde0e24 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs @@ -778,6 +778,66 @@ static JsonObject Named(string name, string status, string suite) Assert.AreEqual(1, (long)tests[1]!["retries"]!); } + [TestMethod] + [DataRow(false)] + [DataRow(true)] + public void Merge_RejectsMalformedTestRows_SoTheMergedDocumentStaysValid(bool collapseRetryAttempts) + { + // The mode is passed as a bool because CtrfMergeMode is internal and a test method must be public. + CtrfMergeMode mode = collapseRetryAttempts ? CtrfMergeMode.CollapseRetryAttempts : CtrfMergeMode.Concatenate; + // tests[] comes from an untrusted file. The CTRF schema types its items as Test objects, so carrying a + // bare string or a JSON null through would turn a defect localized to one input into an invalid MERGED + // document. Such rows are dropped — the same policy the merger already applies to a whole non-CTRF input + // — and both modes must agree on that, since the merged tests[] is the same array either way. + // `summary` is deliberately a string here too: `results.summary` is equally untrusted, and reading a + // property off a non-object node throws just as `tests[]` did. + string malformed = """ + {"reportFormat":"CTRF","specVersion":"0.0.0","results":{"summary":"broken","tests":["oops",null,42]}} + """; + string wellFormed = BuildReport(testEntries: [Attempt("t", "passed", uid: "u1")]); + + JsonNode results = JsonNode.Parse(CtrfReportMerger.Merge([malformed, wellFormed], mode))!["results"]!; + + var tests = (JsonArray)results["tests"]!; + Assert.HasCount(1, tests, "Only the real test survives."); + Assert.AreEqual("t", (string?)tests[0]!["name"]); + + // CTRF 8.1: summary.tests equals the tests[] length, and the status buckets must add back up to it — + // a dropped row must not leave a phantom entry in either the array or the counters. + JsonNode summary = results["summary"]!; + Assert.AreEqual(1, (long)summary["tests"]!); + Assert.AreEqual(1, (long)summary["passed"]!); + Assert.AreEqual(0, (long)summary["other"]!); + long bucketSum = (long)summary["passed"]! + (long)summary["failed"]! + (long)summary["skipped"]! + + (long)summary["pending"]! + (long)summary["other"]!; + Assert.AreEqual((long)summary["tests"]!, bucketSum, "Every counted test must land in exactly one status bucket."); + } + + [TestMethod] + public void Merge_CollapseRetryAttempts_ToleratesNonObjectExtra() + { + // `extra` is free-form, so a foreign producer may put a string or an array there. Identity resolution + // must fall back to the suite/name key instead of throwing while indexing it. + static JsonObject WithExtra(string status, JsonNode extra) + { + JsonObject test = Test("t", status); + test["extra"] = extra; + return test; + } + + string attempt1 = BuildReport(testEntries: [WithExtra("failed", "ci-run-14")]); + string attempt2 = BuildReport(testEntries: [WithExtra("passed", new JsonArray("a"))]); + + var tests = (JsonArray)JsonNode.Parse( + CtrfReportMerger.Merge([attempt1, attempt2], CtrfMergeMode.CollapseRetryAttempts))!["results"]!["tests"]!; + + // Both rows share the name identity, so they collapse even though neither carries a usable extra.uid. + Assert.HasCount(1, tests); + Assert.AreEqual("passed", (string?)tests[0]!["status"]); + Assert.AreEqual(1, (long)tests[0]!["retries"]!); + Assert.IsTrue((bool)tests[0]!["flaky"]!); + } + private static JsonObject Attempt(string name, string status, string uid, long duration = 1, string? message = null) { var test = new JsonObject From 69f5b9b366fb56f5ae52799c7d24fb4e096128c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Thu, 30 Jul 2026 14:28:48 +0200 Subject: [PATCH 03/11] Guard string reads in CtrfReportMerger against wrong-typed values Rejecting non-object `tests[]` elements proved each row is an object, but not that the values read out of it are strings. The explicit `(string?)` conversion on a `JsonNode` THROWS for a non-string value rather than yielding null, so a CTRF document that clears the merger's format gate could still crash it from four sites: the summary counter loop's `status` (both merge modes), a `suite` segment during identity resolution, and the `status` of a rebuilt attempt and of a collapsed row. Route these through a shared `ReadString` helper that treats a value of any other JSON type as absent, so an unreadable status classifies as `other` instead of aborting the merge. A non-string `suite` segment falls back to its JSON text, which keeps the identity key distinct and deterministic rather than silently fusing two different suites. The `reportFormat`, `runId`, `testId`, `extra.uid` and `name` reads already used the safe pattern and now share the helper, so the unsafe conversion has no remaining foothold in the file. Extend the malformed-row test with a numeric `status`, a `suite` holding a number and an object, and a nested `retryAttempts[]` entry with a numeric `status`. Reverting any of the four guards fails it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ad2ea8a-3349-4979-b27c-dd52bb3b7190 --- .../CtrfReportMerger.cs | 38 +++++++++++-------- .../CtrfReportMergerTests.cs | 29 ++++++++++---- 2 files changed, 44 insertions(+), 23 deletions(-) diff --git a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs index 0defed0db8..854fabcf18 100644 --- a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs +++ b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs @@ -110,7 +110,7 @@ internal static string Merge(IReadOnlyList inputReports, CtrfMergeMode m continue; } - string? format = root["reportFormat"] is JsonValue formatValue && formatValue.TryGetValue(out string? formatText) ? formatText : null; + string? format = ReadString(root, "reportFormat"); if (!string.Equals(format, "CTRF", StringComparison.OrdinalIgnoreCase)) { continue; @@ -120,10 +120,7 @@ internal static string Merge(IReadOnlyList inputReports, CtrfMergeMode m reportCount++; acceptedReports.Add(reportJson); - distinctRunIds.Add( - root["runId"] is JsonValue runIdValue && runIdValue.TryGetValue(out string? runIdText) && !RoslynString.IsNullOrEmpty(runIdText) - ? runIdText - : string.Empty); + distinctRunIds.Add(ReadString(root, "runId") is { Length: > 0 } runIdText ? runIdText : string.Empty); if (root["results"]?["environment"] is JsonObject environment) { @@ -213,7 +210,7 @@ internal static string Merge(IReadOnlyList inputReports, CtrfMergeMode m continue; } - switch ((string?)testObject["status"]) + switch (ReadString(testObject, "status")) { case "passed": passed++; break; case "failed": failed++; break; @@ -422,22 +419,19 @@ private static JsonArray CollapseRetryAttempts(JsonArray tests) /// private static string? GetTestIdentity(JsonObject test) { - if (test["testId"] is JsonValue testIdValue && testIdValue.TryGetValue(out string? testId) && !RoslynString.IsNullOrEmpty(testId)) + if (ReadString(test, "testId") is { Length: > 0 } testId) { return $"testId\u001f{testId}"; } // `extra` is free-form, so a foreign producer may well have written a string or an array there; indexing // anything but an object by property name throws. - if (test["extra"] is JsonObject extra - && extra["uid"] is JsonValue uidValue - && uidValue.TryGetValue(out string? uid) - && !RoslynString.IsNullOrEmpty(uid)) + if (test["extra"] is JsonObject extra && ReadString(extra, "uid") is { Length: > 0 } uid) { return $"uid\u001f{uid}"; } - if (test["name"] is not JsonValue nameValue || !nameValue.TryGetValue(out string? name) || RoslynString.IsNullOrEmpty(name)) + if (ReadString(test, "name") is not { Length: > 0 } name) { return null; } @@ -447,7 +441,13 @@ private static JsonArray CollapseRetryAttempts(JsonArray tests) { foreach (JsonNode? segment in suite) { - identity.Append('\u001f').Append((string?)segment); + // A suite segment is normally a string, but the document is untrusted. Fall back to the + // segment's JSON text so a non-string segment still contributes a distinct, deterministic part + // of the key instead of throwing on the string conversion. + identity.Append('\u001f').Append( + segment is JsonValue segmentValue && segmentValue.TryGetValue(out string? segmentText) + ? segmentText + : segment?.ToJsonString()); } } @@ -483,7 +483,7 @@ private static JsonNode BuildCollapsedTest(JsonObject final, List pr } attempt["attempt"] = i + 1; - anyFailed |= (string?)attempt["status"] == "failed"; + anyFailed |= ReadString(attempt, "status") == "failed"; } collapsed["retryAttempts"] = history; @@ -491,7 +491,7 @@ private static JsonNode BuildCollapsedTest(JsonObject final, List pr // CTRF 9.22: flaky only when the FINAL status is passed after at least one failed attempt. Recomputed // (rather than inherited) because an input's own flag only describes the attempt that produced it. - if ((string?)collapsed["status"] == "passed" && anyFailed) + if (ReadString(collapsed, "status") == "passed" && anyFailed) { collapsed["flaky"] = true; } @@ -570,6 +570,14 @@ private static JsonNode ToRetryAttempt(JsonObject test) return attempt; } + /// + /// Reads a string property, treating a value of any other JSON type as absent. The explicit + /// (string?) conversion on a THROWS for a non-string value rather than + /// yielding , and every document reaching the merger is untrusted. + /// + private static string? ReadString(JsonObject owner, string propertyName) + => owner[propertyName] is JsonValue value && value.TryGetValue(out string? text) ? text : null; + /// /// Reads an integral property from a JSON node, tolerating anything the node may actually be. The node comes /// straight out of an untrusted document — results.summary is not guaranteed to be an object — so a diff --git a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs index d76dde0e24..cc35db1b47 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs @@ -790,24 +790,37 @@ public void Merge_RejectsMalformedTestRows_SoTheMergedDocumentStaysValid(bool co // document. Such rows are dropped — the same policy the merger already applies to a whole non-CTRF input // — and both modes must agree on that, since the merged tests[] is the same array either way. // `summary` is deliberately a string here too: `results.summary` is equally untrusted, and reading a - // property off a non-object node throws just as `tests[]` did. + // property off a non-object node throws just as `tests[]` did. The well-formed-looking rows carry + // wrong-typed VALUES — a numeric `status`, a numeric `suite` segment, a numeric `status` inside a nested + // `retryAttempts[]` entry — because the explicit (string?) conversion on a JsonNode throws for a + // non-string value instead of yielding null. string malformed = """ - {"reportFormat":"CTRF","specVersion":"0.0.0","results":{"summary":"broken","tests":["oops",null,42]}} + {"reportFormat":"CTRF","specVersion":"0.0.0","results":{"summary":"broken","tests":[ + "oops", + null, + 42, + {"name":"numeric status","status":7}, + {"name":"numeric suite","status":"failed","suite":["A",7,{"x":1}]}, + {"name":"nested","status":"passed","extra":{"uid":"n1"},"retryAttempts":[{"attempt":1,"status":3}]} + ]}} """; string wellFormed = BuildReport(testEntries: [Attempt("t", "passed", uid: "u1")]); JsonNode results = JsonNode.Parse(CtrfReportMerger.Merge([malformed, wellFormed], mode))!["results"]!; var tests = (JsonArray)results["tests"]!; - Assert.HasCount(1, tests, "Only the real test survives."); - Assert.AreEqual("t", (string?)tests[0]!["name"]); + Assert.HasCount(4, tests, "The three non-object elements are dropped; wrong-typed values are tolerated."); + Assert.AreEqual("numeric status", (string?)tests[0]!["name"]); + Assert.AreEqual("t", (string?)tests[3]!["name"]); // CTRF 8.1: summary.tests equals the tests[] length, and the status buckets must add back up to it — - // a dropped row must not leave a phantom entry in either the array or the counters. + // a dropped row must not leave a phantom entry in either the array or the counters. An unreadable + // status is classified as 'other' rather than crashing the merge. JsonNode summary = results["summary"]!; - Assert.AreEqual(1, (long)summary["tests"]!); - Assert.AreEqual(1, (long)summary["passed"]!); - Assert.AreEqual(0, (long)summary["other"]!); + Assert.AreEqual(4, (long)summary["tests"]!); + Assert.AreEqual(2, (long)summary["passed"]!); + Assert.AreEqual(1, (long)summary["failed"]!); + Assert.AreEqual(1, (long)summary["other"]!, "A non-string status is unclassifiable."); long bucketSum = (long)summary["passed"]! + (long)summary["failed"]! + (long)summary["skipped"]! + (long)summary["pending"]! + (long)summary["other"]!; Assert.AreEqual((long)summary["tests"]!, bucketSum, "Every counted test must land in exactly one status bucket."); From 6f1741d88eb63984b0fc98c0add88a14608f9d56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Thu, 30 Jul 2026 16:46:11 +0200 Subject: [PATCH 04/11] Give each merge mode its own reportId, and cover all runId branches Both merge modes derived `reportId` from the accepted inputs alone, so merging the same inputs concatenated and collapsed produced two materially different documents under one id. CTRF 5.3 wants a distinct `reportId` for a materially changed document, so fold the mode into the derivation. Determinism per mode is unchanged: the same inputs merged the same way still reproduce the same id. The acceptance test only exercised the fresh-GUID branch of the orchestrator's run-id resolution, so neither an explicitly supplied correlation id being preserved nor the dotnet test execution id being adopted was covered. Parameterize it over all three branches and assert the expected id, not just that the attempts agree. Making the orchestrator overwrite the incoming id now fails four of the six cases. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ad2ea8a-3349-4979-b27c-dd52bb3b7190 --- .../CtrfReportMerger.cs | 24 +++++---- .../RetryFailedTestsTests.cs | 49 ++++++++++++++++--- .../CtrfReportMergerTests.cs | 26 ++++++++++ 3 files changed, 82 insertions(+), 17 deletions(-) diff --git a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs index 854fabcf18..c98795670c 100644 --- a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs +++ b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs @@ -17,7 +17,7 @@ namespace Microsoft.Testing.Extensions.CtrfReport; /// /// results.tests[] arrays are concatenated as-is, unless asks for successive attempts of the same test to be folded into one row. /// results.summary counters are re-derived by counting the merged tests[] (so summary.tests always matches the array length); start/stop use the earliest/latest across inputs, duration is the resulting span. -/// reportFormat and specVersion are taken from the first report; reportId is derived deterministically from the inputs, so identical inputs reproduce the same id (RFC 018 idempotency). +/// reportFormat and specVersion are taken from the first report; reportId is derived deterministically from the inputs AND the merge mode, so identical inputs reproduce the same id (RFC 018 idempotency) while the two modes — which produce materially different documents — get distinct ids. /// runId is carried over when every input agrees on one, because the merged document describes the same logical run as its inputs while remaining a distinct artifact with its own reportId (see ctrf-io/ctrf#58). /// tool keeps a concrete identity only when every input reported the exact same tool object; otherwise (inputs disagree or any input omits it) a neutral merger identity is used, so one framework is not attributed to another's tests. /// environment keeps the first report's shared fields, but module-specific values under extra (testApplication, exitCode) are dropped rather than presented as describing all merged modules. @@ -269,7 +269,7 @@ internal static string Merge(IReadOnlyList inputReports, CtrfMergeMode m { ["reportFormat"] = first["reportFormat"]?.DeepClone() ?? "CTRF", ["specVersion"] = first["specVersion"]?.DeepClone() ?? "0.0.0", - ["reportId"] = CreateDeterministicReportId(acceptedReports), + ["reportId"] = CreateDeterministicReportId(acceptedReports, mode), }; // A merged document is a new artifact (hence its own reportId) but it still describes the same logical @@ -664,19 +664,25 @@ private static bool TryReadLong(JsonNode node, string propertyName, out long val } /// - /// Derives a stable reportId from the accepted CTRF input reports so identical inputs reproduce - /// the same id on every retry (RFC 018 idempotency) without a random source or reusing an input report's - /// id. Only the payloads that passed CTRF validation are hashed, so a rejected non-CTRF input cannot - /// alter the merged report's identity. A non-cryptographic 128-bit FNV-1a fill is sufficient here — the - /// id only needs to be deterministic and collision-resistant enough to identify a merged report, not - /// secret. + /// Derives a stable reportId from the accepted CTRF input reports and the merge mode, so identical + /// inputs merged the same way reproduce the same id on every retry (RFC 018 idempotency) without a random + /// source or reusing an input report's id, while the same inputs merged a DIFFERENT way — which yields a + /// materially different document — get a distinct id, as CTRF 5.3 requires. Only the payloads that passed + /// CTRF validation are hashed, so a rejected non-CTRF input cannot alter the merged report's identity. A + /// non-cryptographic 128-bit FNV-1a fill is sufficient here — the id only needs to be deterministic and + /// collision-resistant enough to identify a merged report, not secret. /// - private static string CreateDeterministicReportId(IReadOnlyList acceptedReports) + private static string CreateDeterministicReportId(IReadOnlyList acceptedReports, CtrfMergeMode mode) { const ulong fnvPrime = 1099511628211UL; ulong hashLow = 14695981039346656037UL; ulong hashHigh = 0x9E3779B97F4A7C15UL; + // Fold in the merge mode: the same inputs concatenated and collapsed are two materially different + // documents, and CTRF 5.3 wants a distinct reportId for each rather than one id naming both. + hashLow = (hashLow ^ (ulong)mode) * fnvPrime; + hashHigh = (hashHigh ^ ((ulong)mode + 1UL)) * fnvPrime; + foreach (string report in acceptedReports) { foreach (char c in report) diff --git a/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/RetryFailedTestsTests.cs b/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/RetryFailedTestsTests.cs index 05f22ed05d..6cb66202e4 100644 --- a/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/RetryFailedTestsTests.cs +++ b/test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/RetryFailedTestsTests.cs @@ -551,9 +551,22 @@ public async Task RetryFailedTests_WithMinimumExpectedTests_StripsThresholdOnRet testHostResult.AssertOutputDoesNotContain("Minimum expected tests policy violation"); } + internal static IEnumerable<(string Tfm, string? SeededVariable)> GetRunIdMatrix() + { + foreach (string tfm in TargetFrameworks.Net) + { + // The orchestrator resolves the logical run id as: an explicitly set id wins, else the dotnet test + // execution id (which already identifies this test application's process tree), else a fresh one. + // Exercise all three branches. + yield return (tfm, null); + yield return (tfm, EnvironmentVariableConstants.TESTINGPLATFORM_LOGICAL_RUN_ID); + yield return (tfm, EnvironmentVariableConstants.TESTINGPLATFORM_DOTNETTEST_EXECUTIONID); + } + } + [TestMethod] - [DynamicData(nameof(TargetFrameworks.NetForDynamicData), typeof(TargetFrameworks))] - public async Task RetryFailedTests_CtrfReports_ShareRunIdButNotReportId(string tfm) + [DynamicData(nameof(GetRunIdMatrix))] + public async Task RetryFailedTests_CtrfReports_ShareRunIdButNotReportId(string tfm, string? seededVariable) { // Each attempt is a separate process that writes its own CTRF document, but together they are one // logical run. Per ctrf-io/ctrf#58 those documents SHOULD share a `runId` while each stays a distinct @@ -564,14 +577,25 @@ public async Task RetryFailedTests_CtrfReports_ShareRunIdButNotReportId(string t // METHOD1=1 makes TestMethod1 fail on the first attempt and pass on the second, so exactly two // attempts run and each writes a CTRF report. + Dictionary environmentVariables = new() + { + { EnvironmentVariableConstants.TESTINGPLATFORM_TELEMETRY_OPTOUT, "1" }, + { "METHOD1", "1" }, + { "RESULTDIR", resultDirectory }, + }; + + // When a correlation id is supplied from outside, the attempts must adopt THAT id rather than minting + // their own — that is what lets a CI job tie several modules or machines into one logical run. + string? expectedRunId = null; + if (seededVariable is not null) + { + expectedRunId = $"seeded-{Guid.NewGuid():N}"; + environmentVariables[seededVariable] = expectedRunId; + } + TestHostResult testHostResult = await testHost.ExecuteAsync( $"--retry-failed-tests 3 --report-ctrf --results-directory {resultDirectory}", - new() - { - { EnvironmentVariableConstants.TESTINGPLATFORM_TELEMETRY_OPTOUT, "1" }, - { "METHOD1", "1" }, - { "RESULTDIR", resultDirectory }, - }, + environmentVariables, cancellationToken: TestContext.CancellationToken); testHostResult.AssertExitCodeIs(ExitCode.Success); @@ -590,6 +614,15 @@ .. Directory.GetFiles(resultDirectory, "*.ctrf.json", SearchOption.AllDirectorie Assert.AreEqual(runIds[0], runIds[1], "Both attempts belong to the same logical run, so they must share a runId."); Assert.AreNotEqual(reportIds[0], reportIds[1], "Each attempt is a distinct artifact, so it must have its own reportId."); Assert.AreNotEqual(runIds[0], reportIds[0], "runId and reportId identify different things and must not be the same value."); + + if (expectedRunId is not null) + { + Assert.AreEqual(expectedRunId, runIds[0], $"'{seededVariable}' must be honored instead of minting a new run id."); + } + else + { + Assert.IsTrue(Guid.TryParse(runIds[0], out _), $"An uncorrelated run must mint a GUID run id, got '{runIds[0]}'."); + } } private static string ReadRequiredStringProperty(string filePath, string propertyName) diff --git a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs index cc35db1b47..c92811d761 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs @@ -851,6 +851,32 @@ static JsonObject WithExtra(string status, JsonNode extra) Assert.IsTrue((bool)tests[0]!["flaky"]!); } + [TestMethod] + public void Merge_ReportId_IsDeterministicPerModeAndDiffersBetweenModes() + { + // The two modes turn the same inputs into materially different documents (three rows vs one collapsed + // row here), so CTRF 5.3 requires each to get its own reportId — one id must not name both artifacts. + // Determinism per mode (RFC 018 idempotency) must survive that. + string attempt1 = BuildReport(testEntries: [Attempt("t", "failed", uid: "u1")]); + string attempt2 = BuildReport(testEntries: [Attempt("t", "passed", uid: "u1")]); + string[] inputs = [attempt1, attempt2]; + + string concatenated = (string)JsonNode.Parse(CtrfReportMerger.Merge(inputs, CtrfMergeMode.Concatenate))!["reportId"]!; + string collapsed = (string)JsonNode.Parse(CtrfReportMerger.Merge(inputs, CtrfMergeMode.CollapseRetryAttempts))!["reportId"]!; + + Assert.AreNotEqual(concatenated, collapsed, "Two materially different merged documents must not share a reportId."); + + // Re-merging the same inputs the same way reproduces the id. + Assert.AreEqual(concatenated, (string)JsonNode.Parse(CtrfReportMerger.Merge(inputs, CtrfMergeMode.Concatenate))!["reportId"]!); + Assert.AreEqual(collapsed, (string)JsonNode.Parse(CtrfReportMerger.Merge(inputs, CtrfMergeMode.CollapseRetryAttempts))!["reportId"]!); + + // The default overload keeps concatenating, so it must agree with the explicit Concatenate mode. + Assert.AreEqual(concatenated, (string)JsonNode.Parse(CtrfReportMerger.Merge(inputs))!["reportId"]!); + + // CTRF 5.3: reportId MUST be a valid UUID when present. + Assert.IsTrue(Guid.TryParse(collapsed, out _), $"reportId must be a UUID, got '{collapsed}'."); + } + private static JsonObject Attempt(string name, string status, string uid, long duration = 1, string? message = null) { var test = new JsonObject From d8d607c7ac5d8ca9dcc56bb0b76079d46fd38544 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Thu, 30 Jul 2026 17:07:20 +0200 Subject: [PATCH 05/11] Isolate acceptance tests from an ambient logical run id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `TestHost.ExecuteAsync` copies the parent process environment into each child test host except for the variables in `ToSkipEnvironmentVariables`, and `TESTINGPLATFORM_LOGICAL_RUN_ID` was not among them. That makes the new run-id acceptance test observe an id it did not choose whenever the variable happens to be set in the developer's shell or the CI job — which is precisely the usage this PR documents for correlating several modules or machines, so adopting the feature would have broken the repo's own tests. Skip it, next to `TESTINGPLATFORM_DOTNETTEST_EXECUTIONID`, which is already skipped for the same reason. The two seeded cases are unaffected: they inject the variable through the explicit dictionary, which wins over the ambient copy. Verified by setting a non-GUID value in the parent environment: without the entry it fails 4 of the 6 matrix cases — the uncorrelated case plus both execution-id cases, since an ambient logical run id also outranks the seeded execution id — and passes all 6 with it. Also assert that suppressing `runId` leaves the merged document's own `reportId` intact, which the run-id omission tests did not cover. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ad2ea8a-3349-4979-b27c-dd52bb3b7190 --- .../CtrfReportMergerTests.cs | 4 ++++ .../WellKnownEnvironmentVariables.cs | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs index c92811d761..970cee9dcd 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs @@ -575,6 +575,9 @@ public void Merge_OmitsRunId_WhenInputsBelongToDifferentRuns() JsonNode merged = JsonNode.Parse(CtrfReportMerger.Merge([a, b]))!; Assert.IsNull(merged["runId"]); + + // Suppressing the run correlation must not suppress the merged document's own identity. + Assert.IsNotNull((string?)merged["reportId"]); } [TestMethod] @@ -588,6 +591,7 @@ public void Merge_OmitsRunId_WhenAnInputHasNone() JsonNode merged = JsonNode.Parse(CtrfReportMerger.Merge([a, b]))!; Assert.IsNull(merged["runId"]); + Assert.IsNotNull((string?)merged["reportId"]); } [TestMethod] diff --git a/test/Utilities/Microsoft.Testing.TestInfrastructure/WellKnownEnvironmentVariables.cs b/test/Utilities/Microsoft.Testing.TestInfrastructure/WellKnownEnvironmentVariables.cs index cf618db08d..5e09a81d42 100644 --- a/test/Utilities/Microsoft.Testing.TestInfrastructure/WellKnownEnvironmentVariables.cs +++ b/test/Utilities/Microsoft.Testing.TestInfrastructure/WellKnownEnvironmentVariables.cs @@ -98,6 +98,11 @@ public static class WellKnownEnvironmentVariables "TESTINGPLATFORM_DOTNETTEST_EXECUTIONID", "DOTNET_CLI_TEST_COMMAND_WORKING_DIRECTORY", + // Logical run correlation. A CI job may set this to tie several modules or machines into one + // logical run, so it must not bleed into child test hosts and make a test observe an id it did + // not choose. Tests that exercise run correlation inject it explicitly. + "TESTINGPLATFORM_LOGICAL_RUN_ID", + // Isolate from the skip banner in case of parent, children tests "TESTINGPLATFORM_CONSOLEOUTPUTDEVICE_SKIP_BANNER", From 36a9dcd3b164226e20c84bfc7c3891fb10160fae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Thu, 30 Jul 2026 17:10:56 +0200 Subject: [PATCH 06/11] Assert reportId is distinct from a correlated runId Pins that a supplied correlation id does not leak into the document's own artifact id on the two seeded resolution paths. The generated-id test already checked this for the uncorrelated path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ad2ea8a-3349-4979-b27c-dd52bb3b7190 --- .../CtrfReportEngineTests.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportEngineTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportEngineTests.cs index e06892b89a..37060a19b7 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportEngineTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportEngineTests.cs @@ -845,6 +845,9 @@ public async Task GenerateReportAsync_RunId_UsesTheSharedLogicalRunId() using var document = JsonDocument.Parse(memoryStream.GetUtf8Content()); Assert.AreEqual("run-42", document.RootElement.GetProperty("runId").GetString()); + + // The run and the document are different things: a correlated run must not leak into the artifact id. + Assert.AreNotEqual("run-42", document.RootElement.GetProperty("reportId").GetString()); } [TestMethod] @@ -862,6 +865,7 @@ public async Task GenerateReportAsync_RunId_FallsBackToTheDotnetTestExecutionId( using var document = JsonDocument.Parse(memoryStream.GetUtf8Content()); Assert.AreEqual("execution-7", document.RootElement.GetProperty("runId").GetString()); + Assert.AreNotEqual("execution-7", document.RootElement.GetProperty("reportId").GetString()); } [TestMethod] From 05a31e9c753e34016cd348cb8c625e1ff811b587 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Thu, 30 Jul 2026 17:26:38 +0200 Subject: [PATCH 07/11] Make the collapse identity key unambiguous The suite/name fallback key separated its components without escaping them, but a CTRF `name` and suite segment are arbitrary non-empty strings that may contain the separator. So `suite: ["A"], name: "B\u001fC"` and `suite: ["A", "B"], name: "C"` flattened to the same key, and collapse mode fused two unrelated tests into one, silently dropping a result. Length-prefix each component so the encoding is unambiguous whatever the components contain. The `testId` and `extra.uid` keys are single-component and carry distinct literal prefixes, so they were never ambiguous. Also correct the class summary: `Concatenate` no longer combines `tests[]` entirely as-is, since elements that are not Test objects are dropped during ingestion. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ad2ea8a-3349-4979-b27c-dd52bb3b7190 --- .../CtrfReportMerger.cs | 18 ++++++++++-- .../CtrfReportMergerTests.cs | 29 +++++++++++++++++++ 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs index c98795670c..1464ac5743 100644 --- a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs +++ b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs @@ -15,7 +15,7 @@ namespace Microsoft.Testing.Extensions.CtrfReport; /// This is a pure, invocation-agnostic JSON-level merge (no I/O, no clock) that mirrors the /// TRX and JUnit mergers, demonstrating that the same post-processing shape fits a JSON format: /// -/// results.tests[] arrays are concatenated as-is, unless asks for successive attempts of the same test to be folded into one row. +/// results.tests[] arrays are concatenated, except that elements which are not Test objects are dropped so one input's malformed row cannot invalidate the merged document; additionally folds successive attempts of the same test into one row. /// results.summary counters are re-derived by counting the merged tests[] (so summary.tests always matches the array length); start/stop use the earliest/latest across inputs, duration is the resulting span. /// reportFormat and specVersion are taken from the first report; reportId is derived deterministically from the inputs AND the merge mode, so identical inputs reproduce the same id (RFC 018 idempotency) while the two modes — which produce materially different documents — get distinct ids. /// runId is carried over when every input agrees on one, because the merged document describes the same logical run as its inputs while remaining a distinct artifact with its own reportId (see ctrf-io/ctrf#58). @@ -417,6 +417,13 @@ private static JsonArray CollapseRetryAttempts(JsonArray tests) /// producer-supplied extra.uid, and finally the suite path plus name. Returns /// when the row carries none of them. /// + /// + /// The suite/name fallback length-prefixes every component rather than just separating them. A CTRF + /// name or suite segment is an arbitrary non-empty string, so it may itself contain the separator: + /// with plain separation, suite: ["A"], name: "B\u001fC" and suite: ["A", "B"], name: "C" + /// would produce the same key and collapse two unrelated tests into one, silently dropping a result. + /// Length prefixes make the encoding unambiguous whatever the components contain. + /// private static string? GetTestIdentity(JsonObject test) { if (ReadString(test, "testId") is { Length: > 0 } testId) @@ -444,16 +451,21 @@ private static JsonArray CollapseRetryAttempts(JsonArray tests) // A suite segment is normally a string, but the document is untrusted. Fall back to the // segment's JSON text so a non-string segment still contributes a distinct, deterministic part // of the key instead of throwing on the string conversion. - identity.Append('\u001f').Append( + AppendIdentityComponent( + identity, segment is JsonValue segmentValue && segmentValue.TryGetValue(out string? segmentText) ? segmentText : segment?.ToJsonString()); } } - return identity.Append('\u001f').Append(name).ToString(); + AppendIdentityComponent(identity, name); + return identity.ToString(); } + private static void AppendIdentityComponent(StringBuilder identity, string? component) + => identity.Append('\u001f').Append(component?.Length ?? -1).Append(':').Append(component); + private static JsonNode BuildCollapsedTest(JsonObject final, List priors) { var collapsed = (JsonObject)final.DeepClone(); diff --git a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs index 970cee9dcd..ea4cdb2248 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs @@ -881,6 +881,35 @@ public void Merge_ReportId_IsDeterministicPerModeAndDiffersBetweenModes() Assert.IsTrue(Guid.TryParse(collapsed, out _), $"reportId must be a UUID, got '{collapsed}'."); } + [TestMethod] + public void Merge_CollapseRetryAttempts_DoesNotFuseTestsWhoseNameContainsTheIdentitySeparator() + { + // A CTRF `name` is an arbitrary non-empty string, so it may contain whatever character the identity key + // uses as a separator. With plain separation, suite ["A"] + name "B\u001fC" and suite ["A","B"] + name + // "C" flatten to the same key, which would fuse two unrelated tests and silently drop a result. + static JsonObject Named(string status, string name, params string[] suite) + { + JsonObject test = Test(name, status); + test["suite"] = new JsonArray([.. suite.Select(s => (JsonNode)JsonValue.Create(s)!)]); + return test; + } + + string report = BuildReport(testEntries: + [ + Named("failed", "B\u001fC", "A"), + Named("passed", "C", "A", "B"), + ]); + + var tests = (JsonArray)JsonNode.Parse( + CtrfReportMerger.Merge([report], CtrfMergeMode.CollapseRetryAttempts))!["results"]!["tests"]!; + + Assert.HasCount(2, tests, "Two distinct tests must not collapse into one."); + Assert.AreEqual("failed", (string?)tests[0]!["status"]); + Assert.AreEqual("passed", (string?)tests[1]!["status"]); + Assert.IsNull(tests[0]!["retries"], "Neither row is a retry of the other."); + Assert.IsNull(tests[1]!["retries"]); + } + private static JsonObject Attempt(string name, string status, string uid, long duration = 1, string? message = null) { var test = new JsonObject From 620f652d8d55fc40c6a348978ffd6eac21c16ce7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Thu, 30 Jul 2026 17:41:29 +0200 Subject: [PATCH 08/11] Honor the legacy id and discriminate ambiguous fallback rows Two gaps in the collapse identity, both of which silently lose a result by fusing unrelated tests. CTRF 9.1 defines `id` as a stable test-case identifier that consumers treat as legacy, preferring `testId` only when both are present. It was not consulted at all, so an id-only report fell through to the suite/name heuristic and distinct same-named tests could be fused. Check it after `testId`. Suite plus name is also not unique by itself: parameterized rows share both while differing only in `parameters` (9.30), and same-named tests in different files differ only by `filePath` (9.19). Fold both into the fallback key. Should a producer serialize `parameters` inconsistently between attempts, the effect is an unfolded retry -- a duplicated row rather than a lost result, which is the safe direction to fail in. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ad2ea8a-3349-4979-b27c-dd52bb3b7190 --- .../CtrfReportMerger.cs | 36 ++++++++-- .../CtrfReportMergerTests.cs | 65 +++++++++++++++++++ 2 files changed, 94 insertions(+), 7 deletions(-) diff --git a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs index 1464ac5743..4f15762562 100644 --- a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs +++ b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs @@ -414,15 +414,30 @@ private static JsonArray CollapseRetryAttempts(JsonArray tests) /// /// Computes the key identifying the logical test a row describes, preferring the CTRF testId, then the - /// producer-supplied extra.uid, and finally the suite path plus name. Returns - /// when the row carries none of them. + /// legacy id, then the producer-supplied extra.uid, and finally the suite path plus name and the + /// other stable discriminators the Test object offers. Returns when the row carries + /// none of them. /// /// - /// The suite/name fallback length-prefixes every component rather than just separating them. A CTRF - /// name or suite segment is an arbitrary non-empty string, so it may itself contain the separator: - /// with plain separation, suite: ["A"], name: "B\u001fC" and suite: ["A", "B"], name: "C" - /// would produce the same key and collapse two unrelated tests into one, silently dropping a result. - /// Length prefixes make the encoding unambiguous whatever the components contain. + /// + /// The identifier order follows CTRF 9.1: id is a stable test-case identifier that consumers treat as + /// legacy, using testId in preference only when both are present. Ignoring an id-only report + /// would drop it to the heuristic fallback and risk fusing distinct same-named tests. + /// + /// + /// The fallback length-prefixes every component rather than just separating them. A CTRF name or suite + /// segment is an arbitrary non-empty string, so it may itself contain the separator: with plain separation, + /// suite: ["A"], name: "B\u001fC" and suite: ["A", "B"], name: "C" would produce the same key + /// and collapse two unrelated tests into one, silently dropping a result. Length prefixes make the encoding + /// unambiguous whatever the components contain. + /// + /// + /// The fallback also folds in filePath (9.19) and parameters (9.30), because suite plus name + /// alone is not unique: parameterized rows share both while differing only in their parameters, and + /// same-named tests in different files differ only by path. If a producer were to serialize parameters + /// inconsistently between attempts, the effect is that a retry is not folded — a duplicated row rather than a + /// lost result, which is the safe direction to fail in. + /// /// private static string? GetTestIdentity(JsonObject test) { @@ -431,6 +446,11 @@ private static JsonArray CollapseRetryAttempts(JsonArray tests) return $"testId\u001f{testId}"; } + if (ReadString(test, "id") is { Length: > 0 } id) + { + return $"id\u001f{id}"; + } + // `extra` is free-form, so a foreign producer may well have written a string or an array there; indexing // anything but an object by property name throws. if (test["extra"] is JsonObject extra && ReadString(extra, "uid") is { Length: > 0 } uid) @@ -460,6 +480,8 @@ private static JsonArray CollapseRetryAttempts(JsonArray tests) } AppendIdentityComponent(identity, name); + AppendIdentityComponent(identity, ReadString(test, "filePath")); + AppendIdentityComponent(identity, test["parameters"]?.ToJsonString()); return identity.ToString(); } diff --git a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs index ea4cdb2248..6f1292d547 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs @@ -910,6 +910,71 @@ static JsonObject Named(string status, string name, params string[] suite) Assert.IsNull(tests[1]!["retries"]); } + [TestMethod] + public void Merge_CollapseRetryAttempts_UsesLegacyIdWhenTestIdIsAbsent() + { + // CTRF 9.1: `id` is a stable test-case identifier that consumers treat as legacy, preferring `testId` + // only when both are present. An id-only report must therefore collapse on it rather than dropping to + // the suite/name heuristic, which would fuse these two distinct same-named tests. + static JsonObject WithId(string id, string status) + { + JsonObject test = Test("same name", status); + test["id"] = id; + return test; + } + + string attempt1 = BuildReport(testEntries: [WithId("id-1", "failed"), WithId("id-2", "failed")]); + string attempt2 = BuildReport(testEntries: [WithId("id-1", "passed"), WithId("id-2", "failed")]); + + var tests = (JsonArray)JsonNode.Parse( + CtrfReportMerger.Merge([attempt1, attempt2], CtrfMergeMode.CollapseRetryAttempts))!["results"]!["tests"]!; + + Assert.HasCount(2, tests, "Two distinct ids must stay two tests."); + Assert.AreEqual("passed", (string?)tests[0]!["status"]); + Assert.IsTrue((bool)tests[0]!["flaky"]!); + Assert.AreEqual("failed", (string?)tests[1]!["status"]); + Assert.AreEqual(1, (long)tests[1]!["retries"]!); + } + + [TestMethod] + public void Merge_CollapseRetryAttempts_DoesNotFuseRowsThatDifferOnlyByParametersOrFilePath() + { + // Suite plus name is not unique on its own: parameterized rows share both while differing in their + // parameters (CTRF 9.30), and same-named tests in different files differ only by path (9.19). Fusing + // them would silently drop a result. + static JsonObject Row(string status, JsonNode? parameters, string? filePath) + { + JsonObject test = Test("same name", status); + if (parameters is not null) + { + test["parameters"] = parameters; + } + + if (filePath is not null) + { + test["filePath"] = filePath; + } + + return test; + } + + string report = BuildReport(testEntries: + [ + Row("failed", new JsonObject { ["value"] = 1 }, null), + Row("passed", new JsonObject { ["value"] = 2 }, null), + Row("skipped", null, "a.cs"), + Row("passed", null, "b.cs"), + ]); + + var tests = (JsonArray)JsonNode.Parse( + CtrfReportMerger.Merge([report], CtrfMergeMode.CollapseRetryAttempts))!["results"]!["tests"]!; + + Assert.HasCount(4, tests, "Rows differing only by parameters or filePath are distinct tests."); + Assert.AreSequenceEqual( + (string?[])["failed", "passed", "skipped", "passed"], + tests.Select(t => (string?)t!["status"]).ToArray()); + } + private static JsonObject Attempt(string name, string status, string uid, long duration = 1, string? message = null) { var test = new JsonObject From 2149871b346171388c19490c2a211f77037de248 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Thu, 30 Jul 2026 17:55:20 +0200 Subject: [PATCH 09/11] Keep the merged retry history within the section 11 schema Two ways a foreign document could put schema-invalid content into the merged retry history. A promoted test row copied `status` verbatim, so a row carrying `"status": 7` emitted an attempt with a numeric status even though CTRF 11.3 constrains it to five string values. Normalize anything outside that vocabulary -- wrong-typed, or a status the producer invented -- to `other`, which is both legal and the bucket the summary already counts it in. Attempts an input had already nested were cloned wholesale, bypassing the section 11 projection entirely, so a numeric nested status or any test-only field survived into the merged history despite `additionalProperties: false`. Route them through the same projection as a promoted row, which applies the whitelist and the status normalization. Nothing our own engine writes is lost: every field it emits on a nested attempt is either in the whitelist or inside `extra`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ad2ea8a-3349-4979-b27c-dd52bb3b7190 --- .../CtrfReportMerger.cs | 23 ++++++++--- .../CtrfReportMergerTests.cs | 41 +++++++++++++++++++ 2 files changed, 59 insertions(+), 5 deletions(-) diff --git a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs index 4f15762562..174de2782e 100644 --- a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs +++ b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs @@ -552,9 +552,10 @@ private static void AppendAttempts(JsonArray history, JsonObject test) } /// - /// Copies retry attempt objects an input already recorded into the history being built, skipping entries that - /// are not objects: a element could not describe an execution and is not a valid retry - /// attempt object. + /// Copies the retry attempts an input already recorded into the history being built, projecting each through + /// the same section 11 shaping as a promoted test row so a foreign producer's nested attempt cannot smuggle a + /// wrong-typed status or a disallowed field into the merged history. Entries that are not objects are skipped: + /// they could not describe an execution. /// private static void AppendNestedAttempts(JsonArray history, JsonArray nested) { @@ -562,7 +563,7 @@ private static void AppendNestedAttempts(JsonArray history, JsonArray nested) { if (attempt is JsonObject attemptObject) { - history.Add(attemptObject.DeepClone()); + history.Add(ToRetryAttempt(attemptObject)); } } } @@ -578,7 +579,7 @@ private static JsonNode ToRetryAttempt(JsonObject test) var attempt = new JsonObject { ["attempt"] = 1, - ["status"] = test["status"]?.DeepClone() ?? "other", + ["status"] = ReadStatus(test), }; foreach (string field in RetryAttemptFields) @@ -612,6 +613,18 @@ private static JsonNode ToRetryAttempt(JsonObject test) private static string? ReadString(JsonObject owner, string propertyName) => owner[propertyName] is JsonValue value && value.TryGetValue(out string? text) ? text : null; + /// + /// Reads a CTRF status, normalizing anything outside the vocabulary section 11.3 allows — a wrong-typed + /// value, or a status this producer invented — to other. Copying such a value verbatim into a retry + /// attempt would make the merged document schema-invalid, and other is both a legal status and the + /// bucket the summary already counts it in. + /// + private static string ReadStatus(JsonObject owner) + { + string? status = ReadString(owner, "status"); + return status is "passed" or "failed" or "skipped" or "pending" or "other" ? status : "other"; + } + /// /// Reads an integral property from a JSON node, tolerating anything the node may actually be. The node comes /// straight out of an untrusted document — results.summary is not guaranteed to be an object — so a diff --git a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs index 6f1292d547..e0e0407c92 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs @@ -975,6 +975,47 @@ static JsonObject Row(string status, JsonNode? parameters, string? filePath) tests.Select(t => (string?)t!["status"]).ToArray()); } + [TestMethod] + public void Merge_CollapseRetryAttempts_KeepsRetryHistorySchemaValid() + { + // CTRF section 11 constrains a retry attempt: `status` must be one of five values, and no unknown field + // may appear outside `extra`. Both a promoted test row and an attempt an input already nested must be + // shaped to that, otherwise one foreign document makes the merged history schema-invalid. + JsonObject firstAttempt = Test("t", "failed"); + firstAttempt["extra"] = new JsonObject { ["uid"] = "u1" }; + firstAttempt["status"] = 7; + firstAttempt["retryAttempts"] = new JsonArray(new JsonObject + { + ["attempt"] = 1, + ["status"] = 3, + ["message"] = "nested", + ["name"] = "a test-only field section 11 forbids", + }); + + JsonObject finalAttempt = Test("t", "passed"); + finalAttempt["extra"] = new JsonObject { ["uid"] = "u1" }; + + string report = BuildReport(testEntries: [firstAttempt, finalAttempt]); + + JsonNode test = ((JsonArray)JsonNode.Parse( + CtrfReportMerger.Merge([report], CtrfMergeMode.CollapseRetryAttempts))!["results"]!["tests"]!)[0]!; + + var retryAttempts = (JsonArray)test["retryAttempts"]!; + Assert.HasCount(2, retryAttempts); + + // The nested attempt keeps its diagnostics but loses the wrong-typed status and the test-only field. + Assert.AreEqual("other", (string?)retryAttempts[0]!["status"], "A non-string status is normalized."); + Assert.AreEqual("nested", (string?)retryAttempts[0]!["message"]); + Assert.IsNull(retryAttempts[0]!["name"], "Section 11 forbids unknown fields outside 'extra'."); + + // The promoted row is normalized the same way. + Assert.AreEqual("other", (string?)retryAttempts[1]!["status"]); + + // Attempt numbers stay a contiguous 1..N-1 sequence after the projection. + Assert.AreEqual(1, (long)retryAttempts[0]!["attempt"]!); + Assert.AreEqual(2, (long)retryAttempts[1]!["attempt"]!); + } + private static JsonObject Attempt(string name, string status, string uid, long duration = 1, string? message = null) { var test = new JsonObject From 203ef4ae702a383ec47b7935ef2eb203f40cb57a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Thu, 30 Jul 2026 18:07:00 +0200 Subject: [PATCH 10/11] Document and pin the merger's validity contract Successive review rounds have asked the merger to validate or repair Test rows it passes through: reject or normalize rows with a missing or wrong-typed required field, and reshape the `retryAttempts[]` of a row that had nothing merged into it. Both are declined, for one reason worth stating in the code rather than re-arguing each round. The merger guarantees the shape of what it SYNTHESIZES -- the summary, the identity fields, and the retry attempt objects it builds and renumbers -- and relays verbatim what it passes through. It drops only elements that cannot be a Test at all, since those alone break the array's item type. Repairing a Test row would mean either dropping it, losing a real result, or rewriting it, fabricating an outcome the producer never reported. Both are worse than relaying a defect that belongs to the input document. The corollary is that merging a single document with unique identities leaves its tests[] untouched: this combines reports, it is not a CTRF validator. That also explains the asymmetry the second comment noticed. A row with cross-report priors gets a rebuilt history because the merger must renumber it into a contiguous 1..N-1, so that array is its own; a row without priors keeps the producer's array untouched. No behavior change. Adds a test pinning the pass-through so it cannot be eroded silently. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ad2ea8a-3349-4979-b27c-dd52bb3b7190 --- .../CtrfReportMerger.cs | 19 ++++++++++++ .../CtrfReportMergerTests.cs | 31 +++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs index 174de2782e..7f77fea3b9 100644 --- a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs +++ b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs @@ -22,6 +22,20 @@ namespace Microsoft.Testing.Extensions.CtrfReport; /// tool keeps a concrete identity only when every input reported the exact same tool object; otherwise (inputs disagree or any input omits it) a neutral merger identity is used, so one framework is not attributed to another's tests. /// environment keeps the first report's shared fields, but module-specific values under extra (testApplication, exitCode) are dropped rather than presented as describing all merged modules. /// +/// +/// Validity contract. The merger guarantees the shape of what it SYNTHESIZES — the summary, the identity +/// fields, and the retry attempt objects it builds and renumbers — and PRESERVES verbatim what it merely +/// passes through. The only thing it drops is an element that cannot be a Test at all (a non-object), because +/// that alone would break the array's item type for every consumer of the merged file. +/// +/// +/// It deliberately does NOT validate or repair the Test rows themselves, even though an input may carry a +/// row with a missing or wrong-typed required field. Dropping such a row would lose a real result and +/// rewriting it would fabricate an outcome the producer never reported, which are both worse than relaying a +/// defect that belongs to the input document. A corollary is that merging a single document whose identities +/// are already unique leaves its tests[] untouched: the merger combines reports, it is not a CTRF +/// validator or linter. +/// /// internal static class CtrfReportMerger { @@ -491,6 +505,11 @@ private static void AppendIdentityComponent(StringBuilder identity, string? comp private static JsonNode BuildCollapsedTest(JsonObject final, List priors) { var collapsed = (JsonObject)final.DeepClone(); + + // Nothing was merged into this row, so it is passed through rather than synthesized: its own + // `retryAttempts[]` (from in-process retries the producer already recorded) stays exactly as written. + // Below, once there ARE priors, the history becomes the merger's own array — it has to be rebuilt and + // renumbered into a contiguous 1..N-1 — which is why those entries are reshaped and these are not. if (priors.Count == 0) { return collapsed; diff --git a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs index e0e0407c92..29438f4564 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs @@ -1016,6 +1016,37 @@ public void Merge_CollapseRetryAttempts_KeepsRetryHistorySchemaValid() Assert.AreEqual(2, (long)retryAttempts[1]!["attempt"]!); } + [TestMethod] + public void Merge_CollapseRetryAttempts_LeavesAnUnmergedRowExactlyAsWritten() + { + // Validity contract: the merger shapes what it synthesizes and relays what it passes through. A test + // that occurs in only one input (for example one that recovered through in-process retries and was + // therefore never re-run by the orchestrator) has nothing merged into it, so its row -- including the + // retryAttempts[] its producer already recorded -- must come out byte-identical. Repairing it here + // would make merging a single document mutate it. + JsonObject row = Test("t", "passed"); + row["extra"] = new JsonObject { ["uid"] = "u1" }; + row["retryAttempts"] = new JsonArray(new JsonObject + { + ["attempt"] = 7, + ["status"] = "failed", + ["message"] = "recorded by the producer", + }); + + string report = BuildReport(testEntries: [row]); + string original = ((JsonArray)JsonNode.Parse(report)!["results"]!["tests"]!)[0]!.ToJsonString(); + + var tests = (JsonArray)JsonNode.Parse( + CtrfReportMerger.Merge([report], CtrfMergeMode.CollapseRetryAttempts))!["results"]!["tests"]!; + + Assert.HasCount(1, tests); + Assert.AreEqual(original, tests[0]!.ToJsonString(), "An unmerged row must be relayed verbatim."); + + // Specifically: the producer's own attempt numbering is not rewritten, and retries/flaky are not invented. + Assert.AreEqual(7, (long)((JsonArray)tests[0]!["retryAttempts"]!)[0]!["attempt"]!); + Assert.IsNull(tests[0]!["retries"]); + } + private static JsonObject Attempt(string name, string status, string uid, long duration = 1, string? message = null) { var test = new JsonObject From 6a0c80756ff251b2a533dc6b8bf2de79ab21d35f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Thu, 30 Jul 2026 18:18:25 +0200 Subject: [PATCH 11/11] Drop the redundant state machine from the MergeToFileAsync forwarder The three-argument overload only forwards to the four-argument one, so async/await bought nothing but a state machine allocation. Exception timing is unchanged: the inner overload is itself async, so its guard clauses already fault the returned task rather than throwing synchronously, which is what the ThrowsExactlyAsync tests observe either way. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ad2ea8a-3349-4979-b27c-dd52bb3b7190 --- .../CtrfReportMerger.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs index 7f77fea3b9..65f2b32d2f 100644 --- a/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs +++ b/src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportMerger.cs @@ -306,11 +306,11 @@ internal static string Merge(IReadOnlyList inputReports, CtrfMergeMode m return merged.ToJsonString(new JsonSerializerOptions { WriteIndented = true }); } - internal static async Task MergeToFileAsync( + internal static Task MergeToFileAsync( IReadOnlyList inputPaths, string outputPath, CancellationToken cancellationToken) - => await MergeToFileAsync(inputPaths, outputPath, CtrfMergeMode.Concatenate, cancellationToken).ConfigureAwait(false); + => MergeToFileAsync(inputPaths, outputPath, CtrfMergeMode.Concatenate, cancellationToken); internal static async Task MergeToFileAsync( IReadOnlyList inputPaths,