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

Add bail on load errors #2672

Closed
wants to merge 1 commit into from

Conversation

yagudaev
Copy link

@yagudaev yagudaev commented Apr 20, 2017

Problem description

If Karma receives an error during the load of one of the plugins it currently swallows that error and continues running other plugins and exits with a zero error code.

I discovered this by investigating a successful build that didn't run any test on CircleCI. Turns out CI servers need a non-zero exit code to mark a build as failed.

Webpack 1.0 had a similar issue and they introduced a bail option that is turned on by default in 2.0 with a workaround npm package for 1.0 called webpack-fail-plugin. It would catch errors in plugins like babel that are used to compile the assets. A syntax error in the source code would cause babel plugin to fail and webpack would swallow the exception unless the bail option is on or fail plugin is present. See: webpack/webpack#708

In the case of Karma the failure was in the PhantomJS npm package, where it could not locate the binary. It should have been a show stopper and reported a failure, but proceeded to load and execute the next plugins. See: karma-runner/karma-phantomjs-launcher#120

This is my first attempt at looking at Karma, so please forgive my ignorance. This PR is a suggested solution and will need adjustments with your help.

Outstanding questions:

  1. Should we fail by default?
  2. Should we also introduce a bail option that is on by default? Or perhaps use another option?
  3. Is this the right place to handle errors like this?

TODOs (would love help):

  • Write a test for a plugin load error

Other related: codymikol/karma-webpack#66

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@yagudaev
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@yagudaev
Copy link
Author

Travis build seems to be broken on master, let me know when it is fixed and I will rebase. Would love to get this merged.

@wesleycho
Copy link
Member

@yagudaev this is a legitimate failure - this is a failure based on the commit message not conforming to the commit standards this project follows. See here for details (sorry it is so buried - it is very easy to miss, and took me some drilling into to find this link).

@PhilippMeissner
Copy link

Any update on this?
Just faced this very issue and had our CI sitting idle and block later builds for 90 minutes.

@johnjbarton
Copy link
Contributor

@yagudaev Thanks! I think a test for this issue is critical because we already are supposed to exit on load errors:
https://github.com/karma-runner/karma/blob/master/lib/server.js#L213

Something like karma/test/e2e/launcher-error.feature but load-error.feature would be awesome.

@johnjbarton
Copy link
Contributor

Sorry this did not work out. I'm going to close a lot of older pull requests as the source code has changed a lot over the last year.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants