Nunit-console: Handle early termination of multiple assert block

[CharliePoole]

@CharliePoole commented on Sun Nov 20 2016

According to the spec, we want the test will be terminated immediately in case of Assert.Fail, Assert.Pass, Assert.Inconclusive, Assert.Ignore, Assumeption failures and unexpected exceptions. We are doing that now but we don't have tests and we don't display the special messages called for in the spec:

• Assert.Fail encountered with pending failures: $usermessage$
• Assert.Pass encountered with pending failures: $usermessage$
• Assert.Inconclusive encountered with pending failures: $usermessage$
• Assert.Ignore encountered with pending failures: $usermessage$
• Assumption failed with pending test failures: $usermessage$
• TypeOfException encountered with pending failures: $exceptionmessage$

Question for @nunit/contributors : Should we show the pending failures in any of these cases?

[rprouse]

For me, the priority would be for Assert.Pass because of the contradiction of having failures but then passing. Should we actually be switching the pass to a fail? Otherwise no-one will ever see the failure messages because the test will be green.

I actually wonder if we should not allow any of these asserts/assumptions (except maybe fail) within a multi-assert block. They seem to contradict the intention of the feature. Might we treat using them as an error?

[CharliePoole]

@rprouse Well, it's a change from the spec we all agreed to, but I think you may have a point here.

I think the key is that we may not actually want to treat all of these things the same... For example, Assert.Fail can be used to simulate any other assert as in
C# if ( actual != expected) Assert.Fail("Expected the value to be {0} but it was {1}", expected, actual);
It makes sense that this should work the same as an Assert.That using an equality constraint. That would mean to me that we would record the assertion failure and __not__ terminate the block.

If we want to disallow the others within a multiple assert block, we will have to decide how to handle their appearance. Most likely, we would throw an exception of some kind, making this into an error. Alternatively, we could only disallow them if there were prior failing asserts.

Another option, which intrigues me is to not terminate the block at all in these cases and to store the assertion result with expanded result values, like "Passed" "Ignored" etc. the final test result would be figured out by using the worst of the stored assertions.

I think this sends me back to the spec to try to better articulate what should happen. We can review that before proceeding. Since some of the options above involve framework changes, I'll probably then switch to the nunitlite issue for the first implementation and then port the result to this issue.

[rprouse]

Allowing Assert.Pass in the block bothers me because we may already have a test that is failed and we have just not notified (thrown) yet. The idea of taking a failed test and making it a passed test seems wrong to me. Like you suggested, I think the test should still fail and you should record the pass. Then, if there have been no failures, immediately passing the test seems correct. This dual behaviour is what concerns me.

I also feel that assumptions belong as guard statements at the front of your tests, not in the middle of a multiple assert block. I don't use Ignore and Inconclusive much, so I don't have a common pattern for their use to base an opinion on.

I would be interested in what the rest of the @nunit/contributors team thinks, especially @jcansdale, @circa1741 and @remcomulder since they have been very involved in this feature so far.

[CharliePoole]

I guess I can't either agree or disagree with you until I know what "not allowing" Assert.Pass would mean. We can't literally disallow it... that is people can type it in and it will compile... so we have to define some behavior that occurs when we find it in a block and/or in a block that has already had failures.

One thing I feel strongly is that consistent behavior we can explain simply to users is very important here.

I too want to hear what the others think. Although we all thought the spec was correct as written, it's clear we didn't give it enough thought. I'm hoping that the exercise of revising it will force me to think more clearly about how each different case plays out. I'll post here when it's ready.

[rprouse]

By disallow, I would say we thrown an exception to make it an error with a clear message that X is not supported within a multi-assert block.

I too will differ to other's opinions. My use of a multi-assert block would be for simple things like checking several properties on a class. I wouldn't think of using Assert.Pass or assumptions.

[CharliePoole]

I'm of the same mind. If I were writing advice on how to best use it, I'd say don't use Assert.Pass, Assert.Ignore, Assert.Inconclusive or assumptions. But we need to do something if they are used.

Throwing an exception is one way. Then we can handle all those cases as part of the Error case. I may outline two options, so we have something to choose from.

[CharliePoole]

I updated the spec indicating the way I'd like to go with this. Looking for comments from all @nunit/contributors especially those involved in reviewing the original spec.

https://github.com/nunit/docs/wiki/Multiple-Asserts-Spec#assertmultiple-syntax

[remcomulder]

Personally, this feels like an edge case to me. I'm not sure what the user would expect when terminating the assertion block early... It doesn't really feel like there is a right answer here. Personally, I don't use Assert.Pass or Assert.Inconclusive (NCrunch actually doesn't support inconclusive tests, it just passes/fails the test based on a config setting).

[rprouse]

I've read the updated spec and I agree except for option 2 for Assert.Pass / Assert.Ignore. The reason I don't like option 2 is that the error only appears once your tests start failing. In other words, you could have a perfectly valid test that then turns into a test that is in error. In my opinion, a test error indicates an improperly written test and therefore should always be an error. I realize that is the not the case for tests that sometimes throw unexpected exceptions, but we have control here.

Maybe I am being too strict on my interpretation of errors though, so throwing if used and the test has already failed would be okay. Just voicing my thoughts 😄

[CharliePoole]

@rprouse I can see your point and it's equally easy to implement either approach.

What do others think? I'm inclined to change to use option 1 based on @rprouse 's comment.

[CharliePoole]

Updated the spec to match @rprouse 's suggestion. Removing the design tag since implementation has started.

[CharliePoole]

I can't seem to find any more work to be done on this, all of it having been completed under other issues. I'll close this, but if somebody notices anything that's not yet done, we can reopen it.