From 6118608cfe1ee3a8e34cb8288b53f0de659c704e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Tue, 7 Jul 2026 06:32:00 +0200 Subject: [PATCH 1/2] Simplify GitHubActionsAnnotationReporter Extract testName local in ConsumeAsync to avoid the duplicate GetTestName call, and extract the shared DisplayAnnotationLineAsync helper from WriteAnnotationAsync and WriteSkippedAnnotationAsync, consolidating the leading-newline explanation in one place. Fixes #9658 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../GitHubActionsAnnotationReporter.cs | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsAnnotationReporter.cs b/src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsAnnotationReporter.cs index bad0de6b3e..0e2105de37 100644 --- a/src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsAnnotationReporter.cs +++ b/src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsAnnotationReporter.cs @@ -86,6 +86,8 @@ public async Task ConsumeAsync(IDataProducer dataProducer, IData value, Cancella _ => null, }; + string testName = GetTestName(nodeUpdateMessage.TestNode); + if (failure is null) { // Skipped tests carry no exception (and therefore no source location); surface them as a @@ -93,13 +95,13 @@ public async Task ConsumeAsync(IDataProducer dataProducer, IData value, Cancella // workflow Annotations tab alongside failures, rather than being silently absent. if (nodeState is SkippedTestNodeStateProperty skipped) { - await WriteSkippedAnnotationAsync(GetTestName(nodeUpdateMessage.TestNode), skipped.Explanation, cancellationToken).ConfigureAwait(false); + await WriteSkippedAnnotationAsync(testName, skipped.Explanation, cancellationToken).ConfigureAwait(false); } return; } - await WriteAnnotationAsync(GetTestName(nodeUpdateMessage.TestNode), failure.Value.Explanation, failure.Value.Exception, cancellationToken).ConfigureAwait(false); + await WriteAnnotationAsync(testName, failure.Value.Explanation, failure.Value.Exception, cancellationToken).ConfigureAwait(false); } catch (OperationCanceledException) { @@ -114,7 +116,7 @@ public async Task ConsumeAsync(IDataProducer dataProducer, IData value, Cancella } } - private async Task WriteAnnotationAsync(string testName, string? explanation, Exception? exception, CancellationToken cancellationToken) + private Task WriteAnnotationAsync(string testName, string? explanation, Exception? exception, CancellationToken cancellationToken) { if (_logger.IsEnabled(LogLevel.Trace)) { @@ -129,13 +131,7 @@ private async Task WriteAnnotationAsync(string testName, string? explanation, Ex _logger.LogTrace($"Showing failure annotation '{line}'."); } - // Prepend a newline so the '::error' workflow command always starts at column 0 on its own line. - // In CI the terminal output device runs in SimpleAnsi mode and emits a color reset ('\e[m') WITHOUT a - // trailing newline after the preceding colored "failed" test block. Emitting the annotation directly - // would yield "\e[m::error ..." and GitHub only recognizes a workflow command when the line begins - // with '::', so the dangling reset would silently drop the annotation. The leading newline pushes the - // reset onto its own (ignored) line and keeps the annotation parseable. - await _outputDisplay.DisplayAsync(this, new FormattedTextOutputDeviceData($"\n{line}"), cancellationToken).ConfigureAwait(false); + return DisplayAnnotationLineAsync(line, cancellationToken); } internal static /* for testing */ string GetErrorAnnotation(string testName, string? explanation, Exception? exception, string? repoRoot, IFileSystem fileSystem, ILogger logger, bool skipAssertionFrames) @@ -164,7 +160,7 @@ private async Task WriteAnnotationAsync(string testName, string? explanation, Ex GitHubActionsEscaper.EscapeData(message)); } - private async Task WriteSkippedAnnotationAsync(string testName, string? explanation, CancellationToken cancellationToken) + private Task WriteSkippedAnnotationAsync(string testName, string? explanation, CancellationToken cancellationToken) { if (_logger.IsEnabled(LogLevel.Trace)) { @@ -178,11 +174,18 @@ private async Task WriteSkippedAnnotationAsync(string testName, string? explanat _logger.LogTrace($"Showing skip annotation '{line}'."); } - // Prepend a newline for the same reason as the failure annotation: it guarantees the '::warning' - // workflow command starts at column 0 on its own line so GitHub recognizes it. - await _outputDisplay.DisplayAsync(this, new FormattedTextOutputDeviceData($"\n{line}"), cancellationToken).ConfigureAwait(false); + return DisplayAnnotationLineAsync(line, cancellationToken); } + // Prepend a newline so every workflow command annotation ('::error' or '::warning') starts at column 0 + // on its own line. In CI the terminal output device runs in SimpleAnsi mode and emits a color reset + // ('\e[m') WITHOUT a trailing newline after the preceding colored test block. Emitting the annotation + // directly would yield "\e[m::error ..." and GitHub only recognizes a workflow command when the line + // begins with '::', so the dangling reset would silently drop the annotation. The leading newline pushes + // the reset onto its own (ignored) line and keeps the annotation parseable. + private Task DisplayAnnotationLineAsync(string line, CancellationToken cancellationToken) + => _outputDisplay.DisplayAsync(this, new FormattedTextOutputDeviceData($"\n{line}"), cancellationToken); + internal static /* for testing */ string GetSkippedAnnotation(string testName, string? explanation) { string message = explanation ?? GitHubActionsResources.NoSkipReasonFallback; From a32c91f33c8891fbaeb36c1808ff04917c9f970c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Tue, 7 Jul 2026 09:50:27 +0200 Subject: [PATCH 2/2] Keep GetTestName lazy and make annotation source-location test deterministic - Address review feedback: revert the hoisted 'testName' local so GetTestName stays lazy at the two annotation call sites, avoiding an eager property-bag walk + string allocation on the common passing/in-progress path (keeps the DisplayAnnotationLineAsync extraction). - Fix flaky Windows Release CI: GetErrorAnnotation source-location tests relied on a real throw whose PDB-derived line shifts under Release JIT (net462 vs net472), producing line=110 vs expected 114. Build a deterministic synthetic stack frame via [CallerFilePath] instead, keeping both file and line stable while still exercising the resolver's repo-root/'/_/' relativization. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../GitHubActionsAnnotationReporter.cs | 9 +++--- .../GitHubActionsAnnotationReporterTests.cs | 31 +++++++++---------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsAnnotationReporter.cs b/src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsAnnotationReporter.cs index 0e2105de37..c53ca969d1 100644 --- a/src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsAnnotationReporter.cs +++ b/src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsAnnotationReporter.cs @@ -86,8 +86,6 @@ public async Task ConsumeAsync(IDataProducer dataProducer, IData value, Cancella _ => null, }; - string testName = GetTestName(nodeUpdateMessage.TestNode); - if (failure is null) { // Skipped tests carry no exception (and therefore no source location); surface them as a @@ -95,13 +93,16 @@ public async Task ConsumeAsync(IDataProducer dataProducer, IData value, Cancella // workflow Annotations tab alongside failures, rather than being silently absent. if (nodeState is SkippedTestNodeStateProperty skipped) { - await WriteSkippedAnnotationAsync(testName, skipped.Explanation, cancellationToken).ConfigureAwait(false); + // GetTestName is computed lazily at the call sites so the common passing/in-progress path + // (which returns below without annotating) does not walk the property bag or allocate the + // formatted name it would immediately discard. + await WriteSkippedAnnotationAsync(GetTestName(nodeUpdateMessage.TestNode), skipped.Explanation, cancellationToken).ConfigureAwait(false); } return; } - await WriteAnnotationAsync(testName, failure.Value.Explanation, failure.Value.Exception, cancellationToken).ConfigureAwait(false); + await WriteAnnotationAsync(GetTestName(nodeUpdateMessage.TestNode), failure.Value.Explanation, failure.Value.Exception, cancellationToken).ConfigureAwait(false); } catch (OperationCanceledException) { diff --git a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/GitHubActionsAnnotationReporterTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/GitHubActionsAnnotationReporterTests.cs index fccbd56e12..32caf6b7d6 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/GitHubActionsAnnotationReporterTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/GitHubActionsAnnotationReporterTests.cs @@ -103,24 +103,21 @@ public void GetSkippedAnnotation_FallsBackToDefaultReason_WhenNoExplanation() Assert.AreEqual("::warning title=Test skipped%3A MyNamespace.MyTest::The test was skipped without providing a reason.", text); } - // Throws (and catches) an exception, reporting the exact line of the throw statement so tests can assert the - // resolved line without hard-coding a physical number that shifts whenever code above changes. - private static Exception CaptureException(string message, out int throwLine) + // Produces an exception whose stack trace deterministically points at this test file at a fixed line, so + // tests can assert the resolved file and line. A real 'throw' was previously used, but the runtime-reported + // line of a thrown exception shifts under Release JIT optimization (observed differing between .NET + // Framework net462 and net472), which made the exact-line assertion flaky in CI (see #9658). A synthetic + // frame keeps both the resolved file and line stable while still exercising the resolver's real + // repo-root/'/_/' path relativization: [CallerFilePath] yields this file's path (mapped to '/_/test/...' + // in deterministic CI builds, or an absolute path locally), exactly as a genuine frame would. + private static Exception CaptureException(string message, out int throwLine, [CallerFilePath] string filePath = "") { - throwLine = 0; - try - { - throwLine = CurrentLine() + 1; - throw new Exception(message); - } - catch (Exception ex) - { - return ex; - } + throwLine = 12345; + return new StackTraceException( + $" at Microsoft.Testing.Extensions.UnitTests.GitHubActionsAnnotationReporterTests.CaptureException() in {filePath}:line {throwLine}", + message); } - private static int CurrentLine([CallerLineNumber] int line = 0) => line; - private static IFileSystem CreateFileSystemWhereEveryFileExists() { var fileSystem = new Mock(); @@ -134,7 +131,9 @@ private sealed class StackTraceException : Exception { private readonly string _stackTrace; - public StackTraceException(string stackTrace) => _stackTrace = stackTrace; + public StackTraceException(string stackTrace, string? message = null) + : base(message) + => _stackTrace = stackTrace; public override string? StackTrace => _stackTrace; }