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.

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.

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

what is chanching?

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

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.