-
Notifications
You must be signed in to change notification settings - Fork 291
Fix Assert.Inconclusive in constructor to behave consistently with TestInitialize #7304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Evangelink <[email protected]>
Co-authored-by: Evangelink <[email protected]>
| result.Outcome = UTF.UnitTestOutcome.Timeout; | ||
| result.TestFailureException = new TestFailedException(UTFUnitTestOutcome.Timeout, string.Format(CultureInfo.CurrentCulture, Resource.Execution_Test_Timeout, TestMethodName, TimeoutInfo.Timeout)); | ||
| } | ||
| else if (realException is AssertInconclusiveException) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need all this here?
The caller of CreateTestClassInstance is already wrapped in try/catch. Should we let the exception bubble up from CreateTestClassInstance to be handled by the caller? Or is there really some special handling needed specific to CreateTestClassInstance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some manual lookup and it's also unclear why we need this catch at all.
@copilot can you try removing the catch entirely and letting the caller handle the exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception handling in CreateTestClassInstance is necessary and follows the existing pattern in the codebase. If we let the exception bubble up:
- The error message would be incorrect - it would use
UTA_TestMethodThrowsinstead ofUTA_InstanceCreationError - We'd need to re-throw after catching, complicating control flow
- The caller checks
_classInstance != nullto determine if execution should continue
The current pattern allows constructor failures to gracefully set the result and return null, so the caller can skip TestInitialize and remaining phases. This is consistent with how timeout exceptions in constructors are already handled (line 1036-1039).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 7f17965. Removed the entire catch block from CreateTestClassInstance. The method now simply invokes the constructor and returns the instance. Any exceptions (including AssertInconclusiveException) bubble up to the caller's exception handler which already has the logic to handle them correctly at lines 479-480.
|
@copilot test fails with: |
Co-authored-by: Evangelink <[email protected]>
... Fixed in 637c67a. The issue was that
The enum ordering now matches the actual test lifecycle execution order. |
…ndle it Co-authored-by: Evangelink <[email protected]>
Co-authored-by: Evangelink <[email protected]>
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Evangelink <[email protected]>
Co-authored-by: Evangelink <[email protected]>
Plan to fix Assert.Inconclusive behavior in constructor
TestMethodInfo.cshandles TestInitialize correctly but not constructorChanges
Simplified the fix by removing the catch block from
CreateTestClassInstanceentirely. The caller's exception handler already checks forAssertInconclusiveExceptionand sets the outcome correctly (lines 479-480). This approach:Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.