diff --git a/src/Platform/Microsoft.Testing.Extensions.AzureDevOpsReport/AzureDevOpsReporter.cs b/src/Platform/Microsoft.Testing.Extensions.AzureDevOpsReport/AzureDevOpsReporter.cs index aebb6b47e8..11038c0298 100644 --- a/src/Platform/Microsoft.Testing.Extensions.AzureDevOpsReport/AzureDevOpsReporter.cs +++ b/src/Platform/Microsoft.Testing.Extensions.AzureDevOpsReport/AzureDevOpsReporter.cs @@ -21,6 +21,21 @@ internal sealed class AzureDevOpsReporter : { private const string DeterministicBuildRoot = "/_/"; + // Fully-qualified type prefixes for MSTest assertion implementations. A stack frame whose + // 'code' (i.e., the "Namespace.Type.Method(args)" portion) starts with any of these is treated + // as framework internals and skipped when looking for the user's call site to annotate. + // Matching on the type name (rather than the source file name) is robust to partial-class + // splits (e.g. Assert.AreEqual.cs, Assert.IComparable.cs) and extension-based assertion + // implementations such as Assert.That in Assert.That.cs, and it avoids false positives on user + // files innocently named *Assert.cs. See https://github.com/microsoft/testfx/issues/6925. + private static readonly string[] AssertionImplementationCodePrefixes = + [ + "Microsoft.VisualStudio.TestTools.UnitTesting.Assert.", + "Microsoft.VisualStudio.TestTools.UnitTesting.AssertExtensions.", + "Microsoft.VisualStudio.TestTools.UnitTesting.CollectionAssert.", + "Microsoft.VisualStudio.TestTools.UnitTesting.StringAssert.", + ]; + private readonly IOutputDevice _outputDisplay; private readonly ILogger _logger; private static readonly char[] NewlineCharacters = ['\r', '\n']; @@ -210,13 +225,13 @@ private async Task WriteExceptionAsync(string testDisplayName, string? explanati } string file = location.Value.File; + string code = location.Value.Code; - // TODO: We need better rule for stackframes to opt out from being interesting. - if (file.EndsWith("Assert.cs", StringComparison.Ordinal)) + if (IsAssertionImplementationFrame(code)) { if (logger.IsEnabled(LogLevel.Trace)) { - logger.LogTrace("StackFrame location ends with 'Assert.cs' this is a special pattern that we skip, continuing to next."); + logger.LogTrace($"StackFrame code '{code}' is an MSTest assertion implementation, continuing to next."); } continue; @@ -311,6 +326,19 @@ private static string GetTargetFrameworkMoniker() => TargetFrameworkParser.GetShortTargetFramework(Assembly.GetEntryAssembly()?.GetCustomAttribute()?.FrameworkDisplayName) ?? TargetFrameworkParser.GetShortTargetFramework(RuntimeInformation.FrameworkDescription); + private static bool IsAssertionImplementationFrame(string code) + { + foreach (string prefix in AssertionImplementationCodePrefixes) + { + if (code.StartsWith(prefix, StringComparison.Ordinal)) + { + return true; + } + } + + return false; + } + private static (string Code, string File, int LineNumber)? GetStackFrameLocation(string stackTraceLine) { Match match = StackTraceHelper.GetFrameRegex().Match(stackTraceLine); diff --git a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/AzureDevOpsTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/AzureDevOpsTests.cs index c3f8eefd7e..9155032d18 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/AzureDevOpsTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/AzureDevOpsTests.cs @@ -4,6 +4,9 @@ using Microsoft.Testing.Extensions.AzureDevOpsReport; using Microsoft.Testing.Platform.Helpers; using Microsoft.Testing.Platform.Logging; +using Microsoft.Testing.TestInfrastructure; + +using Moq; namespace Microsoft.Testing.Extensions.UnitTests; @@ -26,7 +29,7 @@ public void ReportsTheFirstExistingFileInStackTraceWithTheRightLineNumberAndEsca // Trim ##. If we keep it, then when the test fails, the assertion failure will get printed to screen and picked up incorrectly by AzDO, because it scans all output for the ##vso... pattern var logger = new TextLogger(); string? text = AzureDevOpsReporter.GetErrorText("MyTestDisplayName", null, error, "severity", new SystemFileSystem(), logger, "net9.0")?.TrimStart('#'); - Assert.AreEqual("vso[task.logissue type=severity;sourcepath=test/UnitTests/Microsoft.Testing.Extensions.UnitTests/AzureDevOpsTests.cs;linenumber=19;columnnumber=1][MyTestDisplayName] [net9.0] this is an error%0Awith%0Dnewline", text, $"\nLogs:\n{string.Join("\n", logger.Logs)}"); + Assert.AreEqual("vso[task.logissue type=severity;sourcepath=test/UnitTests/Microsoft.Testing.Extensions.UnitTests/AzureDevOpsTests.cs;linenumber=22;columnnumber=1][MyTestDisplayName] [net9.0] this is an error%0Awith%0Dnewline", text, $"\nLogs:\n{string.Join("\n", logger.Logs)}"); } [TestMethod] @@ -45,7 +48,168 @@ public void ReportsTheFirstExistingFileInStackTraceWithTheRightLineNumberAndEsca // Trim ##. If we keep it, then when the test fails, the assertion failure will get printed to screen and picked up incorrectly by AzDO, because it scans all output for the ##vso... pattern var logger = new TextLogger(); string? text = AzureDevOpsReporter.GetErrorText("MyTestDisplayName", "Some custom reason\nwith\rnewline", error, "severity", new SystemFileSystem(), logger, "net9.0")?.TrimStart('#'); - Assert.AreEqual("vso[task.logissue type=severity;sourcepath=test/UnitTests/Microsoft.Testing.Extensions.UnitTests/AzureDevOpsTests.cs;linenumber=38;columnnumber=1][MyTestDisplayName] [net9.0] Some custom reason%0Awith%0Dnewline", text, $"\nLogs:\n{string.Join("\n", logger.Logs)}"); + Assert.AreEqual("vso[task.logissue type=severity;sourcepath=test/UnitTests/Microsoft.Testing.Extensions.UnitTests/AzureDevOpsTests.cs;linenumber=41;columnnumber=1][MyTestDisplayName] [net9.0] Some custom reason%0Awith%0Dnewline", text, $"\nLogs:\n{string.Join("\n", logger.Logs)}"); + } + + [TestMethod] + public void SkipsMSTestAssertImplementationFrameInPartialClassFile() + { + // Regression test for https://github.com/microsoft/testfx/issues/6925. + // + // The Assert class is split into partial-class files such as Assert.IComparable.cs and + // Assert.AreEqual.cs. Their file names do not end with "Assert.cs", so the previous + // filename-based skip heuristic missed them and the reporter incorrectly annotated the + // framework implementation. The fix matches on the fully-qualified type prefix in the + // 'code' capture instead, so every Assert/CollectionAssert/StringAssert partial is + // correctly recognized as framework internals. + (string userFile, int userLine) = GetCurrentLocation(); + + // The Assert.IComparable.cs file actually exists in the repo, so the file-existence + // check would happily accept the framework frame if we did not skip it. + string stackTrace = string.Join( + Environment.NewLine, + " at Microsoft.VisualStudio.TestTools.UnitTesting.Assert.IsLessThan[T](T upperBound, T value, String message) in /_/src/TestFramework/TestFramework/Assertions/Assert.IComparable.cs:line 138", + $" at Microsoft.Testing.Extensions.UnitTests.AzureDevOpsTests.SkipsMSTestAssertImplementationFrameInPartialClassFile() in {userFile}:line {userLine}"); + + var error = new SyntheticStackTraceException("boom", stackTrace); + + var logger = new TextLogger(); + string? text = AzureDevOpsReporter.GetErrorText("MyTestDisplayName", null, error, "severity", new SystemFileSystem(), logger, "net9.0")?.TrimStart('#'); + + Assert.AreEqual( + $"vso[task.logissue type=severity;sourcepath=test/UnitTests/Microsoft.Testing.Extensions.UnitTests/AzureDevOpsTests.cs;linenumber={userLine};columnnumber=1][MyTestDisplayName] [net9.0] boom", + text, + $"\nLogs:\n{string.Join("\n", logger.Logs)}"); + } + + [TestMethod] + public void SkipsMSTestCollectionAssertImplementationFrameInPartialClassFile() + { + (string userFile, int userLine) = GetCurrentLocation(); + + string stackTrace = string.Join( + Environment.NewLine, + " at Microsoft.VisualStudio.TestTools.UnitTesting.CollectionAssert.AreEqual(ICollection expected, ICollection actual, String message) in /_/src/TestFramework/TestFramework/Assertions/CollectionAssert.Equality.cs:line 42", + $" at Microsoft.Testing.Extensions.UnitTests.AzureDevOpsTests.SkipsMSTestCollectionAssertImplementationFrameInPartialClassFile() in {userFile}:line {userLine}"); + + var error = new SyntheticStackTraceException("boom", stackTrace); + + var logger = new TextLogger(); + string? text = AzureDevOpsReporter.GetErrorText("MyTestDisplayName", null, error, "severity", new SystemFileSystem(), logger, "net9.0")?.TrimStart('#'); + + Assert.AreEqual( + $"vso[task.logissue type=severity;sourcepath=test/UnitTests/Microsoft.Testing.Extensions.UnitTests/AzureDevOpsTests.cs;linenumber={userLine};columnnumber=1][MyTestDisplayName] [net9.0] boom", + text, + $"\nLogs:\n{string.Join("\n", logger.Logs)}"); + } + + [TestMethod] + public void SkipsMSTestStringAssertImplementationFrame() + { + (string userFile, int userLine) = GetCurrentLocation(); + + string stackTrace = string.Join( + Environment.NewLine, + " at Microsoft.VisualStudio.TestTools.UnitTesting.StringAssert.Contains(String value, String substring, String message) in /_/src/TestFramework/TestFramework/Assertions/StringAssert.cs:line 17", + $" at Microsoft.Testing.Extensions.UnitTests.AzureDevOpsTests.SkipsMSTestStringAssertImplementationFrame() in {userFile}:line {userLine}"); + + var error = new SyntheticStackTraceException("boom", stackTrace); + + var logger = new TextLogger(); + string? text = AzureDevOpsReporter.GetErrorText("MyTestDisplayName", null, error, "severity", new SystemFileSystem(), logger, "net9.0")?.TrimStart('#'); + + Assert.AreEqual( + $"vso[task.logissue type=severity;sourcepath=test/UnitTests/Microsoft.Testing.Extensions.UnitTests/AzureDevOpsTests.cs;linenumber={userLine};columnnumber=1][MyTestDisplayName] [net9.0] boom", + text, + $"\nLogs:\n{string.Join("\n", logger.Logs)}"); + } + + [TestMethod] + public void SkipsMSTestAssertThatImplementationFrame() + { + (string userFile, int userLine) = GetCurrentLocation(); + + string stackTrace = string.Join( + Environment.NewLine, + " at Microsoft.VisualStudio.TestTools.UnitTesting.AssertExtensions.That(Expression`1 condition, String message, String conditionExpression) in /_/src/TestFramework/TestFramework/Assertions/Assert.That.cs:line 27", + $" at Microsoft.Testing.Extensions.UnitTests.AzureDevOpsTests.SkipsMSTestAssertThatImplementationFrame() in {userFile}:line {userLine}"); + + var error = new SyntheticStackTraceException("boom", stackTrace); + + var logger = new TextLogger(); + string? text = AzureDevOpsReporter.GetErrorText("MyTestDisplayName", null, error, "severity", new SystemFileSystem(), logger, "net9.0")?.TrimStart('#'); + + Assert.AreEqual( + $"vso[task.logissue type=severity;sourcepath=test/UnitTests/Microsoft.Testing.Extensions.UnitTests/AzureDevOpsTests.cs;linenumber={userLine};columnnumber=1][MyTestDisplayName] [net9.0] boom", + text, + $"\nLogs:\n{string.Join("\n", logger.Logs)}"); + } + + [TestMethod] + public void DoesNotSkipUserFrameWhoseFileNameEndsWithAssertCs() + { + // The previous heuristic skipped any frame whose file path ended with "Assert.cs", + // which incorrectly hid user code in files named e.g. MyAssert.cs from PR annotations. + // The fix is based on the fully-qualified type name in the frame, so user types are + // never confused with the framework's Assert/CollectionAssert/StringAssert types. + string repoRoot = RootFinder.Find(); + string userFile = Path.Combine(repoRoot, "src", "MyCompany", "MyAssert.cs"); + + string stackTrace = + $" at MyCompany.Verification.MyAssert.Verify(Object value) in {userFile}:line 17"; + + var error = new SyntheticStackTraceException("boom", stackTrace); + + var fileSystem = new Mock(); + fileSystem.Setup(fs => fs.ExistFile(It.IsAny())).Returns(true); + + var logger = new TextLogger(); + string? text = AzureDevOpsReporter.GetErrorText("MyTestDisplayName", null, error, "severity", fileSystem.Object, logger, "net9.0")?.TrimStart('#'); + + Assert.AreEqual( + "vso[task.logissue type=severity;sourcepath=src/MyCompany/MyAssert.cs;linenumber=17;columnnumber=1][MyTestDisplayName] [net9.0] boom", + text, + $"\nLogs:\n{string.Join("\n", logger.Logs)}"); + } + + [TestMethod] + public void DoesNotSkipUserFrameWhoseTypeNameStartsWithAssert() + { + // A user type literally named "Assert" (e.g. MyCompany.Tests.Assert) must not be + // mistaken for Microsoft.VisualStudio.TestTools.UnitTesting.Assert. The prefix check + // is anchored on the full MSTest namespace, so user types in other namespaces are safe. + string repoRoot = RootFinder.Find(); + string userFile = Path.Combine(repoRoot, "src", "MyCompany", "MyAssertions.cs"); + + string stackTrace = + $" at MyCompany.Tests.Assert.Equal[T](T expected, T actual) in {userFile}:line 25"; + + var error = new SyntheticStackTraceException("boom", stackTrace); + + var fileSystem = new Mock(); + fileSystem.Setup(fs => fs.ExistFile(It.IsAny())).Returns(true); + + var logger = new TextLogger(); + string? text = AzureDevOpsReporter.GetErrorText("MyTestDisplayName", null, error, "severity", fileSystem.Object, logger, "net9.0")?.TrimStart('#'); + + Assert.AreEqual( + "vso[task.logissue type=severity;sourcepath=src/MyCompany/MyAssertions.cs;linenumber=25;columnnumber=1][MyTestDisplayName] [net9.0] boom", + text, + $"\nLogs:\n{string.Join("\n", logger.Logs)}"); + } + + private static (string FilePath, int LineNumber) GetCurrentLocation( + [CallerFilePath] string? filePath = null, + [CallerLineNumber] int lineNumber = 0) + => (filePath!, lineNumber); + + private sealed class SyntheticStackTraceException : Exception + { + public SyntheticStackTraceException(string message, string stackTrace) + : base(message) + => StackTrace = stackTrace; + + public override string? StackTrace { get; } } private class TextLogger : ILogger