fix(tests): Time out a hung test instead of the whole shard - #24472
MartinZikmund wants to merge 1 commit into
Conversation
Only tests carrying an explicit [Timeout] were bounded; every other body was awaited forever, so one hang discarded the shard's entire results file - including the ~1750 tests that had already passed and the name of the one that hung. Default to 5 minutes, well above the ~77s the shape parity tests need on WebAssembly, and cancel the timer once the test completes so the default does not leave one pending per test. fixes #24471 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jox2uQ5z5SLRqfrhGMg2ZX
github-actions
Bot
added
the
area/automation
|
Claude finished @MartinZikmund's task in 3m 3s —— View job Review complete ✅Scope:
Verdict: fix-first2 inline findings posted, both medium/low severity — no blockers to the core fix. Cross-cutting notes (not tied to a single line):
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Ceiling for a test body that declares no [Timeout]. Deliberately far above any real test | ||
| // — the shape parity tests measure ~77s on WebAssembly — because its only job is to turn a | ||
| // hung test into one named failure rather than a whole shard whose results are never written. | ||
| private static readonly TimeSpan DefaultTestMethodTimeout = TimeSpan.FromMinutes(5); |
There was a problem hiding this comment.
DefaultTestMethodTimeout is a fixed 5 minutes for every build configuration, but the sibling DefaultUnitTestTimeout right above it (lines 59-63) is deliberately DEBUG-vs-Release split (300s / 60s) — presumably because a breakpoint held during local debugging can legitimately pause a test body for several minutes, and the old code let untagged tests await forever so that never mattered.
With this change, any untagged async test now gets a hard 5-minute ceiling in DEBUG too. A developer stepping through a runtime test with a debugger for >5 minutes will now hit a spurious TimeoutException where previously it just waited — a regression in the local dev-loop that the CI-focused p50/p99 measurements in the PR description don't cover (those numbers come from unattended CI runs).
Consider gating this like DefaultUnitTestTimeout (e.g. skip/extend the default when Debugger.IsAttached, or reuse the DEBUG/Release split) so the CI robustness fix doesn't trade away the "sit at a breakpoint as long as you want" debugging experience.
| if (timeout.HasValue) | ||
| { | ||
| var timeoutTask = Task.Delay(timeout.Value); | ||
| var timeout = GetTestTimeout(test) ?? DefaultTestMethodTimeout; |
There was a problem hiding this comment.
No test exercises the new default-timeout path itself (a [Timeout]-less test that hangs → reported as a named TimeoutException instead of stalling the run). The PR body explains this is hard to produce deterministically in CI, which is fair, but per the repo's red/fix/green guidance for bug fixes it's worth at least a narrow unit test around GetTestTimeout(test) ?? DefaultTestMethodTimeout (e.g. a fake UnitTestMethodInfo with/without [Timeout]) to lock in the "explicit attribute overrides the default" contract this line encodes, since that's the one thing this fix must never regress.
There was a problem hiding this comment.
🟢 Approval recommended
The logic change is straightforward and appears correct for preventing indefinite awaits, with only a minor optional simplification/perf tweak noted in review comments.
Pull request overview
This PR updates the SamplesApp unit test harness so a hung async test (with no [Timeout]) fails as a named timeout instead of hanging the entire shard and preventing result publication.
Changes:
- Introduces a 5-minute default timeout ceiling for test bodies that don’t declare
[Timeout]. - Replaces the previous “timeout only if attribute exists” branching with a single timeout path for
Task-returning tests. - Cancels the timeout timer once the test completes to avoid accumulating pending timers across large runs.
File summaries
| File | Description |
|---|---|
| src/SamplesApp/SamplesApp.UnitTests.Shared/Controls/UnitTest/UnitTestsControl.cs | Adds a default test-body timeout and applies it uniformly to async tests to avoid shard-wide hangs and missing results. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| using var timeoutCancellation = new CancellationTokenSource(); | ||
| var timeoutTask = Task.Delay(timeout, timeoutCancellation.Token); | ||
|
|
||
| if (resultingTask == timeoutTask) | ||
| { | ||
| throw new TimeoutException( | ||
| $"Test execution timed out after {timeout.Value}"); | ||
| } | ||
| var resultingTask = await Task.WhenAny(task, timeoutTask); | ||
|
|
||
| // Rethrow exception if failed OR task cancelled if task **internally** raised | ||
| // a TaskCancelledException (we don't provide any cancellation token). | ||
| await resultingTask; | ||
| } | ||
| else | ||
| if (resultingTask == timeoutTask) | ||
| { | ||
| await task; | ||
| throw new TimeoutException( | ||
| $"Test execution timed out after {timeout}"); | ||
| } | ||
|
|
||
| // Release the timer, so the default timeout does not leave one pending | ||
| // per test for the remainder of the run. | ||
| timeoutCancellation.Cancel(); | ||
|
|
||
| // Rethrow exception if failed OR task cancelled if task **internally** raised | ||
| // a TaskCancelledException (we don't provide any cancellation token). | ||
| await resultingTask; |
|
|
morning4coffe-dev
left a comment
morning4coffe-dev
left a comment
There was a problem hiding this comment.
Reviewed e13b6388e2ee, including the surrounding retry, exception-reporting and cleanup paths.
I exercised the exact selected await blocks from base and head, the unchanged GetTestTimeout logic, and the actual five-minute default in an isolated .NET probe. Seven checks passed: explicit attribute precedence; the unannotated base control remaining hung; completed tasks cancelling their timers; preservation of the original exception; preservation of task cancellation; an explicit 75 ms timeout; and the real default timeout against a genuinely incomplete task. The default fired after 300.061 seconds with the expected TimeoutException, while the base control was still incomplete. The default was not shortened or replaced with a fake timer.
The timeout exception still flows through the existing named-test failure/retry path. Cancelling the losing timer before awaiting the winning task preserves fault and cancellation behavior rather than turning either into a success.
The existing debugger-duration note is a useful non-blocking follow-up: unannotated Debug tests now also have the deliberate five-minute ceiling. This change does not interrupt synchronous blocking or cancel arbitrary underlying test work; the outer harness watchdog remains necessary.
Validation limits: the selected source logic used minimal test-metadata stand-ins. This was not a full SamplesApp, native WinUI, or WebAssembly runtime run, nor a newly executed CI hang. Other reviewers' threads were left unchanged.
GitHub Issue: closes #24471
PR Type:
🐞 Bugfix
What changed? 🚀
UnitTestsControlonly bounded a test body when the method carried an explicit[Timeout]; the else branch was a bareawait task. So one hung async test with no attribute was awaited forever, the results XML was never written, and the job died on the harness or job timeout instead — discarding the whole shard, including up to ~1750 results that had already passed and the name of the test that hung.That makes it an amplifier: three unrelated WebAssembly failure mechanisms (the sample runner never registering, a mid-run hang, and heap exhaustion) all collapse into the same undiagnosable "no results file" symptom. It is also why failures inside timed-out jobs never reach any flakiness analysis — they contribute zero rows.
[Timeout]now overrides a default instead of being the only source of one. The single timeout path replaces the if/else, and the timer is cancelled once the test completes so a long default doesn't leave one pending per test for the rest of the run.Choosing the default
The existing
DefaultUnitTestTimeout(60 s in Release, applied only to initialize/cleanup) was the obvious thing to reuse — and it would have broken CI. Measured per-test durations on healthy agents:When_Polyline)When_Line)The shape-parity tests legitimately reach ~77 s on WebAssembly, so a 60 s default would have turned currently-green builds red. 5 minutes is ~4× the observed healthy maximum, which is ample for its actual job — converting an infinite hang into one named failure — while staying well inside the shard budget.
Durations were recovered from the per-test
Running test X()lines in the published Android device logs and the WebAssembly job logs of build 232676.Known limitation
On single-threaded WebAssembly, a
Task.Delaycontinuation cannot run if a test blocks the dispatcher synchronously. This fix covers async hangs, which is the common case; a synchronous block still needs a JS-side watchdog. Not addressed here.Validation
UnoIslandsSamplesApp.Skia(which includesSamplesApp.UnitTests.Shared) builds with 0 errors onnet10.0.GetTestTimeoutstill returns the[Timeout]value when present, so existing per-test timeouts are unchanged; only the previously-unbounded path gains a ceiling.PR Checklist ✅
Screenshots Compare Test Runresults.🤖 Generated with Claude Code
https://claude.ai/code/session_01Jox2uQ5z5SLRqfrhGMg2ZX