Skip flaky ToolTaskThatTimeoutAndRetry test on CI#13668
Skip flaky ToolTaskThatTimeoutAndRetry test on CI#13668jankratochvilcz wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Disables the flaky ToolTaskThatTimeoutAndRetry unit test on CI by adding a CI-only skip attribute, aligning with the ongoing effort to reduce spurious CI failures (Fixes #13667).
Changes:
- Added a
[SkipOnCI(...)]attribute to skipToolTaskThatTimeoutAndRetrywhen running in CI.
| [SkipOnCI("This test is consistently flaky even after multiple attempts to stabilize it")] | ||
| public void ToolTaskThatTimeoutAndRetry(int repeats, bool timeoutOnFirstExecution) |
There was a problem hiding this comment.
SkipOnCI is only referenced here and isn’t defined anywhere in this repo (no SkipOnCIAttribute implementation under src/). If this attribute isn’t provided by the referenced xUnit packages, this will fail to compile or won’t actually skip on CI. Please either (1) add a SkipOnCIAttribute to the shared test infrastructure (e.g., src/UnitTests.Shared) that checks common CI env vars, or (2) switch to an existing conditional-skip mechanism already used in this repo.
Review Summary✅ Attribute validity
✅ Attribute placement
✅ Pragmatic approach given historyThis test has a well-documented history of flakiness with at least 3 prior stabilization attempts:
The test is fundamentally timing-dependent (it expects a 100ms 📋 Suggestions (non-blocking)
🔍 Test coverage impactThe test verifies that
Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
There was a problem hiding this comment.
LGTM with minor suggestion. The use of [SkipOnCI] is correct, the attribute is available from Microsoft.DotNet.XUnitV3Extensions, and placement on the [Theory] method is valid. Given the extensive history of stabilization attempts (3+ PRs), skipping this inherently timing-sensitive test on CI is a pragmatic choice.
Only suggestion: reference a tracking issue in the skip message to ensure the test doesn't stay permanently disabled without visibility.
Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
- #13668
search_pull_requests: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - #8544
search_pull_requests: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by Expert Code Review (on open) for issue #13668 · ● 6.7M
| [InlineData(1, false)] | ||
| [InlineData(3, false)] | ||
| [InlineData(3, true)] | ||
| [SkipOnCI("This test is consistently flaky even after multiple attempts to stabilize it")] |
There was a problem hiding this comment.
Suggestion (non-blocking): Consider adding a tracking issue URL to the skip message so it's clear this is intended to be temporary and there's a path to re-enablement. For example:
[SkipOnCI("https://github.com/dotnet/msbuild/issues/XXXX - consistently flaky due to timing sensitivity")]This follows the pattern used elsewhere in the repo (e.g., [WindowsOnlyFact(Skip = "https://github.com/dotnet/msbuild/issues/1250")]).
Disables the
ToolTaskThatTimeoutAndRetrytest on CI using[SkipOnCI]. This test has been consistently flaky despite multiple stabilization attempts.