Add regression tests for 8 previously untested bug fixes#15627
Add regression tests for 8 previously untested bug fixes#15627nohwnd wants to merge 1 commit intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds regression coverage for a set of previously untested bug fixes across multiple components (TestHostProvider, CommunicationUtilities, TrxLogger, HtmlLogger, ObjectModel), ensuring the fixed behaviors remain protected from regressions.
Changes:
- Introduces new “RegressionBugFixTests” test classes in 5 unit-test projects.
- Adds targeted assertions for specific historical bugs (stderr forwarding level, ARM64 hosting selection, socket IO exception propagation, TRX logger behaviors, HtmlLogger directory creation, PortablePdbReader null validation).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/RegressionBugFixTests.cs | Adds regression tests for stderr forwarding level and ARM64 Windows hosting selection. |
| test/Microsoft.TestPlatform.ObjectModel.UnitTests/RegressionBugFixTests.cs | Adds regression coverage for PortablePdbReader constructor null-argument validation. |
| test/Microsoft.TestPlatform.Extensions.TrxLogger.UnitTests/RegressionBugFixTests.cs | Adds regression tests for TRX error info ordering, WarnOnFileOverwrite parsing, and TestOutcome enum semantics. |
| test/Microsoft.TestPlatform.Extensions.HtmlLogger.UnitTests/RegressionBugFixTests.cs | Adds regression tests ensuring HtmlLogger.Initialize creates the results directory. |
| test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/RegressionBugFixTests.cs | Adds regression tests around socket/IO exception propagation behavior in MessageLoopAsync. |
| // GH-2479: On ARM64 Windows, testhost.exe must not be used. | ||
| // The fix added an IsWinOnArm() guard that checks PROCESSOR_ARCHITECTURE. | ||
| // | ||
| // When PROCESSOR_ARCHITECTURE at Machine level does NOT contain "arm" | ||
| // (i.e., running on x64), the guard returns false and testhost.exe CAN be used. | ||
| // So this test verifies the inverse: on non-ARM, with testhost.exe present, | ||
| // it IS used (confirming the guard activates only on ARM). | ||
| // | ||
| // On ARM64 hardware, IsWinOnArm() returns true and testhost.exe is skipped. |
There was a problem hiding this comment.
The GH-2479 test setup doesn’t appear to force the precondition where testhost.exe is discoverable, so it may pass even if the regression returns (e.g., if the implementation can’t find testhost.exe and falls back to dotnet + testhost.dll regardless). Also, the comments describe verifying the inverse on non-ARM, but the mocks set Architecture to ARM64. To make this a true regression test: (1) explicitly mock the PROCESSOR_ARCHITECTURE machine-level lookup via IEnvironmentVariableHelper to an ARM value to deterministically exercise the guard, and (2) configure IFileHelper.Exists(...) to return true for the computed testhost.exe path (or via a predicate matching ...EndsWith(\"testhost.exe\")) so the code path would choose it if the guard were removed. Update the comments to match the actual scenario being tested.
| mockEnvironment.SetupGet(e => e.Architecture).Returns(PlatformArchitecture.ARM64); | ||
| mockEnvironment.Setup(ev => ev.OperatingSystem).Returns(PlatformOperatingSystem.Windows); |
There was a problem hiding this comment.
The GH-2479 test setup doesn’t appear to force the precondition where testhost.exe is discoverable, so it may pass even if the regression returns (e.g., if the implementation can’t find testhost.exe and falls back to dotnet + testhost.dll regardless). Also, the comments describe verifying the inverse on non-ARM, but the mocks set Architecture to ARM64. To make this a true regression test: (1) explicitly mock the PROCESSOR_ARCHITECTURE machine-level lookup via IEnvironmentVariableHelper to an ARM value to deterministically exercise the guard, and (2) configure IFileHelper.Exists(...) to return true for the computed testhost.exe path (or via a predicate matching ...EndsWith(\"testhost.exe\")) so the code path would choose it if the guard were removed. Update the comments to match the actual scenario being tested.
| mockFileHelper.Setup(fh => fh.Exists(testhostDllPath)).Returns(true); | ||
| mockFileHelper.Setup(fh => fh.Exists(dotnetPath)).Returns(true); |
There was a problem hiding this comment.
The GH-2479 test setup doesn’t appear to force the precondition where testhost.exe is discoverable, so it may pass even if the regression returns (e.g., if the implementation can’t find testhost.exe and falls back to dotnet + testhost.dll regardless). Also, the comments describe verifying the inverse on non-ARM, but the mocks set Architecture to ARM64. To make this a true regression test: (1) explicitly mock the PROCESSOR_ARCHITECTURE machine-level lookup via IEnvironmentVariableHelper to an ARM value to deterministically exercise the guard, and (2) configure IFileHelper.Exists(...) to return true for the computed testhost.exe path (or via a predicate matching ...EndsWith(\"testhost.exe\")) so the code path would choose it if the guard were removed. Update the comments to match the actual scenario being tested.
| // In both cases, the filename must NOT end with testhost.exe. | ||
| Assert.IsNotNull(startInfo); | ||
| Assert.IsFalse( | ||
| startInfo.FileName!.EndsWith("testhost.exe", StringComparison.OrdinalIgnoreCase), | ||
| "GH-2479: ARM64 on Windows must NOT use testhost.exe."); |
There was a problem hiding this comment.
The GH-2479 test setup doesn’t appear to force the precondition where testhost.exe is discoverable, so it may pass even if the regression returns (e.g., if the implementation can’t find testhost.exe and falls back to dotnet + testhost.dll regardless). Also, the comments describe verifying the inverse on non-ARM, but the mocks set Architecture to ARM64. To make this a true regression test: (1) explicitly mock the PROCESSOR_ARCHITECTURE machine-level lookup via IEnvironmentVariableHelper to an ARM value to deterministically exercise the guard, and (2) configure IFileHelper.Exists(...) to return true for the computed testhost.exe path (or via a predicate matching ...EndsWith(\"testhost.exe\")) so the code path would choose it if the guard were removed. Update the comments to match the actual scenario being tested.
| var dotnetPath = @"c:\tmp\dotnet.exe"; | ||
|
|
||
| mockEnvironment.SetupGet(e => e.Architecture).Returns(PlatformArchitecture.ARM64); | ||
| mockEnvironment.Setup(ev => ev.OperatingSystem).Returns(PlatformOperatingSystem.Windows); | ||
| mockRunsettingHelper.SetupGet(r => r.IsDefaultTargetArchitecture).Returns(false); | ||
| mockProcessHelper.Setup(ph => ph.GetCurrentProcessFileName()).Returns(dotnetPath); | ||
| mockProcessHelper.Setup(ph => ph.GetTestEngineDirectory()).Returns(dotnetPath); | ||
| mockProcessHelper.Setup(ph => ph.GetCurrentProcessArchitecture()).Returns(PlatformArchitecture.ARM64); | ||
| mockFileHelper.Setup(fh => fh.Exists(testhostDllPath)).Returns(true); | ||
| mockFileHelper.Setup(fh => fh.Exists(dotnetPath)).Returns(true); |
There was a problem hiding this comment.
GetTestEngineDirectory() is being set to a full executable path (c:\\tmp\\dotnet.exe) rather than a directory path. If the production code combines this value with filenames, it can generate invalid paths (and reduce the test’s fidelity). Return an actual directory (e.g., c:\\tmp or a temp directory) from GetTestEngineDirectory().
| var dotnetPath = @"c:\tmp\dotnet.exe"; | |
| mockEnvironment.SetupGet(e => e.Architecture).Returns(PlatformArchitecture.ARM64); | |
| mockEnvironment.Setup(ev => ev.OperatingSystem).Returns(PlatformOperatingSystem.Windows); | |
| mockRunsettingHelper.SetupGet(r => r.IsDefaultTargetArchitecture).Returns(false); | |
| mockProcessHelper.Setup(ph => ph.GetCurrentProcessFileName()).Returns(dotnetPath); | |
| mockProcessHelper.Setup(ph => ph.GetTestEngineDirectory()).Returns(dotnetPath); | |
| mockProcessHelper.Setup(ph => ph.GetCurrentProcessArchitecture()).Returns(PlatformArchitecture.ARM64); | |
| mockFileHelper.Setup(fh => fh.Exists(testhostDllPath)).Returns(true); | |
| mockFileHelper.Setup(fh => fh.Exists(dotnetPath)).Returns(true); | |
| var dotnetExePath = Path.Combine(temp, "dotnet.exe"); | |
| var dotnetDirectory = temp; | |
| mockEnvironment.SetupGet(e => e.Architecture).Returns(PlatformArchitecture.ARM64); | |
| mockEnvironment.Setup(ev => ev.OperatingSystem).Returns(PlatformOperatingSystem.Windows); | |
| mockRunsettingHelper.SetupGet(r => r.IsDefaultTargetArchitecture).Returns(false); | |
| mockProcessHelper.Setup(ph => ph.GetCurrentProcessFileName()).Returns(dotnetExePath); | |
| mockProcessHelper.Setup(ph => ph.GetTestEngineDirectory()).Returns(dotnetDirectory); | |
| mockProcessHelper.Setup(ph => ph.GetCurrentProcessArchitecture()).Returns(PlatformArchitecture.ARM64); | |
| mockFileHelper.Setup(fh => fh.Exists(testhostDllPath)).Returns(true); | |
| mockFileHelper.Setup(fh => fh.Exists(dotnetExePath)).Returns(true); |
| mockEnvironment.Setup(ev => ev.OperatingSystem).Returns(PlatformOperatingSystem.Windows); | ||
| mockRunsettingHelper.SetupGet(r => r.IsDefaultTargetArchitecture).Returns(false); | ||
| mockProcessHelper.Setup(ph => ph.GetCurrentProcessFileName()).Returns(dotnetPath); | ||
| mockProcessHelper.Setup(ph => ph.GetTestEngineDirectory()).Returns(dotnetPath); |
There was a problem hiding this comment.
GetTestEngineDirectory() is being set to a full executable path (c:\\tmp\\dotnet.exe) rather than a directory path. If the production code combines this value with filenames, it can generate invalid paths (and reduce the test’s fidelity). Return an actual directory (e.g., c:\\tmp or a temp directory) from GetTestEngineDirectory().
| mockProcessHelper.Setup(ph => ph.GetTestEngineDirectory()).Returns(dotnetPath); | |
| mockProcessHelper.Setup(ph => ph.GetTestEngineDirectory()).Returns(temp); |
| { | ||
| if (Directory.Exists(nonExistentDir)) | ||
| { | ||
| Directory.Delete(nonExistentDir); |
There was a problem hiding this comment.
Test cleanup deletes the directory non-recursively. If Initialize (or future changes) creates any files/subdirectories under nonExistentDir, Directory.Delete(nonExistentDir) will throw and fail the test. Use the recursive overload (and/or ensure the directory is empty) to make the test robust.
| Directory.Delete(nonExistentDir); | |
| Directory.Delete(nonExistentDir, recursive: true); |
|
|
||
| Exception? capturedError = null; | ||
|
|
||
| serverSide.GetStream().Write(new byte[] { 1 }, 0, 1); |
There was a problem hiding this comment.
The second socket-based test uses a synchronous Write with no flush and no cancellation token. Depending on timing/buffering, this can make the test more prone to intermittent hangs until CancelAfter triggers. Prefer mirroring the first test’s approach (WriteAsync + FlushAsync using TestContext.CancellationToken) to more reliably trigger the Poll/NotifyDataAvailable path and reduce flakiness.
| serverSide.GetStream().Write(new byte[] { 1 }, 0, 1); | |
| await serverSide.GetStream().WriteAsync(new byte[] { 1 }, 0, 1, TestContext.CancellationToken); | |
| await serverSide.GetStream().FlushAsync(TestContext.CancellationToken); |
| using var cts = CancellationTokenSource.CreateLinkedTokenSource(TestContext.CancellationToken); | ||
| cts.CancelAfter(TimeSpan.FromSeconds(5)); | ||
|
|
||
| await client.MessageLoopAsync(channel.Object, error => capturedError = error, cts.Token); |
There was a problem hiding this comment.
The second socket-based test uses a synchronous Write with no flush and no cancellation token. Depending on timing/buffering, this can make the test more prone to intermittent hangs until CancelAfter triggers. Prefer mirroring the first test’s approach (WriteAsync + FlushAsync using TestContext.CancellationToken) to more reliably trigger the Poll/NotifyDataAvailable path and reduce flakiness.
5e77a3d to
322bd8d
Compare
Tests verify the specific behavior that was fixed — each test would fail if the corresponding fix were reverted. - microsoftGH-4461: Socket IOException not propagated when child exits (2 tests) - microsoftGH-2319: ErrorStackTrace settable before ErrorMessage (3 tests) - microsoftGH-5132: WarnOnFileOverwrite parameter parsed and applied (2 tests) - microsoftGH-4243: TestOutcome.Error is value 0, no Min/Max aliases (3 tests) - microsoftGH-5184: Stderr forwarded as Informational, not Error (2 tests) - microsoftGH-2479: ARM64 on Windows uses DLL hosting, not testhost.exe (1 test) - microsoftGH-2483: HtmlLogger.Initialize creates results directory (2 tests) - microsoftGH-3454: PortablePdbReader(MetadataReaderProvider) validates null (1 test) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
322bd8d to
4c37368
Compare
Summary
Add 25 regression tests (22 unit + 3 integration) across 13 files for 11 previously untested bug fixes.
Each test verifies the specific behavior that was fixed and would fail if the fix were reverted.
Unit Tests
Integration Tests