Fix TaskCanceledException in TestReporter when ConnectedTask is canceled#1544
Fix TaskCanceledException in TestReporter when ConnectedTask is canceled#1544rolfbjarne wants to merge 2 commits intodotnet:mainfrom
Conversation
When _listener.ConnectedTask is canceled (e.g. due to process timeout), Task.IsCompleted returns true but accessing .Result throws TaskCanceledException. This caused numerous test runs to report as HarnessException instead of reporting their actual result. Use IsCompletedSuccessfully instead of IsCompleted to guard .Result access - it only returns true for tasks that ran to completion (not canceled or faulted). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a cancellation edge-case in the iOS shared TestReporter so canceled listener connection tasks don’t throw TaskCanceledException when their Result is accessed, allowing test runs to report the actual outcome instead of failing as a harness error.
Changes:
- Switches guards from
Task.IsCompletedtoTask.IsCompletedSuccessfullybefore reading_listener.ConnectedTask.Result. - Applies the same fix both for the “test execution started” check and for the empty run-log launch-failure detection.
| if (!_listener.ConnectedTask.IsCompletedSuccessfully || !_listener.ConnectedTask.Result) | ||
| { | ||
| _launchFailure = true; | ||
| } |
There was a problem hiding this comment.
Add a unit test covering the scenario this change fixes: when ConnectedTask is canceled (e.g., due to a timeout) and the run log is empty, GetPidFromRunLog/CollectSimulatorResult should not throw and should treat it as a launch failure. This will prevent regressions where accessing ConnectedTask.Result reintroduces TaskCanceledException.
| if (!_listener.ConnectedTask.IsCompletedSuccessfully || !_listener.ConnectedTask.Result) | |
| { | |
| _launchFailure = true; | |
| } | |
| var connectedTask = _listener.ConnectedTask; | |
| // If the task never completed, or if accessing the result throws (e.g. due to cancellation), | |
| // treat this as a launch failure instead of surfacing TaskCanceledException. | |
| if (!connectedTask.IsCompleted) | |
| { | |
| _launchFailure = true; | |
| } | |
| else | |
| { | |
| bool connected = false; | |
| try | |
| { | |
| connected = connectedTask.Result; | |
| } | |
| catch (AggregateException ae) when (ae.InnerException is TaskCanceledException) | |
| { | |
| connected = false; | |
| } | |
| catch (TaskCanceledException) | |
| { | |
| connected = false; | |
| } | |
| if (!connected) | |
| { | |
| _launchFailure = true; | |
| } | |
| } |
vitek-karas
left a comment
There was a problem hiding this comment.
If it's easy we should add a test, other than that this looks good.
When _listener.ConnectedTask is canceled (e.g. due to process timeout),
Task.IsCompleted returns true but accessing .Result throws
TaskCanceledException. This caused numerous test runs to report as
HarnessException instead of reporting their actual result.
Use IsCompletedSuccessfully instead of IsCompleted to guard .Result
access - it only returns true for tasks that ran to completion (not
canceled or faulted).