-
Notifications
You must be signed in to change notification settings - Fork 723
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
Apartment attribute ignored when .runsettings contains DefaultTimeout #4658
Comments
Can you raise PR to the https://github.com/nunit/nunit.issues repo with your repro solution ? |
Sure thing, PR here: nunit/nunit.issues#4 |
Thanks! |
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. |
@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. |
@OsirisTerje Yes, I think we should drop all The Most Does this mean we can remove it "silently" in NUnit 3.xx? The default timeout can be handled with |
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 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 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 also like the idea of having a setting read by the framework to control run-level "maximum test duration" similar to how Timeout uses |
My previous reply looks much longer on a phone. I suppose I'm suggesting a setting-based solution here. Perhaps we introduce a new |
@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. 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 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. |
@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. |
@OsirisTerje Yes, once we are happy with the removal in V4, we should also do that in V3. |
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 @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? |
@stevenaw Yes,exactly so. Regression bug introduced there.
This is a bug, even then, and our tests did not catch it. Cause trouble for the ApartmentState.STA, and also for SingleThread attribute. |
@OsirisTerje 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 |
@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? |
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 Do we want to report a diagnostic (warning?) for Timeout usages on netcore builds to ask users to consider 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. |
I'm still not sure where the responsibility of the runner end and where the framework starts. We can keep the One thing I thought of just now. Assuming that tests indirectly call That way end-user code doesn't need modifying to obey the The only tests that then still can hang are tests that don't do any |
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 |
@manfred-brands I just realized that a suite relying heavily on a third-party assertion library (FluentAssertions, etc) may not call |
@stevenaw I just loaded and investigated, but FluentAssertions doesn't call into any NUnit methods. |
Thanks for catching that @manfred-brands ! They must be doing that to avoid a hard dependency.
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 |
@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. .NET Framework call .NET Core, Set CancellationToken and check it in some code in We could add a .NET 8.0 (or 7.0) target and use ControlledExecution.Run which is a This still leaves NET6.0 as a non-terminating runtime unless we call @OsirisTerje 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
My 5 cents: Leave it as is, non-terminating.
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 |
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. |
@rprouse How do we differ between error and fault? Or do you mean that we should force the test to fail? |
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. |
Agree, then we should do that. |
It is not that we cannot run the test in STA, we cannot timeout. Should we then not reject timeout usage instead? |
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. |
I'm on my computer now, so I can see the code. If you look at There is also a If I remember right, the main place |
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. |
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 I believe these solutions are all independent of #4598 . I believe that issue will also go away if we were to use |
I will work on this and put up a PR. |
I have a test fixure that uses the Apartment attribute to run tests in the STA. Below is a working simplification:
.csproj
.cs
This test passed without issue (via Visual Studio 2022's Test Explorer, vstest.console command line, dotnet test) until I started using a .runsettings file that included the DefaultTimeout property:
.runsettings
Now, the test fails. Below is the output shown when running dotnet test:
As a work-around, I will remove DefaultTimeout from the .runsettings file. I like having this property set though, as it helps to handle some tests that hang or otherwise cause our automated builds to run longer than they should.
It seems like this issue might be related to one logged a couple months ago: #4598
The text was updated successfully, but these errors were encountered: