computmaxer / karma-jspm

Other
74 stars 50 forks source link

Silent Errors #56

Closed scott-coates closed 9 years ago

scott-coates commented 9 years ago

7 Introduced a change so that any System.import errors would be thrown to Karma. Before this change, errors from System.js were silent altogether.

I'm using Karma@v0.12.31 and I'm still getting silent errors. Take a look at https://github.com/Workiva/karma-jspm/blob/4d3511e6d928ff6e6d8fa0d79249c46c6d3276bc/src/adapter.js#L58

and you'll see

 ['catch'](function(e){
                setTimeout(function() {
                    throw e;
            });
        });

If I make the following change, the errors are correctly reported to Karma:

 ['catch'](function(e){

                    throw e;

        });

So, throwing immediately instead of on the next tick fixes my issue.

Thoughts? Am I missing something, why are we using setTimeout?

seph-m commented 9 years ago

This actually causes a relatively serious problem. Say there is a javascript error in one of your test files. This issue causes all of the tests in your project to run successfully except for the tests after the error in the file where the error occurs. You wont realize that your tests are not actually being run unless you keep close track of how many tests should be running. We just ran into this, and I am glad to have tracked it down after spending only 8 hours investigating. The following code will exhibit the behavior:

it('should do stuff', function() {
    expect(true).to.equal(true);

});

it('should do more stuff', function() {
    expect(true).to.equal(true);

});

//All the tests in this file after this point will not be run, but no error will be reported and 
//the test run will still succeed.
var a = asdf.length;
var asdf = [1,2,3];

it('should do even moar stuff', function() {
    expect(true).to.equal(true);

});

In this case, the first 2 tests will run and succeed, but the third test will just not be run at all. No error will be reported and karma will run to completion reporting that 2 of 2 tests ran successfully. The only place to see the "length is undefined" error is by looking in the debug console in chrome, which you dont usually do unless there is a broken test. So, broken code just got merged because all the tests would "pass" in the pull request build, and you would never know that some of them were not run at all. If I modify the adapter.js code as noted above by @scoarescoare , then everything functions as expected, and the test run is aborted due to the error.

Im sure there was a reason to wrap the throw in a setTimout, but we might have to rethink it given that this causes a dangerous and very difficult to track down problem.

Gu1 commented 9 years ago

Just ran into the same problem... And also spend 8 hours debugging it...