Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

andreaspsson
Copy link
Contributor

Fixes #1300

lib/server.js Outdated
@@ -133,6 +133,18 @@ class Server extends KarmaEventEmitter {
return this._fileList ? this._fileList.refresh() : Promise.resolve()
}

calculateExitCode (results, config) {
Copy link
Contributor Author

@andreaspsson andreaspsson Aug 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@andreaspsson andreaspsson force-pushed the fix/configurable-exit-code branch from dc684c6 to d2a62b4 Compare August 23, 2018 11:02
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is chanching?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 andreaspsson force-pushed the fix/configurable-exit-code branch from d2a62b4 to 801d5b2 Compare August 24, 2018 11:25
@andreaspsson
Copy link
Contributor Author

@johnjbarton Updated patch after you comments

Copy link
Contributor

@johnjbarton johnjbarton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

lib/browser_collection.js Outdated Show resolved Hide resolved
lib/browser_collection.js Show resolved Hide resolved
@andreaspsson andreaspsson force-pushed the fix/configurable-exit-code branch from 801d5b2 to 2da8c25 Compare August 27, 2018 07:44
@johnjbarton johnjbarton merged commit 74da748 into karma-runner:master Aug 30, 2018
@sambernet
Copy link

sambernet commented Dec 4, 2018

@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
Copy link
Contributor

A PR for docs always welcome.

ilgonmic added a commit to JetBrains/kotlin that referenced this pull request Sep 24, 2019
ilgonmic added a commit to JetBrains/kotlin that referenced this pull request Sep 24, 2019
ilgonmic added a commit to JetBrains/kotlin that referenced this pull request Sep 24, 2019
ilgonmic added a commit to JetBrains/kotlin that referenced this pull request Oct 2, 2019
ilgonmic added a commit to JetBrains/kotlin that referenced this pull request Oct 3, 2019
ilgonmic added a commit to JetBrains/kotlin that referenced this pull request Oct 17, 2019
ilgonmic added a commit to JetBrains/kotlin that referenced this pull request Oct 21, 2019
ilgonmic added a commit to JetBrains/kotlin that referenced this pull request Oct 23, 2019
(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)
ilgonmic added a commit to JetBrains/kotlin that referenced this pull request Oct 23, 2019
(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)
ilgonmic added a commit to JetBrains/kotlin that referenced this pull request Oct 24, 2019
(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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants