Uh oh!
There was an error while loading. Please reload this page.
Harden acceptance test timing assertions - #10910
Harden acceptance test timing assertions#10910Amaury Levé (Evangelink) wants to merge 5 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
What changed in this PR
Hardens acceptance-test timing checks against slow CI agents while preserving regression detection.
Changes:
- Replaces tight numeric ceilings with documented 30-second limits.
- Extends blocking fixtures and removes a redundant timing assertion.
- Adds contributor guidance for process-level timing checks.
| File | Description |
|---|---|
.github/copilot-instructions.md | Documents robust timing-assertion practices. |
DataConsumerThroughputTests.cs | Adds a CI-tolerant throughput limit. |
TestHostProcessLifetimeHandlerTests.cs | Relaxes timing limits and extends blocking operations. |
TimeoutWhenExpiresTests.cs | Removes the redundant stopwatch assertion. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/TestHostProcessLifetimeHandlerTests.cs — WaitAsync times out only the proxy await; it cancels neither this polling task nor the in-flight… |
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/TestHostProcessLifetimeHandlerTests.cs — WaitAsync times out only the proxy await; it cancels neither this polling task nor the in-flight… View resolved comment |
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/TestHostProcessLifetimeHandlerTests.cs:71
- This still has an observation race: the child writes
finalizationStartedFileimmediately before entering its 10-second sleep, but the stopwatch starts only when the parent is scheduled and notices that file. If the parent is delayed, some or all of the blocking interval is omitted, so a broken finalization timeout can satisfy the five-second assertion. Use a two-way rendezvous: have the child publish a ready marker and wait for a release marker, then start the stopwatch before releasing it into the blocking callback.
This issue also appears on line 103 of the same file.
await WaitForFileAsync(finalizationStartedFile, markerTimeout.Token);
markerTimeout.CancelAfter(Timeout.InfiniteTimeSpan);
var stopwatch = Stopwatch.StartNew();
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/TestHostProcessLifetimeHandlerTests.cs:105
- The same observation race exists for disposal: the child appends the attempt marker and immediately begins its 10-second sleep, while this stopwatch starts only after the parent polling loop runs. Parent scheduling delay therefore gets subtracted from the measured disposal time and can let a regressed timeout pass. Add a child-to-parent ready marker plus a parent-to-child release marker, starting the stopwatch before releasing disposal to block.
await WaitForFileAsync(disposalAttemptsFile, markerTimeout.Token);
markerTimeout.CancelAfter(Timeout.InfiniteTimeSpan);
var stopwatch = Stopwatch.StartNew();
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Amaury Levé (Evangelink)
commented
Sep 1, 2026
Addressed the two suppressed observation-race findings from the latest Copilot review in |
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/TestHostProcessLifetimeHandlerTests.cs — Stopwatch starts when execution is released, not when OnTestHostProcessExitedAsync begins (the… |
Suppressed comments (1)
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/TestHostProcessLifetimeHandlerTests.cs:115
- This stopwatch also starts at execution release rather than after
Disposebegins (the disposal-attempt marker is written at callback entry). Consequently, the test does not implement the callback-scoped timing described by the PR and includes timeout/pre-disposal work. Either wait fordisposalAttemptsFilebefore starting timing, or revise the PR description to state the actual scope.
var stopwatch = Stopwatch.StartNew();
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/TestHostProcessLifetimeHandlerTests.cs — Stopwatch starts when execution is released, not when OnTestHostProcessExitedAsync begins (the… View resolved comment |

Hardcoded wall-clock ceilings made acceptance tests susceptible to loaded CI agents, while one attribute-precedence check raced against the exact configured timeout with no margin.
TimeSpanlimits and document their CI allowance.1000mstimeout already proves attribute precedence.Validation: packed the repository and ran the affected MTP and MSTest acceptance classes, including the revised lifetime-handler tests across all targeted frameworks.
Fixes#10899