Cleanup AssemblyInitialize failure handling#7547
Conversation
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestAssemblyInfo.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR refactors MSTest adapter assembly-initialize failure handling so initialization failures are returned as TestResult data instead of being propagated via thrown exceptions, aligning the runner and unit tests with the new behavior.
Changes:
- Change
TestAssemblyInfo.RunAssemblyInitializeAsyncto return aTestResultand store initialization failures asTestFailedException. - Update
UnitTestRunnerto consume the returnedTestResult(removing special-case catching ofTestFailedExceptionfor assembly init). - Update unit tests to assert on
TestResult.TestFailureExceptionrather than expecting exceptions to be thrown.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestAssemblyInfoTests.cs | Updates tests to validate returned TestResult and failure exception contents instead of exception throwing. |
| src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs | Uses the TestResult returned by assembly initialization and attaches captured logs/errors/trace. |
| src/Adapter/MSTestAdapter.PlatformServices/Execution/TestAssemblyInfo.cs | Changes assembly init API to return TestResult and normalizes stored init failures to TestFailedException. |
You can also share your feedback on Copilot code review. Take the survey.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestAssemblyInfo.cs
Show resolved
Hide resolved
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestAssemblyInfo.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
This PR refactors AssemblyInitialize failure handling in the MSTest adapter execution flow by returning a TestResult from RunAssemblyInitializeAsync instead of throwing, and updates the runner/tests accordingly.
Changes:
- Change
TestAssemblyInfo.RunAssemblyInitializeAsyncto return aTestResultand cache failures asTestFailedException. - Update
UnitTestRunnerto consume the returnedTestResultrather than relying on catchingTestFailedException. - Update
TestAssemblyInfoTeststo assert on returnedTestResult.TestFailureExceptioninstead of expecting exceptions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestAssemblyInfoTests.cs | Updates unit tests to validate returned TestResult/failure exception rather than thrown exceptions. |
| src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs | Adjusts assembly-initialize invocation to use the returned TestResult. |
| src/Adapter/MSTestAdapter.PlatformServices/Execution/TestAssemblyInfo.cs | Implements new RunAssemblyInitializeAsync contract returning TestResult and normalizes failures to TestFailedException. |
You can also share your feedback on Copilot code review. Take the survey.
Evangelink
left a comment
There was a problem hiding this comment.
Clean refactoring — removing the throw/catch dance in favor of returning TestResult directly simplifies the control flow nicely and avoids exception overhead on the normal failure path. The GetTestFailedExceptionFromAssemblyInitializeException extraction keeps the wrapping logic clean, and the property type narrowing (Exception? → TestFailedException?) is safe since TestAssemblyInfo is internal sealed. The hasExecuted guard addition in RunAssemblyInitializeShouldPassOnTheTestContextToAssemblyInitMethod is a nice touch.
Left a couple of minor nits on the tests.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestAssemblyInfoTests.cs
Show resolved
Hide resolved
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestAssemblyInfoTests.cs
Show resolved
Hide resolved
|
@Youssef1313 I've opened a new pull request, #7550, to work on those changes. Once the pull request is ready, I'll request review from you. |
…eAsync` tests (#7550) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
No description provided.