Nunit-console: Error in SetUpFixture does not result in non-zero exit code

[fakefeik]

I have the following SetUpFixture:

[SetUpFixture]
public class ConverterCoverageLoggerWorker
{
    [OneTimeSetUp]
    public static void BeforeTests()
    {
        // ...
    }

    [OneTimeTearDown]
    public static void AfterTests()
    {
        throw new Exception("Should be reported");
    }
}

I run tests using NUnit 3.10.1, nunit-console 3.8.0 in TeamCity using this NUnit build step
When I run tests with this SetUp in TeamCity, it prints out the following log:

[11:55:59] :     [Step 2/2] Errors, Failures and Warnings
[11:55:59] :     [Step 2/2] 
[11:55:59] :     [Step 2/2] 1) TearDown Error : Tests.ConverterCoverageLoggerWorker
[11:55:59] :     [Step 2/2] TearDown : System.Exception : Should be reported
[11:55:59] :     [Step 2/2] --TearDown
[11:55:59] :     [Step 2/2]    at Tests.ConverterCoverageLoggerWorker.AfterTests() in C:\buildAgent\work\c5011b50b2ec7e96\EDI\Tests\ConverterCoverageLoggerWorker.cs:line 39
[11:55:59] :     [Step 2/2] 
[11:55:59] :     [Step 2/2] Run Settings
[11:55:59] :     [Step 2/2]     RuntimeFramework: net-4.0
[11:55:59] :     [Step 2/2]     DisposeRunners: True
[11:55:59] :     [Step 2/2]     WorkDirectory: C:\buildAgent\work\c5011b50b2ec7e96\EDI\Tests\bin
[11:55:59] :     [Step 2/2]     BasePath: C:\buildAgent\work\c5011b50b2ec7e96\EDI\Tests\bin
[11:55:59] :     [Step 2/2]     ImageRuntimeVersion: 4.0.30319
[11:55:59] :     [Step 2/2]     ImageTargetFrameworkName: .NETFramework,Version=v4.7.2
[11:55:59] :     [Step 2/2]     ImageRequiresX86: False
[11:55:59] :     [Step 2/2]     ImageRequiresDefaultAppDomainAssemblyResolver: False
[11:55:59] :     [Step 2/2]     NumberOfTestWorkers: 4
[11:55:59] :     [Step 2/2] 
[11:55:59] :     [Step 2/2] Test Run Summary
[11:55:59] :     [Step 2/2]   Overall result: Failed
[11:55:59] :     [Step 2/2]   Test Count: 2810, Passed: 2804, Failed: 0, Warnings: 0, Inconclusive: 0, Skipped: 6
[11:55:59] :     [Step 2/2]     Skipped Tests - Ignored: 0, Explicit: 6, Other: 0
[11:55:59] :     [Step 2/2]   Start time: 2018-10-12 06:23:50Z
[11:55:59] :     [Step 2/2]     End time: 2018-10-12 06:55:58Z
[11:55:59] :     [Step 2/2]     Duration: 1928.670 seconds
[11:55:59] :     [Step 2/2] 
[11:55:59] :     [Step 2/2] Results (nunit3) saved as C:\buildAgent\temp\buildTmp\QaEKVqMV4RKaa7ZyErML2KcKw5NaoLUb.nunit.xml
[11:55:59] :     [Step 2/2] Process exited with code 0

But TeamCity shows that build is successful and all tests have passed. My guess is that TeamCity looks at nunit3-console zero exit code and thinks that everything is fine. Is this an intended behaviour or will it be fixed?

[mikkelbu]

Seems like the same issue as in #282 and #464 (both about exceptions in OneTimeSetups/OneTimeTearDowns), so I say it should be fixed. I'll take a look at this, but first in the end of next week as I will be quite busy next week.

@nunit/engine-team We have not yet decided on what we want the return code should be. Personally, I think UNEXPECTED_ERROR would be fine, but the again I don't look at the actual return codes (besides whether it is negative).

Context

In #464 Charlie has proposed

I think we should return UNEXPECTED_ERROR for this situation, or invent a new negative code covering any kind of fixture closure or teardown error.

and in #282 he has listed the possibilities

We should decide what the return code should be in these cases. One approach would be to return the number of errors and failures displayed, even if they are not test case errors. Another would be to create a new negative return code value indicating that onetime setup or teardown errors occured even though all test cases passed. A third simple approach would be to return the existing "unexpected error" code.

[ChrisMaddock]

@mikkelbu I think we should have a distinct, negative return code for these cases. UNEXPECTED_ERROR represents places where the console itself has crashed, that's not what we mean here.

Hopefully we won't run out of numbers... 😄

[CharliePoole]

A simple solution might be to count the error, returning 1 in this example. This would be a subtle redefinition of a positive return value from "number of failed test cases" to "number of displayed failures" but I doubt that would be confusing to the user who is simply looking at the console result and it would solve a long-standing problem. I __think__ we may have done something similar with invalid fixtures in the past, but I'm not sure that the code remains.