lazd / gulp-karma

Karma plugin for gulp
MIT License
75 stars 36 forks source link

Unhandled exception when test fails #23

Closed ninjatronic closed 9 years ago

ninjatronic commented 10 years ago

When I have a test that fails I get the following exception:

/Users/pete/work/projects/console/node_modules/gulp-karma/index.js:56
        stream.emit('error', new gutil.PluginError('gulp-karma', 'karma exited
                             ^
TypeError: undefined is not a function
  at done (/Users/pete/work/projects/console/node_modules/gulp-karma/index.js:56:30)
  at ChildProcess.<anonymous> (/Users/pete/kinvey/projects/console/node_modules/gulp-karma/index.js:82:7)
  at ChildProcess.EventEmitter.emit (events.js:98:17)
  at Process.ChildProcess._handle.onexit (child_process.js:797:12)

Which breaks the pipe.

lazd commented 10 years ago

Please upgrade to 0.0.4 and report back.

ninjatronic commented 10 years ago

Error is now:

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: karma exited with code 1
  at done (/Users/pete/work/projects/console/node_modules/gulp-karma/index.js:56:30)
  at ChildProcess.<anonymous> (/Users/pete/work/projects/console/node_modules/gulp-karma/index.js:82:7)
  at ChildProcess.EventEmitter.emit (events.js:98:17)
  at Process.ChildProcess._handle.onexit (child_process.js:797:12)
jimthedev commented 10 years ago

I am having this exact same problem.

jimthedev commented 10 years ago

So my thought on this issue is that sometimes I am using gulp-karma and want it to throw the error. This would be in the case of running a command like 'gulp tests'. Other times I want it to suppress the error and still return the failure notification, but wrapped in a json object. This would be useful in the case of watchers. Would this be a worthwhile config option?

jimthedev commented 10 years ago

I have a PR for this that allows you to tell gulp-karma not to die on error. This should help if you're using it in a watcher that you don't want to die on error.

jimthedev commented 10 years ago

@ninjatronic @lazd Ok that PR should do it.

My only concern with this is that in the readme, we say options will take any karma config option. Since this option isn't a karma config option, and is specific to the plugin we're kind of mixing karma options and plugin options. Perhaps it isn't a big deal, but might be worth making the distinction in the future between plugin options and karma options before they get hardcoded into other projects.

ninjatronic commented 10 years ago

@jcummins @lazd the options map allows configFile and action, which are not options you would find in a karma.conf file. Perhaps the distinction could be made like...

karma
  configFile: 'karma.conf.coffee'
  action: 'run'
  dieOnError: false
  config:
    #karma configuration here
    browsers: [
      'PhantomJS'
      'Chrome'
      'Safari'
      'Firefox'
    ]

It's a little clearer there what is, and isn't, a karma option as opposed to a plugin option

jimthedev commented 10 years ago

@ninjatronic Agreed. I very much prefer that type of syntax as it doesn't blur the lines as much as the current config. Since it will require a rewrite of the rest of the variables in the plugin, it probably isn't in the scope this issue. If @lazd is on board then we can open a separate issue to refactor w/that syntax. It would be a breaking change.

ninjatronic commented 10 years ago

@jcummins That's what semver is for :wink:

Seriously: yes, it does make sense

jimthedev commented 10 years ago

@ninjatronic :+1:

Not sure I understand the second part. Do you mean it does make sense for this issue?

ninjatronic commented 10 years ago

No I mean it makes sense to split it out into a separate issue

jimthedev commented 10 years ago

Ok cool!

nickradford commented 10 years ago

Running into this as well - it seems if you're using a reporter such as the junit reporter, this error is thrown before any reports are written to disk.

lazd commented 10 years ago

@ninjatronic and @nickradford: Have you tried adding an error listener on the stream? That should stop it from dying.

See https://github.com/gulpjs/gulp/blob/master/docs/recipes/combining-streams-to-handle-errors.md

lazd commented 9 years ago

gulp-karma is deprecated (#43). Please use Karma directly: https://github.com/karma-runner/gulp-karma