Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Errors thrown by asynchronous code crash jasmine-node #267

Closed
jwoudenberg opened this issue Oct 9, 2013 · 19 comments
Closed

Errors thrown by asynchronous code crash jasmine-node #267

jwoudenberg opened this issue Oct 9, 2013 · 19 comments

Comments

@jwoudenberg
Copy link

I've stumbled upon what I think is a bug in jasmine-node while testing code containing Q based promises. I've written a testcase in this gist:

https://gist.github.com/jwoudenberg/6906155

When running this test from the console, I see one dot for the first completed test and then it just stops. No error, no test-summary, nothing.

Running the same test with the stand-alone jasmine package seems to work fine and reports that the second (and third) errors have failed.

So what's going on here?

@davidposin
Copy link
Contributor

Jasmine-node has its own mechanism for handling asynchronous callbacks. The it function can have an optional done parameter. The done function needs to be called when in the callback for your asynchronous code.


(from the front page)
jasmine-node includes an alternate syntax for writing asynchronous tests. Accepting a done callback in the specification will trigger jasmine-node to run the test asynchronously waiting until the done() callback is called.

var request = require('request');

it("should respond with hello world", function(done) {
request("http://localhost:3000/hello", function(error, response, body){
expect(body).toEqual("hello world");
done();
});
});

@jwoudenberg
Copy link
Author

I tested it using the done() function too, and it still crashes.

Expected behaviour: done() is never called, so the test will fail.
What actually happens: Jasmine stops testing.

(function () {
    var Q = require('q');

    describe('A test', function () {
        it('Should succeed', function () {
            expect(false).toBe(false);
        });

        it('should fail with an error', function (done) {
            Q('foo')
            .then(function () {
                if (true) throw new Error('oops');
                done();
            })
            .done();

            expect(true).toBe(true);
        });

        it('Should fail', function () {
            expect(true).toBe(false);
        });
    });
})();

@davidposin
Copy link
Contributor

If done is not called, how does the test know to finish? I'm a bit confused.

@jwoudenberg
Copy link
Author

It should fail automatically after five seconds. From the same front page:

An asynchronous test will fail after 5000 ms if done() is not called.

The test never fails though, jasmine-node just crashes.

@andyfischer
Copy link

I hit the same issue. Here's a smaller test case:

describe('test', function () {
    it("should't crash", function (done) {
        setTimeout(function() {
            done();
            throw new Error('error');
        }, 0);
    });
});

The issue is that if an exception happens in asynchronous code, Jasmine-node will quit without printing anything. It won't report how many successes/errors there were. Here's my console output when running this test:

afischer-mba ~ $ jasmine-node --matchall test.js 
afischer-mba ~ $ 

(as in... nothing)

For reference here's the kind of output that I'd expect for this code:

afischer-mba ~ $ jasmine-node --matchall test.js 
F

Failures:

  1) test should't crash
   Message:
     Error: error
   Stacktrace:
     Error: error
    at null.<anonymous> (...etc...)
    at null.<anonymous> (/usr/local/share/npm/lib/node_modules/jasmine-node/lib/jasmine-node/async-callback.js:45:37)
    at jasmine.Block.execute (/usr/local/share/npm/lib/node_modules/jasmine-node/lib/jasmine-node/jasmine-1.3.1.js:1064:17)
    at jasmine.Queue.next_ (/usr/local/share/npm/lib/node_modules/jasmine-node/lib/jasmine-node/jasmine-1.3.1.js:2096:31)
    at null._onTimeout (/usr/local/share/npm/lib/node_modules/jasmine-node/lib/jasmine-node/jasmine-1.3.1.js:2086:18)
    at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

Finished in 0.017 seconds
1 test, 1 assertion, 1 failure

Reproduced with jasmine-node versions 1.6.0 and 1.11.0, using Node 0.10.17.

@davidposin
Copy link
Contributor

It has to be something to do with the timing involved in both cases. Jasmine-node works fine in asynchronous cases---I am working on an entire suite for my project as I type.

Maybe something in the delay mechanism is the problem?

@davidposin
Copy link
Contributor

Starting to work through the code, I suspect it has to do with the throwing
the Error. @andyfischer when I run your code via the command line on a
Windows machine I get a failure from timers.js. What platform are you
running on?

An odd point, but when I run the test through grunt I get a fatal
error:error message from jasmine-node and it dies.

On Wed, Oct 16, 2013 at 1:47 PM, andyfischer notifications@github.comwrote:

I hit the same issue. Here's a smaller test case:

describe('test', function () {
it("should't crash", function (done) {
setTimeout(function() {
done();
throw new Error('error');
}, 0);
});
});

The issue is that if an exception happens in asynchronous code,
Jasmine-node will quit without printing anything. It won't report how many
successes/errors there were. Here's my console output when running this
test:

afischer-mba ~ $ jasmine-node --matchall test.js
afischer-mba ~ $

(as in... nothing)

For reference here's the kind of output that I'd expect for this code:

afischer-mba ~ $ jasmine-node --matchall test.js
F

Failures:

  1. test should't crash
    Message:
    Error: error
    Stacktrace:
    Error: error
    at null. (...etc...)
    at null. (/usr/local/share/npm/lib/node_modules/jasmine-node/lib/jasmine-node/async-callback.js:45:37)
    at jasmine.Block.execute (/usr/local/share/npm/lib/node_modules/jasmine-node/lib/jasmine-node/jasmine-1.3.1.js:1064:17)
    at jasmine.Queue.next_ (/usr/local/share/npm/lib/node_modules/jasmine-node/lib/jasmine-node/jasmine-1.3.1.js:2096:31)
    at null._onTimeout (/usr/local/share/npm/lib/node_modules/jasmine-node/lib/jasmine-node/jasmine-1.3.1.js:2086:18)
    at Timer.listOnTimeout as ontimeout

Finished in 0.017 seconds
1 test, 1 assertion, 1 failure

Reproduced with jasmine-node versions 1.6.0 and 1.11.0, using Node 0.10.17.


Reply to this email directly or view it on GitHubhttps://github.com//issues/267#issuecomment-26456291
.

@davidposin
Copy link
Contributor

So this is what I get:
timers.js:103
if (!process.listeners('uncaughtException').length) throw e;
^
Error: error
at Object._onTimeout (D:\VSProjects\MyEsri\MyEsriNode\MyEsriNode\MyEsri\Server\tests\MyEsri.Function.spec.js:18:10)
at Timer.list.ontimeout (timers.js:101:19)

I suspect throwing an error at that level is throwing it to the node context and bypassing jasmine. In that case, I would suggest Jasmine's error matcher;

expect(function(){fn();}).toThrow(e);
https://github.com/pivotal/jasmine/wiki/Matchers

Putting the function wrapper around the code prevents the error from going beyond Jasmine and is still caught in the Jasmine context. That should also make it subject to the 5 second rule. Does that work?

@andyfischer
Copy link

@andyfischer when I run your code via the command line on a Windows machine I get a failure from timers.js

Huh.. I'm running on Mac.

An odd point, but when I run the test through grunt I get a fatal error:error message from jasmine-node and it dies.

Yep I see that too when running the tests through Grunt.

Putting the function wrapper around the code prevents the error from going beyond Jasmine and is still caught in the Jasmine context.

I guess that's an ok workaround, but it's going to be annoying to have to wrap every async test that we have. I'm not as worried about the tests that deliberately throw errors.. I'm more worried about accidental errors (like "Cannot call method of undefined"). Right now if I have a mistake like that, jasmine-node gives me zero information to help find it, which is kind of a dealbreaker for a large codebase.

@davidposin
Copy link
Contributor

@andyfischer Odd, there must be something going on that's different about your environment. The current small test I have running of 22 tests, and 55 assertions, is all async calls and I get both expect error and code errors (way too often, stupid bugs)

You only need to wrap it if you expect your code to throw an error up. I use that to test my validation code inside of callbacks and such.

@davidposin
Copy link
Contributor

@jwoudenberg if you wrap the promise in the thrown matcher you should get what you want
@andyfischer if there is any particular code I can look at to assist you, let me know

@andyfischer
Copy link

You only need to wrap it if you expect your code to throw an error up.

So the situation I'm more worried about is when an error is not expected. One last code sample:

describe('test', function () {
    it("should't crash", function (done) {
        setTimeout(function() {
            something = {};
            somethin.foo = 1;
            done();
        }, 0);
    });
});

In this code there's a typo ('somethin' instead of 'something'). Typos happen. In this case the typo will cause a ReferenceError: somethin is not defined error to be thrown. But because of the mentioned bug.. Jasmine won't print a stack trace or anything for this code. So imagine I have 10k lines of code in my project, and my test framework gives me zero information on where to look to find the typo. Hopefully that makes the problem clear? Thanks

@davidposin
Copy link
Contributor

So I'm not sure what the jasmine-node authors want to do about this. What you have found occurs but it is a definite edge case. The problem isn't that jasmine-node fails to capture async code errors. The problem is one of scope. setTimeout throwing an uncaught exception crashes the node server that jasmine is running on. Jasmine-node stops not because it found an error and crashed, it's that the setTimeout without error handling forces a fatal error on the underlying server. To avoid this, if you put a proper try/catch block inside the function, the jasmine test will timeout. Jasmine-node continues if the error is properly handled inside the function.

The original poster found the same issue via the promise. Since the error is being thrown and there is nothing to catch it, the error bubbles up to the server. Uncaught errors crash the node server. Therefore, the underlying server crashes in that case as well. A proper try/catch block would also have worked in the original case.

Also, I would suggest relying on something like jshint for code validation. I'm not sure a testing framework would be as efficient at catching things like typos as a validation module would. Plus, a validation module gives you a lot of options to optimize the validation process.

@tebriel A resolution would be to implement Domains inside of jasmine-node to prevent unhandled errors from bubbling up to the server.

@davidposin
Copy link
Contributor

Setting this should also help in this case.

"Often you'll want to capture an uncaught exception and log it to the console, this is accomplished by using the --captureExceptions flag. "

@justin-c-rounds
Copy link

Huge thanks to everyone on this thread — I was struggling with the same issue in one of my async tests. Used the captureExceptions flag and discovered the code I was testing was throwing an exception before the async test timed out.

I'm unclear on something, however. My async tests are using runs() and waitsFor() instead of the done() method, which seems to work fine. Is the done() method the recommended way to do async tests with jasmine-node, or is this just an alternative to using runs() and waitsFor()?

@davidposin
Copy link
Contributor

As far as I am aware, done is the preferred method.

However, if you are doing something behind the scenes that you can't directly override, mock, subscribe, or observe then the runs/waits methodology is a good backup. Don't use done if you use this approach.

@davidposin
Copy link
Contributor

oh, and very glad we could help

@tebriel
Copy link
Contributor

tebriel commented Mar 6, 2014

runs and waitsFor have been removed in Jasmine 2.0, now done is the standard way, in jasmine 1.3.x runs and waitsFor are the accepted methods.

@Giszmo
Copy link

Giszmo commented Aug 25, 2015

--captureExceptions totally does the trick. I wonder why that is not the default :( Would have saved me many many hours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants