fix(runner): Make exit code configurable when tests are failing #3116
Conversation
lib/server.js
Outdated
| @@ -133,6 +133,18 @@ class Server extends KarmaEventEmitter { | |||
| return this._fileList ? this._fileList.refresh() : Promise.resolve() | |||
| } | |||
|
|
|||
| calculateExitCode (results, config) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
dc684c6
to
d2a62b4
Compare
lib/cli.js
Outdated
| if (helper.isString(options.failOnFailingTestSuite)) { | ||
| options.failOnFailingTestSuite = options.failOnFailingTestSuite === 'true' | ||
| } else if (typeof options.failOnFailingTestSuite === 'undefined') { | ||
| // Default value to true, to avoid chanching application default behaviour |
There was a problem hiding this comment.
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
lib/server.js
Outdated
| @@ -133,6 +133,18 @@ class Server extends KarmaEventEmitter { | |||
| return this._fileList ? this._fileList.refresh() : Promise.resolve() | |||
| } | |||
|
|
|||
| calculateExitCode (results, config) { | |||
There was a problem hiding this comment.
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.
d2a62b4
to
801d5b2
Compare
|
@johnjbarton Updated patch after you comments |
801d5b2
to
2da8c25
Compare
|
@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 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. |
|
A PR for docs always welcome. |
(cherry picked from commit cb31b42) [Gradle, JS] Update npm dependencies - Karma - provide possibility to check exit code - Webpack - bug fixes - Mocha - bug fixes (cherry picked from commit fa64768) [Gradle, JS] Fully copy of karma-teamcity-reporter (remove npm dependency on it) [Gradle, JS] Copy mocha-teamcity-reporter with licensies [Gradle, JS] Depends node test on our own mocha reporter [Gradle, JS] Refactor and extract team city formatting in separate file Check Karma exit code with disabled failOnFailingTestSuite karma-runner/karma#3116 (cherry picked from commit 2cf79b7)
(cherry picked from commit cb31b42) [Gradle, JS] Update npm dependencies - Karma - provide possibility to check exit code - Webpack - bug fixes - Mocha - bug fixes (cherry picked from commit fa64768) [Gradle, JS] Fully copy of karma-teamcity-reporter (remove npm dependency on it) [Gradle, JS] Copy mocha-teamcity-reporter with licensies [Gradle, JS] Depends node test on our own mocha reporter [Gradle, JS] Refactor and extract team city formatting in separate file Check Karma exit code with disabled failOnFailingTestSuite karma-runner/karma#3116 (cherry picked from commit 2cf79b7)
(cherry picked from commit cb31b42) [Gradle, JS] Update npm dependencies - Karma - provide possibility to check exit code - Webpack - bug fixes - Mocha - bug fixes (cherry picked from commit fa64768) [Gradle, JS] Fully copy of karma-teamcity-reporter (remove npm dependency on it) [Gradle, JS] Copy mocha-teamcity-reporter with licensies [Gradle, JS] Depends node test on our own mocha reporter [Gradle, JS] Refactor and extract team city formatting in separate file Check Karma exit code with disabled failOnFailingTestSuite karma-runner/karma#3116 (cherry picked from commit 2cf79b7)
Fixes #1300