nunit
nunit copied to clipboard
nunit
Feature/4021 cancellationtoken
Fixes #3859 by setting both message and label. Fixes #4021
It doesn't fix the current behaviour of TimeoutAttribute but allows for a modern alternative.
Two commits:
- Add CancellationToken to Test(Execution)Context which will be cancelled on Timeout.
- Allow CancellationToken to be injected into Test methods
If accepted requires update to nunit.analyzers rule NUnit1027 which checks parameters, to allow for CancellationToken parameter, which must be last.
I've tried to make the failing CIs run, but I'm not able to force trigger either. So we probably need to force push a new commit (or force push the latest).
Thanks @mikkelbu You were right. I added a test similar to yours and updated the code to ensure it works.
Hi @manfred-brands Just wanted to circle back to this. I've been waiting to get involved in this PR myself until I had a decent amount of time to sink into understanding it. I think this would be a great fix to eventually land into a release as this seems to be a recurring issue for people
I hope to be able to get to it within the coming weeks. Can you help me understand if it's in need of a review or if it's awaiting changes?
I hope to be able to get to it within the coming weeks. Can you help me understand if it's in need of a review or if it's awaiting changes?
Hi @stevenaw this is not a fix for the TimeOutAttribute on non-Framework runtimes.
I'm now leaning to introduce a new CancelAfterAttribute instead for this to make it clear it is something completely different.
The current code works, I just rebased it today on v13.3-dev.
It is still using the TimeoutAttribute.
The changes are the introduction of a CancellationToken to the TestContext.
The tests can either get this from the TestContext or specify an additional test method parameter.
Hi @stevenaw this is not a fix for the
TimeOutAttributeon non-Framework runtimes. I'm now leaning to introduce a newCancelAfterAttributeinstead for this to make it clear it is something completely different.
Sorry, I'd thought this was the fix for non-framework runtimes. Can you help me understand how it's different?
@manfred-brands What is next step for this PR ?
Good question. We cannot "fix" timeout attribute for .NET Core as there is no Thread.Abort to stop any runaway test. Personally, I don't like reporting an error and then let the test continue running in the background. There will be too many test that will fail a subsequent test after that. So for new code, I suggest to deprecate TimeOutAttribute and add a CancelAfterAttribute and the user needs to respect the passed in CancellationToken.
If that is acceptable for the @nunit/framework-team, I can update this PR to introduce that feature.
We cannot "fix" timeout attribute for .NET Core as there is no Thread.Abort to stop any runaway test. Personally, I don't like reporting an error and then let the test continue running in the background. There will be too many test that will fail a subsequent test after that. So for new code, I suggest to deprecate TimeOutAttribute and add a CancelAfterAttribute and the user needs to respect the passed in CancellationToken.
@manfred-brands Thanks for following up on this. The lack of Thread.Abort() on .NETCore is a real issue. Perhaps it's just lack of familiarity on my part, but would having the Timeout command cancel the CTS not allow for cooperative cancellation if a test opts in to using the newly-exposed token (at least for long-running tests which use operations which support CTs, if the test has opted into use the provided one)? Or are you saying we can't fix it to "just work" without also requiring each test to opt in by handling the token?
I also wouldn't mind hearing what some of the other reviewers on the PR think.
I think I missed something; why does adding the CancellationToken depend on having a solution for the lack of Thread.Abort in .NET Core+?
I think I missed something; why does adding the
CancellationTokendepend on having a solution for the lack ofThread.Abortin .NET Core+?
Correct. Hence my suggestion for a CancelAfterAttribute iso using the existing TimeoutAttribute
But we have not heard anything from the @nunit/framework-team if this is acceptable.
I will update the PR to only add the new attribute and functionality without affecting the existing timeout behaviour.
Then end-user can switch to the new cooperative cancellation if they want to.
@manfred-brands Just doing a bit of cleanup here. Should this be closed as superseded by https://github.com/nunit/nunit/pull/4386 based on your comment in https://github.com/nunit/nunit/issues/4447#issuecomment-1661516745 ?