Derive line numbers from source in DiaSession and adapter tests#15528
Derive line numbers from source in DiaSession and adapter tests#15528nohwnd wants to merge 2 commits intomicrosoft:mainfrom
Conversation
Replace hardcoded line numbers with source-derived values so tests are resilient to code moving around in test assets. The debug/release branching is kept where needed since PDB sequence points genuinely differ between modes (debug points to opening brace, release points to first executable statement). Tests fixed: - DiaSessionTests.GetNavigationDataShouldReturnCorrectFileNameAndLineNumber - DiaSessionTests.GetNavigationDataShouldReturnCorrectDataForAsyncMethod - DiaSessionTests.GetNavigationDataShouldReturnCorrectDataForOverLoadedMethod - DiaSessionTests.DiaSessionPerfTest - DifferentTestFrameworkSimpleTests.RunTestsWithNunitAdapter - DifferentTestFrameworkSimpleTests.RunTestsWithXunitAdapter Fixes microsoft#15458 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates integration tests to derive expected PDB line numbers from test asset source files instead of relying on hardcoded values, improving resilience when test assets change line positions while preserving Debug/Release differences.
Changes:
- Replace hardcoded expected line numbers in adapter/translation-layer tests with source-derived values.
- Replace DiaSessionTests’ hardcoded line assertions and helper validators with source-range discovery helpers.
- Add simple source scanners to locate method body start/end lines in test assets.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/Microsoft.TestPlatform.Library.IntegrationTests/TranslationLayerTests/DifferentTestFrameworkSimpleTests.cs | Computes expected test case line numbers by scanning test asset source for the method body start line. |
| test/Microsoft.TestPlatform.Library.IntegrationTests/DiaSessionTests.cs | Computes expected DIA navigation min/max lines by scanning test asset source for method body start/end instead of using fixed constants. |
test/Microsoft.TestPlatform.Library.IntegrationTests/DiaSessionTests.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.TestPlatform.Library.IntegrationTests/DiaSessionTests.cs
Outdated
Show resolved
Hide resolved
...Platform.Library.IntegrationTests/TranslationLayerTests/DifferentTestFrameworkSimpleTests.cs
Outdated
Show resolved
Hide resolved
...Platform.Library.IntegrationTests/TranslationLayerTests/DifferentTestFrameworkSimpleTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates integration tests to derive expected line numbers directly from the source test assets (and keep debug/release differences where PDB sequence points differ), reducing fragility when assets move.
Changes:
- Add helpers to locate method body boundaries / first executable line by scanning source files.
- Replace hardcoded expected line numbers in adapter integration tests with source-derived values.
- Update DiaSession integration tests to assert source-derived min/max navigation line numbers and fix perf timing measurement.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/Microsoft.TestPlatform.Library.IntegrationTests/TranslationLayerTests/DifferentTestFrameworkSimpleTests.cs | Derives expected adapter-reported line numbers from test asset source instead of hardcoding. |
| test/Microsoft.TestPlatform.Library.IntegrationTests/DiaSessionTests.cs | Derives expected DIA navigation line numbers from source and fixes perf test timing assertion. |
| // Derive expected line number from source so the test is resilient to code moving around. | ||
| var expectedLine = FindMethodBodyStartLine( | ||
| Path.Combine(_testEnvironment.TestAssetsPath, "XUTestProject", "Class1.cs"), | ||
| "PassTestMethod1"); | ||
| // In Release, PDB sequence points start at first executable statement, not the opening brace. | ||
| if (IntegrationTestEnvironment.BuildConfiguration.StartsWith("release", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| Assert.AreEqual(15, testCase.First().TestCase.LineNumber); | ||
| } | ||
| else | ||
| { | ||
| Assert.AreEqual(14, testCase.First().TestCase.LineNumber); | ||
| expectedLine++; | ||
| } | ||
|
|
||
| Assert.AreEqual(expectedLine, testCase.First().TestCase.LineNumber); |
There was a problem hiding this comment.
expectedLine++ assumes the first executable statement is always exactly one line after the opening brace. This will break if the method starts with a blank line, comment, #if, or any non-executable line. Use the same approach as the NUnit test (scan for the first executable line) so Release builds remain resilient to source edits.
| // Derive expected line numbers from the source file so the test is resilient to code moving around. | ||
| var sourceFile = Path.Combine(_testEnvironment.TestAssetsPath, "SimpleClassLibrary", "Class1.cs"); | ||
| var (bodyStart, bodyEnd) = FindMethodBodyRange(sourceFile, "PassingTest"); | ||
| // In Debug, PDB min line points to opening brace. In Release, it points to first executable statement. | ||
| var expectedMin = IntegrationTestEnvironment.BuildConfiguration.StartsWith("release", StringComparison.OrdinalIgnoreCase) | ||
| ? bodyStart + 1 | ||
| : bodyStart; | ||
| Assert.AreEqual(expectedMin, diaNavigationData.MinLineNumber, "Incorrect min line number"); | ||
| Assert.AreEqual(bodyEnd, diaNavigationData.MaxLineNumber, "Incorrect max line number"); |
There was a problem hiding this comment.
For Release builds, expectedMin = bodyStart + 1 has the same fragility as a hardcoded line number: it assumes the first executable statement is exactly one line after {. If the method body begins with comments/blank lines/directives, this will be wrong. Consider deriving the first executable line by scanning forward from bodyStart (similar to FindFirstExecutableLine in the adapter tests) and use that as expectedMin for Release.
| private static (int BodyStart, int BodyEnd) FindMethodBodyRange(string sourceFile, string methodName) | ||
| { | ||
| // Release builds optimize code, hence min line numbers are different. | ||
| if (IntegrationTestEnvironment.BuildConfiguration.StartsWith("release", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| Assert.AreEqual(min, max, "Incorrect min line number"); | ||
| } | ||
| else | ||
| var lines = File.ReadAllLines(sourceFile); | ||
| for (int i = 0; i < lines.Length; i++) | ||
| { | ||
| if (max == 22) | ||
| if (lines[i].Contains($" {methodName}(")) | ||
| { | ||
| Assert.AreEqual(min + 1, max, "Incorrect min line number"); | ||
| } | ||
| else if (max == 26) | ||
| { | ||
| Assert.AreEqual(min + 1, max, "Incorrect min line number"); | ||
| } | ||
| else | ||
| { | ||
| Assert.Fail($"Incorrect min/max line number. Expected Max to be 22 or 26. And Min to be 21 or 25. But Min was {min}, and Max was {max}."); | ||
| int braceDepth = 0; | ||
| int bodyStart = -1; | ||
| for (int j = i; j < lines.Length; j++) | ||
| { | ||
| var line = lines[j]; | ||
| foreach (var ch in line) | ||
| { | ||
| if (ch == '{') | ||
| { | ||
| if (bodyStart == -1) | ||
| { | ||
| bodyStart = j + 1; // 1-based | ||
| } | ||
|
|
||
| braceDepth++; | ||
| } | ||
| else if (ch == '}') | ||
| { | ||
| braceDepth--; | ||
| if (braceDepth == 0) | ||
| { | ||
| return (bodyStart, j + 1); // 1-based | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static void ValidateMinLineNumber(int expected, int actual) | ||
| { | ||
| // Release builds optimize code, hence min line numbers are different. | ||
| if (IntegrationTestEnvironment.BuildConfiguration.StartsWith("release", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| Assert.AreEqual(expected + 1, actual, "Incorrect min line number"); | ||
| } | ||
| else | ||
| { | ||
| Assert.AreEqual(expected, actual, "Incorrect min line number"); | ||
| } | ||
| throw new InvalidOperationException($"Could not find method '{methodName}' in '{sourceFile}'."); | ||
| } |
There was a problem hiding this comment.
The method locator/parsing here is quite brittle: Contains($\" {methodName}(\") can miss valid declarations (e.g., start-of-line, different spacing, generic methods) and can match non-declarations (comments/strings). The brace counter also counts braces inside comments/strings, which can produce incorrect ranges. Since these helpers are specifically to make tests resilient, it’s worth making them a bit more robust by (a) using a regex to identify a method declaration more reliably (as done in DifferentTestFrameworkSimpleTests), and (b) skipping commented lines (at least // ...) when searching, similar to the other helper.
| var sourceFile = Path.Combine(_testEnvironment.TestAssetsPath, "SimpleClassLibrary", "Class1.cs"); | ||
| var (bodyStart, bodyEnd) = FindMethodBodyRange(sourceFile, "OverLoadedMethod"); | ||
| if (IntegrationTestEnvironment.BuildConfiguration.StartsWith("release", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| // Release builds for empty methods have min == max (closing brace is the only sequence point). | ||
| Assert.AreEqual(diaNavigationData.MinLineNumber, diaNavigationData.MaxLineNumber, "Incorrect min line number"); | ||
| } | ||
| else | ||
| { | ||
| Assert.AreEqual(bodyStart, diaNavigationData.MinLineNumber, "Incorrect min line number"); | ||
| Assert.AreEqual(bodyEnd, diaNavigationData.MaxLineNumber, "Incorrect max line number"); | ||
| } |
There was a problem hiding this comment.
In the Release branch, the assertion only checks min == max, but does not validate which line that is. This can let real regressions slip through (e.g., DIA returning a completely unrelated line number that happens to be equal for min/max). Since you already compute (bodyStart, bodyEnd), assert that MinLineNumber/MaxLineNumber equals the expected sequence point line (often the closing brace for empty methods), or at least that it falls within [bodyStart, bodyEnd].
|
Closing - upstream already has a better solution using SourceAssert helper. |
Replace hardcoded line numbers with source-derived values so tests are resilient to code moving around in test assets. The debug/release branching is kept where needed since PDB sequence points genuinely differ between build modes.
Tests fixed:
Fixes #15458