-
Notifications
You must be signed in to change notification settings - Fork 290
Add analyzer to detect Thread.Sleep and Task.Delay in test methods #6960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…detection Co-authored-by: Evangelink <[email protected]>
Co-authored-by: Evangelink <[email protected]>
| return; | ||
| } | ||
|
|
||
| // Check if the invocation is Task.Wait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be looking at Task.Delay instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree for the purpose of this rule, calls to Wait should be fine but we should instead catch Task.Delay.
@copilot please update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we're detecting Task.Wait (blocking call) not Task.Delay. Task.Delay is the recommended async alternative. The analyzer detects Thread.Sleep and Task.Wait as problematic patterns, while Task.Delay (used with await) is the suggested replacement. Commit: 79f86da
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to detect Task.Delay instead of Task.Wait. Task.Wait is now allowed as it's not an arbitrary delay. Commit: ae6e946
| /// MSTEST0059: <inheritdoc cref="Resources.AvoidBlockingCallsInTestsTitle"/>. | ||
| /// </summary> | ||
| [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)] | ||
| public sealed class AvoidBlockingCallsInTestsAnalyzer : DiagnosticAnalyzer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this name is accurate. It's not about blocking IMO. It's about arbitrary delays/sleeps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree! Please @copilot update the name, rule name etc to reflect that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to AvoidSleepAndDelayInTests to better reflect that it detects arbitrary delays/sleeps. Commit: 79f86da
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already renamed in previous commit 79f86da. The analyzer is now named AvoidSleepAndDelayInTests.
| MessageFormat, | ||
| Description, | ||
| Category.Design, | ||
| DiagnosticSeverity.Warning, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't put as Warning by default. That's too aggressive IMO.
It should be Info. And not even sure if should be enabled by default or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot update as info and make it disabled by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to Info severity and disabled by default. Commit: 79f86da
- Renamed AvoidBlockingCallsInTests to AvoidSleepAndDelayInTests to better reflect that it detects arbitrary delays/sleeps rather than blocking calls - Changed severity from Warning to Info - Changed isEnabledByDefault from true to false - Updated all resource strings and localization files - Updated AnalyzerReleases.Unshipped.md Co-authored-by: Evangelink <[email protected]>
- Updated analyzer to detect Task.Delay calls instead of Task.Wait - Task.Wait is now allowed, as it's not an arbitrary delay - Task.Delay without await is detected as problematic pattern - Updated all resource strings and localization files - Updated tests to reflect the new behavior: - Task.Delay without await now triggers diagnostic - await Task.Delay is allowed (no diagnostic) - Task.Wait is now allowed (no diagnostic) Co-authored-by: Evangelink <[email protected]>
Thread.Sleep() and Task.Delay() cause test flakiness when operations don't complete in fixed time windows. This analyzer detects these patterns at build time instead of via CodeQL, shifting feedback left.
Changes
New analyzer:
AvoidSleepAndDelayInTestsAnalyzer(MSTEST0059)Thread.Sleep()andTask.Delay()invocations in test methods and fixturesTest coverage: 13 test cases covering all fixture types, positive/negative scenarios, and async alternatives
Localization: Added resources for all 13 supported languages
Example
Note
This analyzer is disabled by default and reports as Info severity. Users can enable it in their
.editorconfigif they want to detect these patterns in their test code.The analyzer detects arbitrary delays (
Thread.Sleepand non-awaitedTask.Delay) that can cause test flakiness.Task.Wait()is not flagged as it represents synchronous waiting for task completion rather than an arbitrary delay.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.