Add cooperative slow-test diagnostics for browser#10128
Conversation
Track active tests internally in the simplified browser and WASI output path, and report long-running tests with exponential backoff without relying on threads, processes, or named pipes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2eba6c9f-f63f-4726-a0ce-f90ed2404431
There was a problem hiding this comment.
Pull request overview
Adds cooperative slow-test diagnostics for browser/WASI environments.
Changes:
- Tracks active tests and reports slow tests with exponential backoff.
- Shares terminal progress configuration and localized messaging.
- Adds focused tests, API tracking, and browser documentation.
Show a summary per file
| File | Description |
|---|---|
SimplifiedConsoleOutputDeviceTests.cs |
Tests tracking and reporter lifecycle. |
ActiveTestTrackerTests.cs |
Tests thresholds, backoff, and UID handling. |
TerminalOutputDevice.Initialization.cs |
Uses shared progress configuration. |
TerminalOutputDevice.cs |
Removes relocated constants. |
SimplifiedConsoleOutputDeviceBase.cs |
Integrates tracking and cooperative reporting. |
ProgressReportingConfiguration.cs |
Centralizes environment-based thresholds. |
ActiveTestTracker.cs |
Implements active-test tracking and backoff. |
InternalAPI.Unshipped.txt |
Tracks new internal APIs. |
BrowserPlayground/README.md |
Documents slow-test diagnostics. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Medium
| if (nodeStateProp is InProgressTestNodeStateProperty) | ||
| { | ||
| _activeTestTracker.Start(testNodeStateChanged.TestNode.Uid, testNodeStateChanged.TestNode.DisplayName); | ||
| } |
| Set `MTP_PROGRESS_SLOW_TEST_SECONDS` to a non-negative integer to change the first | ||
| reporting threshold; `0` disables these diagnostics. Test starts are tracked silently, | ||
| so this does not duplicate normal per-test progress output. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) | ||
| { | ||
| } |
🧪 Test quality grade — PR #10128
This advisory comment was generated automatically. Grades are heuristic Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Summary
Well-structured PR that adds cooperative slow-test diagnostics to the simplified console output device, enabling browser-wasm scenarios where timers/threads are unavailable. The design is sound: async polling with exponential backoff, clean separation of tracker logic, and good test coverage.
Findings
| Severity | Finding |
|---|---|
Potential race in OnTestSessionFinishingAsync — fields _slowTestReporterCancellationTokenSource and _slowTestReporterTask are read/nulled without atomicity guarantees. Use Interlocked.Exchange. |
|
| 💡 Low | GetDueDiagnostics() allocates on every poll even when no tests are due (empty list → array). Consider early-return with cached empty array or avoid the list allocation. |
| 💡 Low | The reporter won't restart if a second test session starts after the first finishes (flag not reset). Probably fine for current usage but worth a comment. |
Verdict
The race condition in session teardown is the only item worth addressing before merge — the rest are suggestions. Overall this is clean, well-tested code with good documentation.
| @@ -224,6 +275,11 @@ public Task OnTestSessionStartingAsync(ITestSessionContext testSessionContext) | |||
| if (_firstCallTo_OnSessionStartingAsync) | |||
There was a problem hiding this comment.
The CancellationTokenSource.CreateLinkedTokenSource(cancellationToken) captures the session-starting cancellation token, but if OnTestSessionStartingAsync is called more than once (the guard only gates the first call), the second invocation is a no-op. However, if a second session does start after the first finishes (edge case), the fields were nulled in OnTestSessionFinishingAsync but the _firstCallTo_OnSessionStartingAsync flag is never reset, so the reporter won't restart. This is likely fine for single-session hosts, but worth a comment for maintainability.
| { | ||
| return []; | ||
| } | ||
|
|
There was a problem hiding this comment.
Minor: GetDueDiagnostics() allocates a List<SlowTestDiagnostic> and then converts it to an array on every poll (every 1 second by default), even when no tests are due. Consider using a pooled list or building the array directly when the count is known to be small, to reduce GC pressure in the hot path — especially relevant for browser-wasm where GC pauses are more visible.
| { | ||
| return long.MaxValue; | ||
| } | ||
|
|
There was a problem hiding this comment.
Good: the overflow guard (nextThresholdTicks > long.MaxValue / 2) prevents infinite loops for extremely long-running tests. Clean implementation.
| @@ -207,9 +234,33 @@ public async Task DisplayAfterSessionEndRunAsync(CancellationToken cancellationT | |||
|
|
|||
There was a problem hiding this comment.
Race condition: _slowTestReporterCancellationTokenSource and _slowTestReporterTask are read without synchronization. If OnTestSessionFinishingAsync is called concurrently (e.g. abort path races with normal completion), one caller could read the CTS while another nulls it. Consider using Interlocked.Exchange to atomically swap both fields to null, ensuring exactly one caller cancels and awaits.
Extract exponential threshold state for reuse by browser and WASI diagnostics, terminal heartbeat output, and CI slow-test reporters. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2eba6c9f-f63f-4726-a0ce-f90ed2404431
| TimeSpan elapsed = detail.Stopwatch.Elapsed; | ||
| SlowTestThresholdState thresholdState = _slowTestThresholdStates.TryGetValue(detail.Id, out SlowTestThresholdState? stored) | ||
| ? stored | ||
| : new(_slowTestThreshold); | ||
| if (!thresholdState.IsDue(elapsed)) |
| /// </remarks> | ||
| internal async Task ReportSlowTestsOnceAsync(CancellationToken cancellationToken) | ||
| { | ||
| SlowTestDiagnostic[] diagnostics = _activeTestTracker.GetDueDiagnostics(); |
| var diagnostics = new List<SlowTestDiagnostic>(); | ||
| foreach ((TestNodeUid uid, ActiveTest activeTest) in _activeTests.OrderBy(static pair => pair.Key.Value, StringComparer.Ordinal)) | ||
| { | ||
| TimeSpan elapsed = activeTest.Stopwatch.Elapsed; | ||
| if (!activeTest.SlowTestThreshold.IsDue(elapsed)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| diagnostics.Add(new(uid, activeTest.DisplayName, elapsed)); | ||
| } | ||
|
|
||
| return [.. diagnostics]; |
Browser and WASI test runs cannot use the HangDump extension because it depends on process APIs and named pipes. This adds a lightweight diagnostic path that identifies active tests which remain running without relying on unsupported browser threading or process facilities.
[slow] still running after ...lines using the existingMTP_PROGRESS_SLOW_TEST_SECONDSsetting (0disables reporting).Validation includes focused platform and extension unit tests, warning-free multi-target builds for the platform and both linked-source CI extensions, and publishing/running BrowserPlayground under Node. MSBuild validation was captured with binary logs.
Related to #2196, specifically #2196 (comment).