Stop using [Timeout] so that there should be no thread abort on the t…

[jnm2]

Possibly fixes #2288, at least closes it for now.

This attempts to simplify the situation so that a thread abort exception will never happen on the test execution thread.

I would have originally picked Assert.That(thread.Join(1000), ... over [Timeout(1000)] if it had crossed my mind when I first wrote it because it's less wasteful.

[ChrisMaddock]

@jnm2 - have you noticed these tests failed on a CI run? 🙂

https://ci.appveyor.com/project/CharliePoole/nunit/build/3.8.0-ci-04140-threading-t#L1213

[jnm2]

@ChrisMaddock Did not see them, but it looks like a fluke. The first test is not related an also had a timeout issue. It means the AppVeyor server was busy and that we should be increasing our timeout lengths. If we see this more than once we can go to two seconds. The amount is not the important thing, it's just a convenience.

1. Failed : NUnit.Framework.Internal.Execution.EventQueueTests+DequeueBlocking_StopTest.DequeueBlocking_Stop
   Test exceeded Timeout value of 1000ms
2. Failed : NUnit.Framework.Internal.ThreadUtilityTests.Abort
   Native message pump was not able to be interrupted to enable a managed thread abort.
   Expected: True
   But was: False
   at NUnit.Framework.Internal.ThreadUtilityTests.AbortOrKillThreadWithMessagePump(Boolean kill)
3. Failed : NUnit.Framework.Internal.ThreadUtilityTests.Kill
   Native message pump was not able to be interrupted to enable a managed thread abort.
   Expected: True
   But was: False
   at NUnit.Framework.Internal.ThreadUtilityTests.AbortOrKillThreadWithMessagePump(Boolean kill)

[ChrisMaddock]

So long as you're happy - I've reran CI. 🙂

[jnm2]

Yeah. But does that make sense though? AppVeyor inserts arbitrary delays sometimes and I don't think there's anything else we can do about it, except perhaps getting the enterprise product after joining the .NET Foundation.

[ChrisMaddock]

Yes, sorry. Maybe that reply didn't read how I meant it. 😄

Sure, Appveyor can sometimes lag a little - it just seemed coincidental that the two tests you'd just added were the ones to fail. If you're happy that's all it is however, we can merge, and see how it goes.

Rather than upping the Timeout - I think we could use RetryAttribute. Unfortunately, I think #2134 might be a bug that would prevent that from working currently. 😞

CI has passed now, merging. 👍

[jnm2]

Sorry, no worries! Retry is an interesting idea, but is that really any different than doubling the timeout?

[ChrisMaddock]

My thinking was, it allows for occasional machine lag, but ensures the action still can be completed within the time specified. So adding Retry would ensure the action can be completed within 1000ms, most-of-the-time. Doubling the timeout would only be ensuring the action can be completed within 2000ms - so 1500ms would be fine.

Perhaps less relevant for these ones, but I'm not sure if we use Timeout for performance purposes anywhere?

[jnm2]

Ah, that makes sense for tests where we care about the amount of time it takes. I'm not sure if we have any of those. I've had mixed luck with them myself at work on our build server.