Add onerror handler for pretest errors

[bouk]

@jejacks0n this is not meant for merging yet but more to discuss different approaches for #479

The approach I took here will work for any driver, but has the drawback of showing very limited information. window.onerror doesn't give a whole lot of info. This does work however, and my test fails as expected when the error happens

I'll also need some help with how/where to define a test for this.

[jejacks0n]

Hey @bouk, thanks for taking this on, and sorry I've been absent from it a bit. I'll merge this when you resolve CI.

The CI failure: ReferenceError: Can't find variable: msg, seems to be related to your change here, https://github.com/modeset/teaspoon/pull/480/files#diff-4630ab299de0b26c012329264476c8a5R69 where msg is not defined in the function arguments. I'd say roll that line back since it's unrelated to your other changes, no?

[bouk]

ah woops that's a copy paste error, fixed

[jejacks0n]

An easy place to start is to unit testing the new setup method.. Check the base teaspoon spec and there'll be some examples. I assume you can call window.onerror directly without having to raise an exception, so that would be my first pass. This may not be possible, and if it's not, I'd extract the logic from within the window.onerror function, move it to a different method and test that individually.. then just make sure window.onerror is the function you expect.

As a follow up, I realized that we could be clobbering an existing window.onerror handler.. you should capture that and call it if it's defined in our own implementation -- pseudo code.

existingHandler = window.onerror
window.onerror = =>
   # our logic
   existingHandler?()

This also breaks some existing stuff, which is tested.. I have a call to foo() in the integration tests which can be found in the spec dummy app assets.. it behaves differently now, and so the output is different.

A few questions now that I thought this through a bit more... when you run jasmine you can disable it's try/catch logic so exceptions are raised -- this makes debugging really nice sometimes. how does this window.onerror handle things now? Should this go only in the console reporters, since that's really the only place we want the behavior?

[bouk]

I'm already handling when there is an existing onerror handler.

I think for this to only do its thing when you're in a console report makes sense, but doesn't that mean just PhantomJS gets this right now? What if someone is using Selenium (which I would assume some people are for CI?)

Also, what do you want me to change about that integration test? Because it's behaving as expected with the new behaviour, do you want the call to foo() removed or the expected output to change?

[bouk]

@jejacks0n I've added a spec for the onerror handler

What do you want me to do about the integration tests? Should I remove the foo() call that makes it fail?

[jejacks0n]

To be clear this is why I took the stance I did. When you have an exception it doesn't necessarily break the world -- things can continue running, it's just important to know that it's an issue that you may not have a test for. Anyway, that's the background. =)

If you could remove it and put it into a different test that covers what your error handling accomplishes that would be awesome. So, I'm fine with accepting the adjustment to the behavior, but the tests accurately reflect the change in behavior so we should 1. remove the case that breaks the integration tests (e.g. the call to an undefined function), and add one that ensures that when something like that does happen it fails the build as it does now.

[jejacks0n]

To give you some more info, if you check in https://github.com/modeset/teaspoon/blob/c9786f54c487a78e55686f76cf93f199654fea53/teaspoon-jasmine/spec/javascripts/integration/first_integration.coffee you'll see an example of that.. so you'll need to adjust the behavior in each of the different frameworks -- e.g. jasmine, mocha, and qunit.

additionally https://github.com/modeset/teaspoon/blob/c9786f54c487a78e55686f76cf93f199654fea53/teaspoon-jasmine/spec/integration_spec.rb you'll find where we basically configure teaspoon on the fly to get those "real" integration tests into the dummy application.

That's how you'd create a new one basically.. so adjust the existing ones to pass, and then create three new ones (one for each framework) that has a "broken_integration" setup similar to what's already in place.

[jejacks0n]

Thanks for taking this on btw.

[bouk]

@jejacks0n I've added a broken spec helper test and fixed the other tests, and it has passed CI!

[jejacks0n]

I need to exclude the js from hound apparently now that coffeescript -> javascript causes so many issues.

[bouk]

@jejacks0n I've updated the hound config to exclude the generated files

[jejacks0n]

Cool, looks good.. will merge after CI is done.

Thanks again for all your work here, much appreciated.

[bouk]

CI is done, and no problem! I'd much appreciate it if you cut a release 😄

[jejacks0n]

I'm not planning on releasing today because I'm pairing on some other things, but hit me up in a few days if I've forgotten.

[bouk]

@jejacks0n just checking in, don't think there's been a release yet

[winniehell]

@jejacks0n Do I get it right that there is still no release containing this? Judging from the changelog, 1.1.5 in March was the last release.