Add support for CancelAfterAttribute #4386
Conversation
manfred-brands
force-pushed
the
feature/4385_CancelAfterAttribute
branch
from
d966f35 to
f1f1789
Compare
|
Convert to draft to see why the Unix based build were hanging. |
manfred-brands
force-pushed
the
feature/4385_CancelAfterAttribute
branch
from
f1f1789 to
9667f43
Compare
This is a cooperative multitasking alternative to TimeoutAttribute
manfred-brands
force-pushed
the
feature/4385_CancelAfterAttribute
branch
from
9667f43 to
ecadfd6
Compare
|
This introduces a new Note that this is cooperative and only works if test code (or code called from there) passes in and obeys this token. |
|
Congrats on finding the underlying linux issue @manfred-brands . I'll try to take a look within the week |
| { | ||
| // Because of the debugger possibly attaching after the test method is started | ||
| // we use a 2-prong approach where we only cancel the test's cancellation token | ||
| // if the debugger is not attached when the timer expires. |
There was a problem hiding this comment.
Thanks for adding this. Good thinking to have this caveat about the debugger
| public void InfiniteLoopWith50msTimeout() | ||
| { | ||
| while (true) { } | ||
| Thread.Sleep(5000); |
There was a problem hiding this comment.
👍 thank you for this update too
There was a problem hiding this comment.
nitpick: Actually, I feel like this may no longer do what the test method name says. Is it worth keeping keeping one busy wait in place to simulate a CPU-bound long running test?
There was a problem hiding this comment.
I mainly made this change to prevent the builds hanging and taking up unnecessary resources.
Yes, they probably should be renamed.
I don't see a point in making a CPU-bound versions. It should not make a difference in 'interuptability'.
| { | ||
| Assert.That(value, Is.EqualTo(1)); | ||
| Assert.That(cancellationToken, Is.Not.EqualTo(CancellationToken.None)); | ||
| Assert.That(cancellationToken, Is.EqualTo(TestContext.CurrentContext.CancellationToken)); |
There was a problem hiding this comment.
Thanks @manfred-brands nicely done getting this working. This looks good to me at first glance, though there is enough here and I am sleepy enough that I would appreciate a second pair of eyes from either myself later or another team member.
@OsirisTerje or @jnm2 Are either of you are up to a review?
|
ping @nunit/framework-team Any one else for a 2nd review? |
|
@manfred-brands There are a lot of changes/additions here. I can't really wrap my brain around it. Is there anything in particular in the code you're worried about? |
@OsirisTerje Not really worried, but could you look at the changes to |
|
@nunit/framework-team Does anyone has any objections to me merging this? |
|
Suggest you just merge it, and then we check the dev build afterwards. I could not see anything that raised any worries here. |
3 participants
Fixes #4385 #1821
This is a cooperative multitasking alternative to TimeoutAttribute