AzureDevOpsReporter: skip Assert frames by type name instead of filename (fixes #6925)#8284
AzureDevOpsReporter: skip Assert frames by type name instead of filename (fixes #6925)#8284Evangelink wants to merge 3 commits into
Conversation
Fix #6925. The previous heuristic skipped stack frames whose file path ended with 'Assert.cs', but the MSTest Assert class is split into partial-class files (Assert.AreEqual.cs, Assert.IComparable.cs, ...) whose names do not match. As a result, the reporter annotated the framework implementation instead of the user's call site. Switch the skip rule to a fully-qualified type prefix check on the 'code' capture from the stack frame regex. This covers every partial-class file (current and future) and removes the false-positive that wrongly skipped user files named *Assert.cs. Closes #8278. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes Azure DevOps reporting so MSTest assertion implementation frames are skipped by fully-qualified frame code/type prefix rather than by source filename.
Changes:
- Adds MSTest assertion type-prefix filtering in
AzureDevOpsReporter. - Adds regression tests for partial assertion files and user files/types that should not be skipped.
- Updates existing expected line numbers after new imports.
Show a summary per file
| File | Description |
|---|---|
src/Platform/Microsoft.Testing.Extensions.AzureDevOpsReport/AzureDevOpsReporter.cs |
Replaces filename-based Assert frame skipping with code-prefix-based assertion implementation detection. |
test/UnitTests/Microsoft.Testing.Extensions.UnitTests/AzureDevOpsTests.cs |
Adds synthetic stack trace tests covering MSTest assertion frame skipping and false-positive avoidance. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
Evangelink
left a comment
There was a problem hiding this comment.
Review Summary
This is a clean, well-scoped fix for #6925. All 21 review dimensions assessed; no blocking issues found.
| Dimension | Status |
|---|---|
| 1. Algorithmic Correctness | ✅ Root cause fixed (type prefix vs. filename). GetStackFrameLocation guarantees code is non-null/non-empty before IsAssertionImplementationFrame is called. |
| 2. Threading & Concurrency | ✅ N/A — no shared mutable state introduced. AssertionImplementationCodePrefixes is static readonly (immutable array). |
| 3. Security & IPC | ✅ N/A |
| 4. Public API / Binary Compat | ✅ All new code is private static — no public surface added. |
| 5. Performance | ✅ foreach over 3-element static array; no allocations on the hot path. |
| 6. Cross-TFM | ✅ string.StartsWith(StringComparison.Ordinal) available on all targets. |
| 7. IDisposable | ✅ N/A |
| 8. Defensive Coding | ✅ code is validated non-empty by GetStackFrameLocation before use. |
| 9. Localization | ✅ Log messages are trace-only diagnostics, not user-facing; no .resx required. |
| 10. Test Isolation | ✅ New tests use synthetic stack traces; no shared state. |
| 11. Assertion Quality | ✅ BannedSymbols.txt bans AwesomeAssertions; tests correctly use MSTest Assert.AreEqual. |
| 12. Flakiness | ✅ GetCurrentLocation uses [CallerFilePath]/[CallerLineNumber] — deterministic at compile time. |
| 13–21. Remaining dimensions | ✅ N/A for this change. |
Observations (non-blocking):
- The
GetCurrentLocation()helper captures line 65 asuserLine, then immediately uses it in the synthetic stack trace at line 72 and in the expected assertion output at line 80. The test is self-consistent, but note that inserting any line betweenGetCurrentLocation()and thestackTraceconstruction would silently shift the captured line without breaking the test (because both references move together). This is an accepted pattern for this kind of test, just worth knowing. - The PR description mentions a follow-up
[StackTraceHidden]initiative (#8277) that would make this heuristic unnecessary on .NET 6+. This PR remains correct and necessary for .NET Framework.
LGTM — the fix is correct, tested, and well-explained.
Generated by Expert Code Review (on open) for issue #8284 · ● 4.6M
|
@copilot address review comments and fix the broken test below: test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TelemetryTests.cs#L132 test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TelemetryTests.cs(132,1): error : [VSTest_RunTests_Succeeds ("net462")] [net11.0] Assert.AreEqual failed. Expected:<0>. Actual:<1>. 'expected' expression: '0', 'actual' expression: 'testResult.ExitCode'. dotnet test failed: |
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Addressed in e1d0f4f: the regression stack frame now uses |
| // splits (e.g. Assert.AreEqual.cs, Assert.IComparable.cs) and 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.", |
Fixes #6925. Closes #8278.
Problem
AzureDevOpsReporter.GetErrorText walks a failing test's stack frames and skips the frame if the file path ends with
Assert.cs, so the reporter annotates the user's call site rather than the framework's assertion implementation:https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Extensions.AzureDevOpsReport/AzureDevOpsReporter.cs#L215-L223
But
Assert/CollectionAssert/StringAssertare split into partial-class files (Assert.AreEqual.cs,Assert.IComparable.cs,Assert.IsInstanceOfType.cs,CollectionAssert.Equality.cs, etc.) whose file names do not end withAssert.cs. The filter misses every one of them, so for cases like theAssert.IsInstanceOfTypefailure in #6925 the reporter incorrectly annotates the framework file instead of the user's test.The filter also has the opposite problem: a user file innocently named
MyAssert.cswould be wrongly skipped.Fix
Switch the skip decision from the file name to the fully-qualified type name captured in the stack frame's
codefield (already parsed byStackTraceHelper.GetFrameRegexand previously unused). A frame is skipped iff itscodestarts with one of:Microsoft.VisualStudio.TestTools.UnitTesting.Assert.Microsoft.VisualStudio.TestTools.UnitTesting.CollectionAssert.Microsoft.VisualStudio.TestTools.UnitTesting.StringAssert.This naturally covers every partial-class file (current and future), is robust against file renames/splits, and removes the
MyAssert.csfalse positive.A complementary follow-up — adding
[StackTraceHidden]on the same APIs once RFC 012 lands — is tracked separately in #8277 and would obviate the reporter heuristic entirely on .NET 6+. This PR is still needed for .NET Framework.Tests
Added 5 tests to
AzureDevOpsTests:SkipsMSTestAssertImplementationFrameInPartialClassFile— regression test for Assert.IComparable not compatible with AzDoReport extension #6925: synthetic stack with a frame inAssert.IComparable.csfollowed by a user frame; asserts the user frame is reported.SkipsMSTestCollectionAssertImplementationFrameInPartialClassFile— same forCollectionAssert.Equality.cs.SkipsMSTestStringAssertImplementationFrame— same forStringAssert.cs.DoesNotSkipUserFrameWhoseFileNameEndsWithAssertCs— verifies the previous false positive on user files named*Assert.csis gone (mockedIFileSystem).DoesNotSkipUserFrameWhoseTypeNameStartsWithAssert— verifies a user type in a non-MSTest namespace namedAssertis not mistaken for the framework'sAssert.The two pre-existing tests' expected line numbers were updated because the
using Moq;directive shifted the throw statements down.All 7 tests pass on
net9.0,net8.0, andnet472. FullMicrosoft.Testing.Extensions.UnitTestssuite (124 tests) passes.