Fix flaky tests: realistic timeouts and robust assertions#13250
Draft
JanProvaznik wants to merge 1 commit intodotnet:mainfrom
Draft
Fix flaky tests: realistic timeouts and robust assertions#13250JanProvaznik wants to merge 1 commit intodotnet:mainfrom
JanProvaznik wants to merge 1 commit intodotnet:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce CI flakiness across several unit test projects by adjusting unrealistic timeouts, bounding potentially infinite waits, and making assertions more tolerant of non-deterministic warnings.
Changes:
- Increased various test timeouts / waits (e.g.,
Exec.Timeout,WaitOne,WaitForExit) to better match real scheduling and CI conditions. - Adjusted
Exec_Tests.Timeoutwarning assertions to tolerate additional intermittent warnings (e.g., MSB5018). - Tweaked
GenerateResourcetimestamp-related sleeps and removed a redundant delay to reduce unnecessary wall-clock time while preserving determinism.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Tasks.UnitTests/ResourceHandling/GenerateResource_Tests.cs | Adjusts sleeps and file timestamp updates in incremental rebuild tests. |
| src/Tasks.UnitTests/ResourceHandling/GenerateResourceOutOfProc_Tests.cs | Mirrors timestamp/sleep adjustments for out-of-proc resource generation tests. |
| src/Tasks.UnitTests/Exec_Tests.cs | Raises Exec.Timeout and makes warning assertions more robust to environmental noise. |
| src/Tasks.UnitTests/DownloadFile_Tests.cs | Increases cancellation completion wait bound and adds a clearer assertion message. |
| src/MSBuild.UnitTests/MSBuildServer_Tests.cs | Adds bounded waits to avoid hangs and increases shutdown/kill timeouts. |
| src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs | Increases timeouts around task host process lifecycle verification. |
| src/Build.UnitTests/BackEnd/TaskBuilder_Tests.cs | Increases synchronization timeout for cancellation-related build test. |
Comments suppressed due to low confidence (1)
src/MSBuild.UnitTests/MSBuildServer_Tests.cs:208
- This test toggles MSBUILDUSESERVER via Environment.SetEnvironmentVariable, which bypasses TestEnvironment tracking and can leak environment state to subsequent tests if this test fails early (or forgets to restore). Prefer using _env.SetEnvironmentVariable for these toggles (or capture/restore the original value) so the environment is reliably reverted on dispose.
mre.WaitOne(60_000).ShouldBeTrue("Timed out waiting for marker file creation indicating server is busy.");
_output.WriteLine("It's OK to go ahead.");
Environment.SetEnvironmentVariable("MSBUILDUSESERVER", "0");
src/Tasks.UnitTests/ResourceHandling/GenerateResourceOutOfProc_Tests.cs
Outdated
Show resolved
Hide resolved
JanProvaznik
force-pushed
the
fix-flaky-tests-timeouts
branch
from
150b90d to
dd93207
Compare
Member
Author
|
(running several times, 1 pass) |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
- CanceledTasksDoNotLogMSB4181: WaitOne 2s→30s (max bound, no CI impact) - TaskNodesDieAfterBuild: WaitForExit 3s→15s, retry Sleep 2s→3s - MSBuildServer: KillTree 1s→10s, bounded infinite mre.WaitOne() to 60s with assertion, WaitForExit 10s→30s with assertion - Exec_Tests.Timeout: warning count exact match (1 or 2) changed to ≥1 to handle non-deterministic MSB5018 from temp file lock contention (documented in dotnet#9176 across 4+ CI failures 2023-2025) - DownloadFile.CanBeCanceled: Wait 1.5s→10s - GenerateResource timestamp tests: Sleep 200ms→500ms across 8 sites, 100ms→500ms for NTFS (1.5s for macOS HFS); removed redundant 1s sleep in ForceSomeOutOfDate - Net CI wall-clock impact: ~+3s across entire test suite
JanProvaznik
force-pushed
the
fix-flaky-tests-timeouts
branch
from
dd93207 to
455ca6c
Compare
Member
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix the top flaky tests identified from analyzing 100 recent CI builds (49% failure rate, 40/49 due to test flakes).
Changes
Genuine design fixes
ShouldBe(1)orShouldBe(2)) toShouldBeGreaterThanOrEqualTo(1). The flake is MSB5018 temp file lock contention—documented in [Flaky test] Microsoft.Build.UnitTests.Exec_Tests.Timeout #9176 across 4+ CI failures from 2023-2025, where another process (likely AV) holds a lock on the.exec.cmdtemp file, generating an additional warning. The timeout value (5ms) is kept as-is.mre.WaitOne(): Was infinite with no timeout — genuine hang risk. Bounded to 60s with assertion message.WaitForExit: CheckedHasExitedafterWaitForExitreturned (redundant+racey). Replaced with singleWaitForExit(30_000).ShouldBeTrue(msg).Timeout increases (correct synchronization, too tight for CI)
These are max bounds that return immediately when the condition is met—zero cost in the happy path.
CI impact
Validation