Added check for enoent error #10

Merged
merged 1 commit into from Apr 27, 2014

3 participants

@spalger

This package is used by browser-launcher to launch a browser headlessly. When run on OSX, which doesn't have Xvfb installed by default, the call to spawn triggers an error event which is unhanded and displays this useless output:

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: spawn ENOENT
    at errnoException (child_process.js:980:11)
    at Process.ChildProcess._handle.onexit (child_process.js:771:34)

Added an listener for "error" that will pass back a useful error message when it can.

@spalger spalger added check for enoent error, as well as a general 'error' event hand…
…ler on the child process, preventing 'uncaught error event' on systems without Xvfb installed
051bdd6
@graue

Same here, on Ubuntu — I didn't have Xvfb installed, and was baffled at the spawn ENOENT error that Testling (which uses this package) threw up. A friendly error message like this would have saved me some time 👍

@kesla kesla merged commit d3c0c85 into kesla:master Apr 27, 2014

1 check passed

Details default The Travis CI build passed
@kesla
Owner

So, I haven't actually tested this PR (I don't use this module frequently anymore) - could someone take a look and ping me if it works as expected?

@graue

Aw shoot. It doesn't. At least, Testling depending on browser-launcher depending on this doesn't work as expected. Now I feel bad for (implicitly) advocating for this to be merged, without trying it. Sorry about that.

What I did: uninstalled Xvfb from my system, and replaced Testling's node_modules/browser-launcher/node_modules/headless with latest master (both with and without this change). Without this change I get the ENOENT error. With this change it simply hangs, not reporting an error or anything. I'm currently trying to figure out why.

@graue

Ah, never mind. node-headless is working perfectly. It's browser-launcher's fault for not checking for and re-throwing the error, and @spenceralger has already submitted a separate PR there: substack/browser-launcher#22. Thank you @kesla for merging this!

@kesla
Owner

Release as 0.1.7

@kesla
Owner

And @graue thanks for helping out!

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