-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[release/9.0-staging][wbt] Prevent InvalidOperationException on TestOutputHelper logging.
#116916
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
[release/9.0-staging][wbt] Prevent InvalidOperationException on TestOutputHelper logging.
#116916
Conversation
… write to xUnit TestOutputHelper after test context has expired.
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 addresses race conditions that cause InvalidOperationException when logging via TestOutputHelper by adding exception handling, disposal guards, and consolidating logging logic.
- Wraps all
TestOutputHelper.WriteLinecalls in try-catch blocks with a warning marker on failure - Introduces an
isDisposedflag to prevent handlers from writing after disposal - Attempts to consolidate duplicate handler logic in
ToolCommand
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/mono/wasm/Wasm.Build.Tests/Common/ToolCommand.cs | Wrapped both error/output data handlers’ WriteLine calls in try-catch blocks |
| src/mono/wasm/Wasm.Build.Tests/BuildTestBase.cs | Added isDisposed flag and guards around logging; updated exception and log methods |
Comments suppressed due to low confidence (3)
src/mono/wasm/Wasm.Build.Tests/BuildTestBase.cs:601
- Add unit tests that simulate
TestOutputHelpercontext expiration to verify both the exception-catching path and the buffered output fallback are exercised.
void LogData(string label, string? message)
src/mono/wasm/Wasm.Build.Tests/BuildTestBase.cs:490
- [nitpick] The flag
isDisposedis used to guard logging after handlers detach. Consider renaming it to something more descriptive (e.g.,handlersDisposedorloggingDisabled) to clarify its purpose.
bool isDisposed = false;
src/mono/wasm/Wasm.Build.Tests/BuildTestBase.cs:620
- Currently
outputBuilder.AppendLineruns unconditionally, which may produce lines withnullwhenmessageis null. Guard this call withif (message != null)to avoid logging an empty or 'null' message.
outputBuilder.AppendLine($"{label} {message}");
InvalidOperationException on TestOutputHelper logging.InvalidOperationException on TestOutputHelper logging.
|
/azp run runtime |
|
Azure Pipelines failed to run 1 pipeline(s). |
|
/azp list |
This comment was marked as resolved.
This comment was marked as resolved.
|
Tagging subscribers to this area: @akoeplinger, @matouskozak, @simonrozsival |
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.
approved. please get a code review. we will treat this as tell mode
Tell mode, it updates only the tests.
Contributes to #105315.
Fix race condition causing "There is no currently active test" exceptions:
It also fixes the CI after #116747 merge. The PR disabling MT tests did not remove runtime job dependency on the disabled jobs. That caused runtime jobs not to get triggered as the dependency was not satisfied. This PR reverts disabling WBT MT jobs and runtime-official, keeping library MT commented out.