Asynchronous errors do not fail tests

[ORESoftware]

Hey all

Just wondering if this is expected output

var test = require('ava').test;

test.beforeEach(t => {
   console.log('before each');
});

test(t => {
     setTimeout(function(){
       throw new Error('123');
     });
});

test(t => {
     setImmediate(function(){
       throw new Error('456');
     });
});

test(t => {
     process.nextTick(function(){
       throw new Error('789');
     });
});

output:

  3 passed
  3 exceptions

  1. Uncaught Exception
  Error: 789


  2. Uncaught Exception
  Error: 456


  3. Uncaught Exception
  Error: 123

shouldn't it be 0 passed, 3 exceptions?

[novemberborn]

@ORESoftware some relevant fixes were made in 0.12. Which version are you using?

[sindresorhus]

I'm getting the same issue with AVA 0.12.

before each
before each
before each

  3 passed
  3 exceptions

  1. Uncaught Exception
  Error: 789
    test.js:21:14


  2. Uncaught Exception
  Error: 456
    Immediate._onImmediate (test.js:15:14)


  3. Uncaught Exception
  Error: 123
    [object Object]._onTimeout (test.js:9:14)

[sindresorhus]

var test = require('ava').test;

Btw, can be just var test = require('ava');

[vadimdemedes]

I don't think we can catch errors on next tick, because at that time test function has completed. And we also can't know where this uncaught error came from. To sum up, I'm not sure we can do anything about it, without using domains.

[novemberborn]

@vdemedes makes sense.

Presumably using t.plan() would prevent the test from passing (or indeed ending).

[ORESoftware]

yeah just fyi

the same thing will happen if you replace

setTimeout/setImmediate/process.nextTick with any other standard async
functions such as

fs.exists('foo', function(err){
throw new Error();
});

it's all about throwing an error inside a function called asynchronously.

the only real solution that I know of is the domain core module. but the
problem is ES6 promises do not support domains because domains are pending
deprecation. My strong belief is that without domains you won't be able to
solve this problem, so my only recommendation is to try to get Node.js TC
to backtrack on domains. Bluebird library supports domains, but not ES6
promises fyi.

Without domains you will just have to have an uncaughtException handler but
you lose all context of which test failed. I tried the same setup as above
in Vows by the way and they literally fail out, on the first error, they
dont seem to even have an uncaughtException error handler.

@vdemedes https://github.com/vdemedes makes sense.

Presumably using t.plan() would prevent the test from passing (or indeed
ending).

—
Reply to this email directly or view it on GitHub
#567 (comment).

[ORESoftware]

You can replace what I wrote above with the following code which is even more realistic, and you should see the same problem as above.

const test = require('ava');
const fs = require('fs');

test(t => {
   return new Promise(function(resolve){
     fs.exists('foo', function(){
       throw new Error('123');
       resolve();  // won't get reached, but here for clarity
     });
   });
});

test(t => {
   return new Promise(function(resolve){
      setTimeout(function(){
         throw new Error('123');
         resolve(); // won't be reached, but here for clarity
     }, 100);
   });
});

test(t => {
      return new Promise(function(resolve){
        setImmediate(function(){
           throw new Error('456');
           resolve(); // won't be reached, but here for clarity
     });
  });
});

test(t => {
 return new Promise(function(resolve){
     process.nextTick(function(){
          throw new Error('789');
          resolve(); // won't be reached, but here for clarity
     });
   });
});

[ORESoftware]

also, Mocha and Tape can solve this problem because they run tests serially, which means they can set the "current test" or "current hook" in the global scope, and then on an uncaughtException they can attach that exception to the current test / hook, but with asynchronous code, you simply do not know what the current test is, at the global scope

[sindresorhus]

Looking at your first example with fresh eyes, it's obvious it wouldn't work as you're trying to use async inside a sync test. Sync tests end in the current tick. If you want to use callbacks, use test.cb().

As for your second example. This is what I got with latest AVA:

4 exceptions

1. Uncaught Exception
Error: 789
  test.js:34:17


2. Uncaught Exception
Error: 456
  Immediate._onImmediate (test.js:25:18)


3. Uncaught Exception
Error: 123
  test.js:7:14
  FSReqWrap.cb [as oncomplete] (fs.js:212:19)


4. Test results were not received from test.js

It shows the correct line for all but setTimeout.

However, I would say you're using it incorrectly. You shouldn't throw asynchronously inside a promise. That's what reject() is for (or promisify the setTimeout function - see delay). If you want to use async tests, use promise compatible semantics. Otherwise, use test.cb().

Might help if you could provide a realistic scenario where this is a problem.

[ORESoftware]

nah, it's going to break down quickly, if you move the asynchronous functions outside the test cases, we now have two functions checkFileExistence1 and checkFileExistence2 that do work outside the test cases themselves (just like you would if you were unit testing code that was not actually in your test suite).

like so

var test = require('ava');
var fs = require('fs');


///////////////////////////////////////////////////////////

function checkFileExistence1(filename, cb){
    fs.exists(filename, function(err){
        throw new Error('123');
        cb(err);  // won't get reached, but here for clarity
    });
}

test(t => {
    return new Promise(function(resolve){
        checkFileExistence1('foo',function(err){
            resolve();
        });
    });
});


///////////////////////////////////////////////////////////////


function checkFileExistence2(filename){
    return new Promise(function(resolve){
        fs.exists(filename, function(err){
            throw new Error('123');
            resolve();  // won't get reached, but here for clarity
        });
    });

}

test(t => {
    return new Promise(function(resolve){
        return checkFileExistence2('foo');
    });
});

as you can see, the line numbers are not reported accurately - meaning - all we have is an uncaught exception, and we don't know which test the errors pertain to.

which is why we get this output

  1. Uncaught Exception
  Error: 123
    test-ava.js:14:15
    FSReqWrap.cb [as oncomplete] (fs.js:212:19)


  2. Uncaught Exception
  Error: 123
    test-ava.js:34:19
    FSReqWrap.cb [as oncomplete] (fs.js:212:19)


  3. Test results were not received from test/test-ava.js

I created a gist for this
https://gist.github.com/ORESoftware/218d7aafd3e3049cd234

I don't think you can solve these kinds of problems easily, so it's probably seriously best to just ignore that this is an issue for now, and perhaps future developments like asyncwrap which supposedly will replace domains will help solve the problem. As long as you have an uncaughtException handler (which this lib does of course), it should be ok (your test suite won't crash), but you won't be able to report the right test as having failed or the line number of the test that failed. Perhaps if more node.js libs get promisified (like fs core) these types of problems will go away, but I am not sure about that.

[ORESoftware]

along with the gist above, which shows a bug

this also appears to be a bug, possibly related, thanks for listening, don't kill the messenger :)

https://gist.github.com/ORESoftware/b56c9330e48ef225b03c

[sindresorhus]

Yeah. We'll keep this open, but I doubt we'll do anything until AsyncWrap lands in Node.js.

You could do what I do and just manually promisify all callback API's. It makes the tests so much nicer as a bonus. See: pify or Bluebird.promisify.

[robertrossmann]

Is there a particular reason why domains cannot be used? Except, of course, that they are "soft-deprecated". It seems that simply wrapping all the test implementation functions in a domain.run() should work just fine, as long as each implementation function gets its own domain instance (which is ok).

[robertrossmann]

Even an error thrown synchronously from a callback test seems to cause ava to just hang. Note that these errors do not necessarily have to come from assertions or intentionally thrown errors, but also from merely making a mistake while writing the test itself, ie. accidentally making a typo in a function call, causing errors such as undefined is not a function to be completely swallowed, leaving the developer clueless as to what is wrong with the test.

test.cb('sync Error in callback-based test causes ava to hang indefinitely', () => {
  process.chdr(__dirname) // TypeError: process.chdr is not a function (typo)
})

[jamestalmage]

Domains don't exist in the browser, so relying on them becomes problematic as we move towards browser support.

There's a PR to fix sync throws that will be merged shortly.

Async throws remain problematic, but we've got an idea for handling that via a custom Babel plugin.

I am on mobile now, but will link tp the relevant issues later.

[loris]

I confirm the bug:

function asyncFn(cb) {
  cb(null, null);
}

test.cb(t => {
  asyncFn((err, res) => {
    t.is(res.bar, 'ok');
    t.end();
  });
});

This will cause ava to hang indefinitely instead of displaying an error like TypeError: Cannot read property 'bar' of null

Not sure why it is tagged as low priority, it makes ava a pain to use to test callback-based methods.

[novemberborn]

@loris your example triggers a test.cb() bug that's been fixed in master. Regardless, the best way to write such tests is to wrap the async functions in a promise and use async/await. That way errors will bubble up to the individual test and can be reported correctly.

This is low-priority because the problem of asynchronous errors goes way deeper than AVA, and even the workarounds we can do here require a large amount of effort. We have other, more pressing concerns.

[sindresorhus]

Just wrap callback APIs in pify and you will have a much better experience in general.

[ORESoftware]

@sindresorhus I tried to make it abundantly clear above that promisifying things won't solve everything, for example:

const test = require('ava');
const fs = require('fs');

function asyncFn() {
    return new Promise(function (resolve) {
        fs.exists('foo', function () {
            // some moron decides to throw here :)
            throw new Error('unlikely but very very possible'); 
        });
    });

}

test(t => {
    return asyncFn().then(function (res) {
        t.is(res.bar, 'ok');
    });
});

We run the above and we get:

  2 exceptions

  1. Uncaught Exception
  Error: unlikely but very very possible
    FSReqWrap.cb [as oncomplete] (fs.js:212:19)


  2. Test results were not received from test\ava\ava-ex.js

90% of APIs are still callback based. if Pify/Promisifying can solve this, maybe the above example is a good one to use to demonstrate this? On this open issue thread? Nevertheless, if you don't have control of the asyncFn because it's in someone else's library, then it gets harder, but let's pretend you do have 100% control.

[ORESoftware]

@loris

the reason why your example hangs is most likely because you need to do this instead

const test = require('ava');

function asyncFn(cb) {
    process.nextTick(function(){   // force async here
        cb(null, null);
    });
}

test.cb(t => {
    asyncFn((err, res) => {
        t.is(res.bar, 'ok');
        t.end();
    });
});

the above works as expected, however, using the latest version of ava, if I omit the process.nextTick, it does hang, FYI, so you are correct in that observation.

[novemberborn]

@ORESoftware that's a bad example. If you use pify or alternatives you need to ensure you resolve/reject the promise with the asynchronous callback results, and do any further processing in a .then() callback, which will correctly propagate exceptions.

[novemberborn]

#214 already discusses AVA's inability to trace uncaught exceptions back to their tests. I've opened #1045 to document the resulting pitfalls, with a suggestion to promisify callback-taking functions.

Closing this as a duplicate of #214.