Warning in TearDown is inconsistent with Assertion failure

[CharliePoole]

When an Assertion is used in TearDown and fails, an error is generated. That is, the result is Failed:Error rather than simply Failed. Similarly, failure of an Assumption generates an error.

NUnit has worked this way historically because it is generally necessary for the TearDown method to complete, in order to clean things up.

As we implemented Warnings, a Warning failure in TearDown works differently from Assertion failure. The assertion is counted and a Warning level AssertionResult is created, but it has no effect on the overall result of the test. If there are no other warnings or failures, the test passes.

Alternatives:

1. Document that Warnings in TearDown have no effect on the result of the test although they may be reported by some runners. This seems confusing to me. If we are going to report it, then the test should have an overall warning result.

2. Handle Warnings in TearDown by changing the result to Warning if the test passed. This sounds like it could be useful but is still inconsistent with failing Assertions.

3. Generate an error for failed warnings in TearDown just as we do for assertion failures.

A Warning in TearDown generally gives seems to be ignored entirely

[CharliePoole]

Hey @nunit/framework-team what do you think?

All three fixes are pretty easy but they go in totally different directions as far as the user goes.

[jnm2]

I'd like a warning in teardown to act just like a warning in the test method. That shouldn't cause cleanup in any other teardown methods to be skipped though, should it? I think we should treat it like throwing from a finally block– that doesn't cause outer finally blocks to be skipped.

[CharliePoole]

@jnm2 There's a logic to that... It basically says that warnings are not the same as failures or errors, therefore it's OK to give them in a teardown. You might even want to warn because you were unsuccessful in cleaning up.

The inconsistency of the title is really just internal. That is, users would probably see a warning as completely different from a failure or error.

I can go that way if others agree.

[CharliePoole]

In fact... here's an idea I like... if we allow warnings in teardown, we could issue a warning ourselves whenever they use Assert or Assume in TearDown! That way we are suggesting "Stop doing this" without breaking compatibility.

[jnm2]

@CharliePoole I like that idea too. However if one teardown method accidentally throws an exception, or even if you throw new Exception(); in one teardown method, I would not want that to prevent any subsequent teardowns from running.

[CharliePoole]

Current (and long-time) behavior is that subsequent teardowns do not get run if you throw an exception in teardown. However, this only applies to a particular teardown level, for example all the teardowns for a single test. For example, if class B derives from class A and they both have teardowns, then after each test we run TearDown B, then TearDown A. If B throws an exception, then A doesn't run.
However, if either A or B has a OneTimeTearDown, that will run after all the test cases finish.

I don't see how we could change this behavior without seriously breaking compatibility and it's hard to imagine how the teardown in the base class would be able to function if the nested teardown had an error.

[jnm2]

@CharliePoole it's hard for me to imagine how the base teardown could fail to function if a nested teardown failed:

[base setup] Create database
[derived setup] Extract zip to temp folder
[test] ...
[derived teardown] Delete temp folder *fails because a file is locked
[base setup] Drop database

On the subject of breaking changes, instinctively I know it is and yet I can't imagine people complaining that the base teardown ran after they hit an unplanned failure in the derived class's teardown.

[jnm2]

I want to be able to rely on teardown guaranteeing that I don't leave things in a bad state, especially for integration tests but even for process state.

[CharliePoole]

Speaking as the devil's advocate, we would have told you historically "Don't do that!"

Some

[CharliePoole]

Continuing... if you know how it works, then you won't write code that way. Instead, you would use a try / catch block to prevent the exception from escaping.

I can see that this would be a useful feature in your use case because each setup performs an unrelated action. Buy nunit can't tell how they interact by itself.

In any case it would be a big new feature.

[jnm2]

Also as devil's advocate, let's say the base class's cleanup (somehow) depends on the derived class's cleanup. What's wrong with trying to clean up and causing a second failure after the first?

Anyway, I hear you. Given NUnit's history, perhaps it would make sense to add this along with an IDisposable fixture disposal feature and treat it like it's in a using, with guarantees of running.

[CharliePoole]

We already have disposal of IDisposable fixtures.

[jnm2]

Ah, now there I would expect the nested try...finally (i.e. using) semantics where inner failures don't stop outer finally blocks from executing.

[CharliePoole]

Not sure what you mean by nested in this context. The user fixture implements IDisposable so there's nothing for NUnit to nest.

[jnm2]

Oh, you're right. So the downside is that you wouldn't be able to apply it to a bunch of fixtures the way we do with before/after test actions. But inheriting from a disposable base class is probably the way to go then.

[mikkelbu]

I prefer solution (2)
Ps. I've only skimmed the following discussion.

[CharliePoole]

Sounds like 3 of us prefer option 2 then.