Apartment attribute ignored when .runsettings contains DefaultTimeout about nunit HOT 33 OPEN

You can't see it in the test adapter, but NUnit can fail a test in two ways. One way is an assertion failure which fails the test. There is another failure state Error that means something is wrong with the test. The console runner shows each type of failure separately.

from nunit.

@OsirisTerje Yes, I think we should drop all Timeout code from anything but .NET Framework.

The TimeoutAttribute was marked Obsolete in NUnit 4, but the code was left in for "backward compatibility", but in this case it was keeping non-properly functioning code with more side effects than expected and no actual benefits.

Most Timeout usages are for emergency fallbacks and never expected to be hit, especially the DefaultTimeout.

Does this mean we can remove it "silently" in NUnit 3.xx?
I mean allow the TimeoutAttribute so code compiles, but not actually create a TimeoutCommand.
It might actually be better than the current code, where the framework claims a test has timed out, but it keeps on running anyhow interfering with the next test.

The default timeout can be handled with --blame-hang-timeout <TIMESPAN> See dotnet test CLI

from nunit.

Can you raise PR to the https://github.com/nunit/nunit.issues repo with your repro solution ?

from nunit.

Sure thing, PR here: nunit/nunit.issues#4

from nunit.

Thanks!
Link to repro: https://github.com/nunit/nunit.issues/tree/main/Issue4658

from nunit.

It is also kind of a duplicate of #4119 , just the timeout being added differently.

Also, timeout doesn't work properly anymore outside .net framework (see #4021), but it should not break the apartmentstate.

Also, defaulttimeout will have no effect in .net 8, so there is actually no point in adding it.

from nunit.

@manfred-brands @stevenaw The defaulttimeout is handled in the CreateTestExecutionContext method in the NUNitTestAssemblyrunner class.

Is it any point at all in letting this set the TestCaseTimeout ? Should we skip this for non net framework ?

Also, in the SimpleWorkItem class we set the TimeoutCommand when there is no Cancellation set. Should we also skip this if non net framework?

In the TimeoutCommand , when we don't have any Thread Abort (for non-net framework), we explicitly set the test to run in a separate thread. This will of course kill the ApartmentState.STA. Should we revisit this ?

This explicit thread setting was introduced in a change in the TimeoutCommand in PR #3366 in order to fix #3054 and #3282, and released in version 3.13.0.

Since the Timeout command has no effect in anything but .net framework, I wonder if we should just drop the timeout command for .net (core). Thus deleting this code change too. If we do that we should let the analyzer react on this to warn devs on wrong use of the Timeout attribute. The defaulttimeout is worse though, since it comes in through the settings.

from nunit.

I see a few different things going on here.

Timeout Support on STA

This, unfortunately, is not a new issue in NUnit. We haven't been able to dig into the failure but as @OsirisTerje mentioned, it came up recently in the 3.x release too. #4188 is another issue outlining it when run through NUnitlite. The discussion there mentioned that this may've worked in earlier framework releases but could've been broken at some point during or after version 3.7 when test parallelization was added. At the time in the thread, the thought was that explicit STA had low usage which then lowered the priority of any potential fixes.

Global Specification of Maximum Test Duration

We've been suggesting moving towards CancelAfter over Timeout as it provides cooperative cancellation. This definitely helps nudge tests towards a safer model, but perhaps we may want to consider if we should support a global cancel after "timeout" period (for lack of a better word) much like how DefaultTimeout from a runsettings file or command line current applies to the Timeout attribute.

Should we reuse the same setting to provide this? From an oversimplified standpoint one could argue it serves the same purpose of telling the framework how long any given test should be allowed to take - independent of cancellation model or mechanism. The downside to globally and silently applying this as a cancelafter duration, of course, is that a test suite which gets upgraded from netfx to modern may forget or be unaware of the need to then move towards the cancellation model. So we'd have to be careful.

I do like the idea of supporting some global setting for this in the framework though. It's true that the dotnet test CLI has some things built-in but we may want to consider the needs of console runner users too even if that project doesn't have a large amount of active development currently. NUnitlite is another runner we may also want to consider usage on here too. Supporting a setting like this framework level could then allow users on any runner to use it.

Cancellation Mechanism

It also seems like in nudging towards this new model though, we're also asking everyone to update their tests and potentially their application code to also support it. This does seem like a big ask to me. I agree about nudging towards best practices but I like the idea of keeping an alternate in place for those - perhaps a significant minority or even majority - who may be bothered by the notion of rewriting everything. To them they may prefer an existing imperfect solution using ThreadAbort on .NET Framework over having to rewrite code. So I like the idea of keeping all that code around in NUnit 3 and in the net4x distro of NUnit4. That gives an out for those who don't want to move even if it's an imperfect model.

Forwards Compatibility

The tricky point to me then, to me, is how to nudge towards what is intrinsically opt-in model of cooperative cancellation with minimal friction to make any eventual move to NUnit 4 as easy as possible. To me, this means feature parity as much as possible between the two attributes within the framework. I also like the idea of making noise (ie: warnings) for if we can detect a misconfiguration.

Solution to the issue at hand
I think the existing fallback solution for Timeout is pretty good in NUnit 4. It seems we've found a bug around output writer reuse (#4598) but I think that's easily fixable. It wouldn't work with STA, but I think that's a longer-standing potentially separate issue.

I also like the idea of having a setting read by the framework to control run-level "maximum test duration" similar to how Timeout uses TimeoutDuration presently. I feel like it would make user upgrades to NUnit 4 easier irrespective of their chosen test runner. Doing this may require a bit of discussion over what we name it and which mechanism should be considered "default" on which runtimes. Doing this may mean we consider warnings for potential misconfigurations. I'm unsure how feasible it would be to detect using a cancellation-based mechanism setting yet having no tests in the run ever opting-in to using the framework-provided cancellation token.

from nunit.

My previous reply looks much longer on a phone.

I suppose I'm suggesting a setting-based solution here. Perhaps we introduce a new DefaultTimeoutMode setting to pair with the existing DefaultTimeout setting to better disambiguate and control maximum test duration at this level. DefaultTimeout could then be used to control max test duration regardless of cancellation mechanism

from nunit.

@stevenaw Adding a global CancelAfter does nothing without changing code as nothing will obey the cancellation token.

As said before, in 99% of the cases the tests don't timeout, so we could stop create a new thread and fix the STA issue.
For the 1% of cases that actually times-out, we need to add a global timeout to our runners (console-runner or nunitlite),
like dotnet test CLI and if a test doesn't finish in such time abort the whole test run and report the hanging test.

Yes, the behaviour on .NET Framework will be different as we can actually abort a single test, but even there we won't execute any finally code in the test itself potentially leaving code in an undefined state. Not all tests use SetUp and TearDown.

The only time a timeout makes sense is if a test depends on an outside component, like a web-api and we want to test our interaction with it.

from nunit.

@manfred-brands @stevenaw Since this is not working in net core , should we also back port this to the 3.X series? Disabling Timeout would make it work there too, even if we don't backport the CancellationToken change.

from nunit.

@OsirisTerje Yes, once we are happy with the removal in V4, we should also do that in V3.

from nunit.

Agreed, any fix here should be back ported if present on v3.

I think it's worth considering that these attributes may be used for more than out-of-proc work. A use case for ending a long-running test could be if a test were added to fix a performance issue to prevent future regressions. We have a similar test in the framework suite for EquivalentTo between large string collections. An expensive CPU-bound task may also check a cancellation token periodically between steps (though this example does not apply to our suite it could to others).

@OsirisTerje are you saying the STA issue is related to the debugging support added in that PR? Interesting if so

I'm a little hesitant to push the timeout responsibility up to the runner if it can be kept in-framework. There are third -party runners out there which would then be left unsupported. @manfred-brands can you explain a bit more about why you don't want to do this in-framework if it were an option there?

from nunit.

@stevenaw Yes,exactly so. Regression bug introduced there.

This explicit thread setting was introduced in a change in the TimeoutCommand in PR #3366 in order to fix #3054 and #3282, and released in version 3.13.0.

This is a bug, even then, and our tests did not catch it. Cause trouble for the ApartmentState.STA, and also for SingleThread attribute.

from nunit.

@OsirisTerje
I admit, it's good to hear it was introduced more recently that 3.7.0. I'm not disputing the bug nature either, just surprised by the recency.

So, is the change you're proposing to just remove the debugger-related change or our entire fallback code? I'm fine with the former but more hesitant about the latter

from nunit.

@stevenaw The code there starts the test in a new thread. It seems to be a fix for both the debugger and the "Timeout attributes causes all tests to count as failures". I haven't dug into the details more than seeing that thread being created, which is so wrong.

And since Timeout have no purpose in net core I can't see why we should not do as @manfred-brands suggest, and simple make it a "do-nothing" command.

Btw, I am not sure what you mean by "fallback code" there, beside the code I mention with the thread. Is there anything else there?

from nunit.

Ok, I'm fine with removing all the threading code if that's what we prefer. Perhaps my notion of "fallback code" is getting muddied somewhere. I think it's still important to be able to fail a test if it exceeds a certain duration but we have MaxTime for that.

Do we want to report a diagnostic (warning?) for Timeout usages on netcore builds to ask users to consider MaxTime instead? Our would we want to duplicate that MaxTime behaviour into the Timeout attribute on netcore so that instead of doing "nothing" that TimeoutAttribute will instead allow the test method to complete and then fail it after if it runs longer than desired. Bringing that behaviour into Timeout on netcore would avoid users having to #ifdef all their tests to target different attributes on a multi-targeted suite.

I still like the idea of further discussing a setting-based way to define a maximum limit for any single test in a suite as it allows the same suite to have different failure thresholds depending on which runner, runtime or environment it may target, but that is an idea for another ticket.

from nunit.

I'm a little hesitant to push the timeout responsibility up to the runner if it can be kept in-framework. There are third -party runners out there which would then be left unsupported. @manfred-brands can you explain a bit more about why you don't want to do this in-framework if it were an option there?

I'm still not sure where the responsibility of the runner end and where the framework starts.
I doubt if every runner calls NUnitTestAssemblyRunner.Run or do they something themselves?

We can keep the TimeoutCommand and even use one and the same version for both .NET Framework and core.
I.e. run the test on the current thread with a timer.
But update the ThreadUtility.Abort to no longer abort the thread but cancel the test.

One thing I thought of just now. Assuming that tests indirectly call Assert.That somehow, we could add a check on our CancellationToken in there: TestContext.CurrentContext.CancellationToken.ThrowIfCancellationRequested(); Or even add it to TestExecutionContext.IncrementAssertCount.

That way end-user code doesn't need modifying to obey the CancellationToken.

The only tests that then still can hang are tests that don't do any Assert calls.
For that all we can do is print out the current test method (using SendMessage?) and Environment.Exit(99).

from nunit.

Oh that's a neat idea @manfred-brands !

EDIT: I think I like that more than my notion of bringing MaxTime into Timeout as it allows earlier feedback

from nunit.

@manfred-brands I just realized that a suite relying heavily on a third-party assertion library (FluentAssertions, etc) may not call Assert.That directly so perhaps IncrementAssertionCount could be more broadly compatible

from nunit.

@stevenaw I just loaded and investigated, but FluentAssertions doesn't call into any NUnit methods.
Only when a tests fails according to their own assertions, they call into a Test Framework specific code which uses reflection to find NUnit.Framework.AssertionException and to instantiate it.

from nunit.

Thanks for catching that @manfred-brands ! They must be doing that to avoid a hard dependency.
I'm also realizing now that it's plausible that tests could exist with no asserts at all. For example, given the following:

[Test, Timeout(60_000)]
public void Test1()
{
    // My potentially long-running stuff
    // With no asserts
}

If the method body takes longer than 60 seconds the test will be marked as Failed. However, if the method completes in under 60 seconds, even without any asserts, the test will be marked as Passed. While potentially uncommon, it's reasonable if the purpose of the test is simply to verify that a piece of code completes in time.

@manfred-brands @OsirisTerje Lots of good ideas have been discussed here. Where have we ended up? Are we leaning towards having Timeout do nothing on net core, or having it run the test method to completion and then checking if it's exceeded (essentially an alias of MaxTime).

from nunit.

@stevenaw All issue, the STA issues #4658 and #4119 and the Test output from #4598 come from creating that new thread we cannot stop anyhow. So we need to remove that.
Then how to handle a "timeout":

.NET Framework call Thread.Abort

.NET Core, Set CancellationToken and check it in some code in TestExecutionContext.
This would work for code that does NUnit tests in an endless loop, but not for code that hangs outside our tests.

We could add a .NET 8.0 (or 7.0) target and use ControlledExecution.Run which is a Thread.Abort equivalent with the same downfalls.

This still leaves NET6.0 as a non-terminating runtime unless we call Environment.Exit which is drastic.

@OsirisTerje Are you ok with adding an extra target runtime?

from nunit.

Are you ok with adding an extra target runtime?

It will make the package 50% larger.

On the other hand, .net 8 do have a lot of changes in itself, so it could be wise to actually compromise on that. We can't remove any of the two others, so we have then to live with the extra baggage.

I would like to hear the others opinion on this:

@jnm2 @rprouse @stevenaw @mikkelbu @SeanKilleen

This still leaves NET6.0 as a non-terminating runtime unless we call Environment.Exit which is drastic.

My 5 cents: Leave it as is, non-terminating.

Are we leaning towards having Timeout do nothing on net core, or having it run the test method to completion and then checking if it's exceeded (essentially an alias of MaxTime)

Imho: Do nothing. I think it will be harder to say that it works as an alias in .net 6 but terminates i net framework and possibly net 8. We should write an article about the usage though (not working in net 6, use MaxTime instead) , and point people to it in all issues that are raised. We would have to do that in all cases anyway

from nunit.

In this case, I think we should raise an error when incompatible features are used. When we can't run the test STA, it should error the test, not rely on the test to fail.

from nunit.

@rprouse How do we differ between error and fault?

Or do you mean that we should force the test to fail?

from nunit.

Agree, then we should do that.

from nunit.

It is not that we cannot run the test in STA, we cannot timeout. Should we then not reject timeout usage instead?

from nunit.

Yes, that's how I understand it too, if we see Timeout in a test for .net 6, we can set it as Error.

People will then get the Error instead of being surprised that the test actually don't work as intended - WHEN a timeout actually should have happened. Many test may have the Timeout attribute as a kind of last line of defense, and for them it might feel annoying. But in those cases they can remove the attribute altogether. They will at least now.

But perhaps we should have an option for turning this on/off. Ignore or Error.

from nunit.

I'm on my computer now, so I can see the code. If you look at ITestResult, it has a ResultState. The ResultState class has a number of pre-defined results. When tests fail normally through assertions, we give them a Failure result. I am suggesting that we give them an Error result to indicate that we cannot run the test because of the conflicting requirements.

There is also a Warning state, but I don't think that results as a failed test, only a warning in the output which will inevitably be missed.

If I remember right, the main place Error is used is for tests where the SetUp or TearDown failed. I think it is also used if tests timeout.

from nunit.

I've tried to read through all the comments, but I must admit that I've had some trouble following all ideas. That being said I think it is fine to add another runtime target (for .NET 8) and I agree with Rob that we should error when incompatible features are used.

from nunit.

Agreed, I like the idea of reporting when STA + Timeout are used. I believe we're suggesting this for all builds and runtimes.

NET6 will be EOL this year (already!?) so I also agree with @mikkelbu about adding NET8 eventually too. I also like @manfred-brands 's idea of using ControlledExution.Run() for any eventual NET8 target as it'll finally unify behaviors of this attribute across runtimes.

I believe these solutions are all independent of #4598 . I believe that issue will also go away if we were to use ControlledExecution.Run - though that would still leave NET6 with that bug. I can keep working away on a PR to pursue reusing the output writer for discussion unless there are objections. It should be ready soon.

from nunit.

I will work on this and put up a PR.

from nunit.