Test error in TestContextOneTimeTearDownTests.

[OsirisTerje]

This test have a set of Asserts in its OneTimeTearDown method.
These asserts show up as errors in Rider and Resharper, but in dotnet test, nor in Test Explorer in Visual Studio.

I have moved it into a separate repro test https://github.com/nunit/nunit.issues/tree/main/Issue4906

There are several issues here:

1. The test fails, so we have an error we have not detected.
   This the essence of this Issue.
   This test seems to have been written way back, in 2014/2015
   The testfixture has one Explicit test, and checks that tthe Result.SkipCOunt is equalto 1. It is not.
   

2. (This pt should probably be transferred to the adapter) Rider/Resharper detects that this is a fault. It is not reported in VS/dotnet , it stops in the adapter. Enabling the dump I see it in the response from the engine/framework, but as it is test output, we have nowhere to put that output.
   I know that MSTest adds a kind of extra test node in these cases. We could do the same. In NUnit all suites are also tests, and might have information. It would make sense to be able to show that. Rider does this too, so you see the node is failing.
   In VS it is shown in the Output/Test window:
   
   Further, NUnitLite does show it, but it is suppressed somewhere in out NUnit build output. I see it in the repro code I made.

Dump file output:

In the dump file we can also see that the skippedcount is 0, ref pt 1 here.

The repro is here: https://github.com/nunit/nunit.issues/tree/main/Issue4906

3. The repro has been enabled for NUNitLite.
   When running with VS the error is visible in the Output/Test as mentioned above.
   When running with NUnitLite, it is not there, and debugging it, show SkippedCount = 1. Which is correct. I have no clue yet why it works when seperated out into this repro, and ONLY when running with NUnitLite.

@manfred-brands @stevenaw Comments ? @rprouse @CharliePoole You guys wrote this test 10 years ago. Does the console show this node? Is this pre-adapter ? Any idea why NUnitLite is confused ?

[UPDATE]
Just checked with the console:

It doesn't react to the test either.

But Rider does:

[CharliePoole]

Each runner decides what to pay attention to in the output. Historically, the console ignored setup and teardown errors of all kinds and only reported on test cases. At some point (?) we decided to report assertion failures in setup and (I believe later) in teardown.

Also, fwiw, at one time we decided that any assertion failure in a teardown was an error, rather than a failed test. That is, it was considered a mistake to have assertions in a teardown, although it wasn't something we could enforce. I'm not sure if that notion has been consistently maintained, however.

[stevenaw]

Thanks for that historical context @CharliePoole
Avoiding assertions in teardowns is something I've seen consistently recommended here, though as you say, it can't be enforced.

Within the framework side, in response to @OsirisTerje 's point 1, I wonder if we should perhaps rewrite these tests a bit.

We also have #937 as an older issue which aims to remove direct Explicit usage in our tests. If that were still desired then one (untested) option here could be to simply move much of the structure into the TestData project in order to run and assert against the results within the framework tests themselves. That would also allow us to keep the implicit validation that the Explicit test doesn't run - if that were still desired or needed of course.

[stevenaw]

Interesting. It appears that the SkipCount line was added specifically to validate the Explicit handling... at least according the the commit message: 043ec55

Perhaps then something has changed to prevent Explicit tests being "skipped"?

[manfred-brands]

it was considered a mistake to have assertions in a teardown

At our work we have a base class that in the OneTimeSetUp subscribes to UnobservedTaskExceptions and in the OneTimeTearDown unsubscribes.

Our base class asserts in the per test TearDown that no exceptions were caught, rethrowing them if there were any.
This correctly fails the test and output is logged in TestExplorer.

If any tasks are still running when NUnit thinks the test is finished, it won't be reported.
We also have a check in the OneTimeTearDown for such case.
But that doesn't break the tests.

NUnit itself, at the moment, doesn't see any exceptions raised in different threads.
Those actually can cause NUnit to be killed.
That could be something to consider adding.

[stevenaw]

I'll check a few early versions tomorrow, but I am beginning to suspect that behavior may've changed prior to NUnit 3.0's launch. A likely solution would be to use ExplicitCount here instead, in addition to any other modifications we may want to remove the assertions from a OneTimeTearDown.

[stevenaw]

I had a hard time understanding the intended SkipCount behavior but what I can glean is that the Explicit test may've been "skipped" at one point whereas now it is never processed or included in result counts at all. I'd think any adjustments on the framework side should probably adjust the count accordingly rather that removing the assertion.

[stevenaw]

Interestingly, moving the tests into the testdata project seems to allow the same assertions to pass
main...onetimeteardowntests

EDIT: I'll try and get some PR up soon, including something to validate that failed asserts in these lifecycle methods result in errors

[stevenaw]

I'm back from vacation. One of the main reasons I haven't pushed a PR here yet is I would like to see if VS is passing in any filters here which could explain the difference in behavior. I'll be trying to investigate that soon