fix(runner): Make exit code configurable when tests are failing

[andreaspsson]

Fixes #1300

[andreaspsson]

I implemented like this because I wanted to let failOnEmptyTestSuite work as it did before. But I think it is kind of strange that the failOnEmptyTestSuite flag overrides any error or disconnect and makes it always return success if it is false and there are no tests, an alternative implementation would be:

if (!results.error && !results.disconnected) {
  if (results.success + results.failed === 0 && !config.failOnEmptyTestSuite) {
    this.log.warn('Test suite was empty.')
    return 0
  }
  if (!config.failOnFailingTestSuite) {
    return 0
  }
}

But that would slightly change the previous behaviour of failOnEmptyTestSuite.

[johnjbarton]

It's hard to figure out all of the cases here, because the exitcode computation is shared between lib/server and lib/browser_collection. I would rather we do it in one place.

Doing the calculation here means we end up passing a different value of results.exitCode to reporters (via run_complete event). So computing in browser_collection would be better.

Part of the calculation depends on config, not currently available in browser_collection.

How about doing all of the work in browser_collection with getResult(config, singleRunBrowserNotCaptured) or even getResult(failOnEmptyTestSuite, failOnFailingTestSuite, singleRunBrowserNotCaptured) ?

That is, your calculateExitCode moves to that file and here we do the same thing as executor,
call getResults() and emit run_complete.

And I agree that failOnEmptyTestSuite should have only one effect: a test that would otherwise exit 0 should exit 1 if it ran no tests.

I hope this is clear.

[andreaspsson]

I saw now that my reasoning about failOnEmptyTestSuite was not 100% correct. If no tests are executed results.error is true when the code reaches this point, so it suppresses errors, but it would also have suppressed exit codes generated by results.disconnected being true in the case of no tests was executed

[johnjbarton]

what is chanching?

[andreaspsson]

I want to keep karma exit with failure unless the new argument is set, as it did before. In new version of the patch I updated the logic using the flag instead, so no default value is needed in cli.js to achieve this

[johnjbarton]

It's hard to figure out all of the cases here, because the exitcode computation is shared between lib/server and lib/browser_collection. I would rather we do it in one place.

Doing the calculation here means we end up passing a different value of results.exitCode to reporters (via run_complete event). So computing in browser_collection would be better.

Part of the calculation depends on config, not currently available in browser_collection.

How about doing all of the work in browser_collection with getResult(config, singleRunBrowserNotCaptured) or even getResult(failOnEmptyTestSuite, failOnFailingTestSuite, singleRunBrowserNotCaptured) ?

That is, your calculateExitCode moves to that file and here we do the same thing as executor,
call getResults() and emit run_complete.

And I agree that failOnEmptyTestSuite should have only one effect: a test that would otherwise exit 0 should exit 1 if it ran no tests.

I hope this is clear.

[andreaspsson]

@johnjbarton Updated patch after you comments

[johnjbarton]

I think this form is much better, thanks for taking the extra step.

[sambernet]

@johnjbarton and @andreaspsson:

After struggling with the problems caused by #1300 I was very pleased to see this has actually been resolved 🍾 🎆

Sending lots of good karma to you guys 😁

Anyway it took me more research to arrive here than necessary, because the new flag does not seem to be documented (yet) in the 3.0 docs.

Is there a good reason for that or is it just missing? I'm aware of the 3.0 in the path and the fact that the new flag was released in 3.1, but I didn't find any newer docs either...

edit: after digging a bit into this PR I noticed that the CLI exposes the option if it is asked kindly enough:

> npx karma start --help
Karma - Spectacular Test Runner for JavaScript.
[...]
Options:
  [...]
  --fail-on-failing-test-suite     Fail on failing test suite.
  --no-fail-on-failing-test-suite  Do not fail on failing test suite.

[johnjbarton]

A PR for docs always welcome.