-
Notifications
You must be signed in to change notification settings - Fork 289
Add TaskRunTestMethodAttribute with integrated timeout for non-cooperative cancellation #6808
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
- Created TaskRunTestMethodAttribute that wraps test execution in Task.Run - Updated UseCooperativeCancellationForTimeoutAnalyzer resources to mention TaskRunTestMethod - Updated UseCooperativeCancellationForTimeoutFixer to offer TaskRunTestMethod as alternative - Added test for new code fix option - Updated PublicAPI.Unshipped.txt with new public API - Updated all XLF localization files Co-authored-by: Evangelink <[email protected]>
- Simplified ExecuteAsync to avoid duplicate helper methods - Fixed logic for wrapping test invocation in Task.Run Co-authored-by: Evangelink <[email protected]>
- Use ExecuteWithTaskRunAsync helper for both code paths to reduce duplication - Ensures consistent Task.Run wrapping logic Co-authored-by: Evangelink <[email protected]>
- Modified UseCooperativeCancellationForTimeoutAnalyzer to check for TaskRunTestMethodAttribute - When TaskRunTestMethodAttribute is present, no diagnostic is reported since it provides Task.Run behavior - Added test to verify TaskRunTestMethodAttribute with Timeout doesn't trigger diagnostic - Fixed test to remove diagnostic marker from fixed code Co-authored-by: Evangelink <[email protected]>
- Added TaskRunTestMethodAttribute to WellKnownTypeNames - Updated analyzer to use SymbolEqualityComparer instead of string comparison - Supports inheritance from TaskRunTestMethodAttribute Co-authored-by: Evangelink <[email protected]>
- Improved comment about TaskRunTestMethodAttribute symbol handling - Extracted IsTaskRunTestMethodAttribute helper method for cleaner code - Better separation of concerns in the analyzer logic Co-authored-by: Evangelink <[email protected]>
src/TestFramework/TestFramework/Attributes/TestMethod/TaskRunTestMethodAttribute.cs
Outdated
Show resolved
Hide resolved
src/TestFramework/TestFramework/Attributes/TestMethod/TaskRunTestMethodAttribute.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="TaskRunTestMethodAttribute"/> class. | ||
| /// </summary> | ||
| public TaskRunTestMethodAttribute([CallerFilePath] string callerFilePath = "", [CallerLineNumber] int callerLineNumber = -1) |
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.
Don't we need a timeout parameter?
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.
Added timeout parameter to the constructor. The attribute now accepts TaskRunTestMethod(5000) where 5000 is the timeout in milliseconds. Commit: ac903bb
| [TaskRunTestMethod] | ||
| [{|#0:Timeout(5000)|}] |
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 think what we would need is [TaskRunTestMethod(5000)]. The whole implementation then is in the attribute, and the adapter knows nothing about this.
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 have done a quick look through the app and we will need the adapter to inject a service so we can know about the timeout defined in run settings or testconfig.json. I have the feeling that to keep parity with the rest we will probably need to keep the Timeout attribute too.
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.
Couldn't we add the timeout value on ITestMethod?
Well, the ITestMethod implementation already has the timeout info, it's just not part of the interface.
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 please handle the timeout
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.
Implemented [TaskRunTestMethod(5000)] with timeout handling in the attribute itself. The code fix now extracts the timeout value from [Timeout] and creates [TaskRunTestMethod(timeout)], removing the need for a separate Timeout attribute. Commit: ac903bb
| { | ||
| // Run the test method in Task.Run so that we can stop awaiting it on timeout | ||
| // while allowing it to complete in the background | ||
| Task<TestResult[]> testTask = Task.Run(executeFunc); |
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.
If STA with specified in runsettings, we are breaking it here because threadpool threads are not STA and we need to create our own custom thread.
I'm also not sure if we should simply just always create our custom thread and never use the threadpool here.
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.
Added STA thread handling. The attribute now checks if the current thread is STA and creates a dedicated STA thread when needed (on Windows platforms), otherwise uses Task.Run with the thread pool. Commit: ac903bb
…estMethodAttribute.cs Co-authored-by: Youssef Victor <[email protected]>
…estMethodAttribute.cs Co-authored-by: Youssef Victor <[email protected]>
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'll need to do some extra prompting because TaskRunTestMethod doesn't prevent dangling tasks. Only cooperative mode on the Test method does. Instead this one is supposed to help with the logic of non cooperative cancellation.
- Added timeout parameter to TaskRunTestMethodAttribute constructors - Implemented timeout handling with Task.WhenAny for non-cooperative timeout - Added STA thread handling to respect thread apartment state - Updated code fix to extract timeout from Timeout attribute and pass to TaskRunTestMethod - Updated tests to use TaskRunTestMethod(5000) instead of separate Timeout attribute - Updated PublicAPI.Unshipped.txt with new signatures Addresses comments from @Youssef1313 and @Evangelink about needing timeout parameter and STA thread handling. Co-authored-by: Evangelink <[email protected]>
Updated the documentation to clarify that TaskRunTestMethod provides non-cooperative timeout handling (stops awaiting but task continues in background). For cooperative mode that prevents dangling tasks, users should use |
|
|
||
| // Run with timeout | ||
| Task<TestResult[]> testTask = RunOnThreadPoolOrCustomThreadAsync(executeFunc); | ||
| Task completedTask = await Task.WhenAny(testTask, Task.Delay(Timeout)).ConfigureAwait(false); |
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.
Is this pattern leaving the timer of Task.Delay running in the background?
Introduces a specialized test method attribute that wraps test execution in
Task.Runwith integrated timeout handling for non-cooperative cancellation scenarios. This provides an alternative to the globalUseCooperativeCancellationsetting by moving timeout logic into the attribute itself.Changes
New Attribute
TaskRunTestMethodAttribute(timeout)- Wraps test invocation inTask.Runwith timeout parameter (in milliseconds)TestMethodAttribute(followsSTATestMethodAttributepattern)Analyzer (MSTEST0045)
TaskRunTestMethodAttributeis presentCooperativeCancellation = trueand[TaskRunTestMethod(n)]Code Fix
CooperativeCancellation = trueon[Timeout]attribute[TestMethod]and[Timeout(n)]with[TaskRunTestMethod(n)][Timeout]attribute and passes to[TaskRunTestMethod][Timeout]attribute when applying TaskRunTestMethod fixUseTaskRunTestMethodFixAPI Surface
PublicAPI.Unshipped.txtwith timeout parameter and Timeout propertyWellKnownTypeNames.csUsage
Key Differences
Users can now explicitly choose the timeout behavior per-test rather than relying on global settings.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.