No reporter prints `uncaughtException` details in case of browser tests

[dominykas]

Briefly mentioned this on https://github.com/busterjs/buster-test/pull/27. Seems that in nodejs cases, the param for uncaughtException handler contains the instance of the Error object itself, but if it comes back from the browser - the details are in the serialized object in the data property of the e param.

I don't think updating all the reporters to check for e.data is the correct approach... What's the route the exception takes once it's emitted by the json-proxy?

[cjohansen]

Wouldn't the correct approach be to fix the object emitted by json-proxy?

[dwittner]

I don't see, what the issue has to do with json-proxy. Isn't that just another reporter? I think we have to ensure, that uncaught exceptions are not handled by remote-runner#emitCustom. If i add a handler for suite:error like this:

    "suite:error": function (msg) {
        this.emit("suite:error", msg.data);
    }

the Error "no contexts" produces a useful error message:

Uncaught exception in Unknown context:

  Error: No contexts

I am still not happy about the "Unknown context", but this can for sure also be fixed.

[dwittner]

Seems we also need a handler for "uncaughtException":

    "uncaughtException": function (msg) {
        this.emit("uncaughtException", msg.data);
    }

Uncaught exception in Unknown context:

  UncaughtError: ./test.js:7 Uncaught TypeError: undefined is not a function

[dominykas]

Where are you adding these handlers?

I might have missed something... but my understanding is that normally, when the error happens in node, the uncaughtException/suite:error handlers get the exception itself as a parameter, whereas when the error goes via json-proxy - the parameter for the final event is an object which has the exception inside the data property. Which means that to be able to report it properly, one needs to handle both cases - the error as a param, and the error as data (which you just did in your samples).

[dwittner]

I added the handlers in buster-test-cli/lib/runners/browser/remote-runner.js. The json-proxy isn't used in case of browser tests. I think you can use it via --reporter json-proxy, but i don't know for what :) Because there are currently no handlers for uncaughtException and suite:error in remote-runner.js, the events are handled by the emitCustom function, which does the following:

this.emit(event, {
    event: event,
    client: this.getClient(msg.slaveId),
    data: msg.data,
    runtime: msg.data.runtime
});

That's why the exception is inside a data property in the uncaughtException and suite:error handlers of the reporters in case of browser tests.

[dominykas]

Huh? Isn't json-proxy used by the browser runner to send data back to the server?

Ofc the fix in remote runner might be even better - I really don't know buster architecture well enough :) (hint hint, some diagrams would be useful)

[dwittner]

No, the data is sent back by ramp, which in turn uses faye.

[dominykas]

Hmmm. @cjohansen's comment: https://github.com/busterjs/buster/issues/327#issuecomment-56949220

[dwittner]

@dominykas, you are right. After a long search i found the point in the code, where json-proxy is configured to be used for browser tests, capture-server-wiring.js:

    var reporter = B.reporters.jsonProxy.create(emitter);
    reporter.listen(runner);

[dominykas]

OK, so we're still left with the decision of where to fix this - json-proxy or remote-runner...

[cjohansen]

I think the fix should be applied in the JSON proxy. It doesn't change any of the other event objects does it?

[cjohansen]

After some more explanation from @dwittner, it seems his suggested solution in remote runner is the correct fix.

[dwittner]

@dominykas, do you want to fix it?

[dominykas]

I'm not sure when I'll have time to get to it. I've never looked at remote runner code either... And when I do find time - I'll probably start with fixes for other issues I've got in progress - so not sure which one of us will get around to this one sooner :) but if it's still there - sure.

[dwittner]

Fixed in version 0.8.7 of buster-test-cli.

[cjohansen]

<3