-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Handle assemblies gracefully #52308
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
Handle assemblies gracefully #52308
Conversation
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.
Pull request overview
This PR improves error handling in the TerminalTestReporter class by replacing direct dictionary access with TryGetValue calls to prevent KeyNotFoundException exceptions when an executionId is not found in the _assemblies dictionary.
Key changes:
- Replaced four instances of direct dictionary access (
_assemblies[executionId]) with safeTryGetValuecalls - Added graceful handling for missing executionIds with early returns
- Special handling in
AssemblyRunCompletedto still report non-zero exit codes even when the executionId is not found
src/Cli/dotnet/Commands/Test/MTP/Terminal/TerminalTestReporter.cs
Outdated
Show resolved
Hide resolved
src/Cli/dotnet/Commands/Test/MTP/Terminal/TerminalTestReporter.cs
Outdated
Show resolved
Hide resolved
src/Cli/dotnet/Commands/Test/MTP/Terminal/TerminalTestReporter.cs
Outdated
Show resolved
Hide resolved
|
Which issue are we fixing here? It feels like this change will ignore more unexpected errors. Is it possible to rather solve the pre-condition that makes this fail? |
Youssef1313
left a comment
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.
This is best to be handled from the caller of TerminalTestReporter so that HandshakeFailure is called instead of AssemblyRunCompleted for the case when we receive a handshake from testhost controller but not from testhost.
Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>
Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>
Youssef1313
left a comment
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.
LGTM. It's best if we can have a test for this.
|
/backport to release/10.0.3xx |
|
Started backporting to |
|
/backport to release/10.0.2xx |
|
Let me port to 10.0.2xx instead. |
|
Started backporting to |
This pull request improves the robustness of the
TerminalTestReporterclass by adding safe dictionary access for test progress state lookups. Instead of directly accessing the_assembliesdictionary, the code now usesTryGetValueto prevent potential exceptions when an execution ID is missing. This ensures smoother error handling and avoids crashes in edge cases.Error handling and robustness improvements:
TryGetValuein methods such asTestCompleted,TestDiscovered, andTestInProgressto gracefully handle missing execution IDs and prevent exceptions. [1] [2] [3]AssemblyRunCompletedto handle cases where the execution ID does not exist by reporting non-zero exit codes to the terminal and skipping state updates, improving stability for non-"TestHost" scenarios.