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

Pass exit code through reporters to karma #82

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ryanvall
Copy link

@ryanvall ryanvall commented Nov 2, 2022

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features) - project doesn't appear to have any, tests in tests dir aren't testing lib behavior
  • Docs have been added / updated (for bug fixes / features) - doesn't seem necessary here unless something needs to be in the releases log (doesn't appear to have been updated in a while)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Bug fix

  • What is the current behavior? (You can also link to an open issue here)

Possibly related to #35, but karma-parallel doesn't appropriately exit with code 1 if coverage thresholds aren't met when this is used in conjunction with karma-coverage. In this issue/subsequent PR support was added in karma/karma-coverage to pass the exit code through in the onExit callback:

karma-runner/karma-coverage#418
karma-runner/karma#3541
karma-runner/karma-coverage@c93f061#diff-691854e392b02b6faf6147eef5d32e3bb9f187025dc025742167d4bdce7443feR286

The existing implementation of onExit in karma-parallel doesn't pass along the returned values from onExit in the individual reporters.

  • What is the new behavior (if this is a feature change)?

The exit code is now passed through from the original reporters.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

Possibly? Users might now have commands/CI builds fail because of coverage, but that would be correct/no longer a false positive. I don't know enough about karma/the reporter API to know if onExit is only ever supposed to be passed an exit code, so if it can be passed other data/arguments this behavior wouldn't be entirely correct.

  • Other information:

I'm not 100% sure if this is the right solution - this wouldn't be an issue for anyone not utilizing a coverage reporter/coverage thresholds. Open to feedback on how to improve this.

@dongyuwei
Copy link

I can confirm latest karma-parallel still has the bug, and this pull-request do fixed the bug.
Please merge and publish a new version, thanks.

Additional info:

"karma": "^6.4.1",

"karma-chrome-launcher": "~3.1.1",

"karma-coverage": "^2.2.0",

"karma-jasmine": "^5.1.0",

"karma-jasmine-html-reporter": "^1.7.0",

"karma-parallel": "^0.3.1",
"karma-viewport": "^1.0.6",

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.

None yet

2 participants