Move common classes for console output to nunit.common

[CharliePoole]

Fixes #1668

[manfred-brands]

A few small items/questions.
Note that there is also a copy of WriteHeader in nunit4-netcore-console/Program.cs which could be replaced as well.

[manfred-brands]

Prefer NUnit.Common

[CharliePoole]

Let me know what you think about my rationale above.

[manfred-brands]

This causes code to increment ErrorCount instead of PassCount.
You changed the MockAssembly.Passed expected count by adding: PassedButFailedInTearDown
Should a test be marked Error if the teardown fails?

[CharliePoole]

I asked @mikkelbu for some background on that change, made seven years ago, as this was the part of the change that gave me the most trouble

ResultReporter was already using PassedInAttribute rather than Passed. I changed how the number is calculated so that Passed could be used but the values are still the same, and all tests are still passing.

@mikkelbu Can you take a look at the last commit, where I made that change?

[manfred-brands]

Your update to ResultReportedTest now expects: Failed_Errors to be 1, previously it was 7. The 6 difference is the PassedButFailedInTearDown you added to Passed

The PassedInAttribute was used in other tests: NUnitFrameworkDriverTests and TestAgentRunnerTests.
If that is what comes back from the engine/framework then indeed the added code in ResultSummary is wrong.
If we want tests to be considered failed if the teardown fails, then this counting might need updating in the framework.

If we keep the counting we have now, you can remove the bool failedInFixtureTearDown parameter.

[mikkelbu]

I asked @mikkelbu for some background on that change, made seven years ago, as this was the part of the change that gave me the most trouble

I'm sorry, but I cannot remember anything about this. I think I only had made a handful of PRs in the console before doing this change, so I think most of the logic was somewhat new for me.

@mikkelbu Can you take a look at the last commit, where I made that change?

I can check it tomorrow night - so in 20 hours or so - and can also take a closer look at my old PR and see if I remember some more.

[manfred-brands]

Nothing more, except for some clean-up as a result of commented out code in the ResultSummary

[CharliePoole]

@manfred-brands There's still something wrong here because this was an unintended change on my part.

I just noticed something interesting... for mock-assembly, the unit tests expect 17 passed and 11 failed. The final package tests expect 23 passed and 5 failed. Both sets of tests are passing!

What I think this means is that the change made in counting was only supposed to affect the summary report and results themselves. reported counts in test-result.xml. Do you recall any discussion about that? @mikkelbu ?

In any case, I don't want to remove a feature unintentionally. So I'll do another iteration.

[mikkelbu]

What I think this means is that the change made in counting was only supposed to affect the summary report and results themselves. reported counts in test-result.xml. Do you recall any discussion about that? @mikkelbu ?

If I understand the question correctly (and compare it against the last sentence in #494 (comment) - copied in below) then it could be so, but I need to check the old PR to be sure.

The only thing that could be confusing is that the XML already contains information about passed and failed tests which will not match what the console will report, but this will be an issue for both approaches. See e.g. the difference in XML for 5 tests and a failure in Setup/TearDownFixture, respectively.

[CharliePoole]

@manfred-brands @mikkelbu

OK... I've restored the original behavior. That is, when running MockAssembly under the console runner, TestResult.xml has 23 passed and 7 failed but the summary report shows only 17 passed and 11 failed due to 6 failures in higher-level teardowns. If we want to change this again at some point, that's OK but I didn't want to change the behavior as an unintended consequence of this issue.

I have changed the naming of many of the counts in MockAssembly to names that make more sense to me. Feel free to call for further changes. I introduced Passed_Raw, Errors_Raw and Failed_Raw, which represent the results of test cases before any higher-level teardown is run. Simple Passed, Errors and Failed take the teardown failures into account.

Tests of the content of TestResult.xml use the XxxxRaw values. Tests of thhe console summary display use the simple values. This was not consistent earlier and it led to lots of confusion (for me)!

Please consider the three _Raw names as an invitation to come up with something better. For example, I thought about using PassedTestCaseButLaterFailed, etc. :-)

@mikkelbu I think at this point, the best use of any review you make is going to be planning for further changes. BTW I've been wondering how nunitlite displays its summary. Is it consistent with the console runner?

UPDATE: I answered my own question. MockAssembly for the framework doesn't have any tests that fail in a higher level TearDown, so the situation never arises. The code for NUnitLite's ResultSummary doesn't deal with the situation of failing in teardown as the one in NUnit-console does.