`after.always` doesn't run if there are uncaught exceptions

[arve0]

Hi! This is the second time I read the documentation at README.md and find that the feature I'm using is not yet released (after some debugging). I'm not sure what it was last time, but this time it is after.always found in commits since release.

Any thoughts about stabilizing/syncing the docs with the release cycle?

[sindresorhus]

after.always was in the 0.15.0 release. If you're referring to 2e98438, it's just updating the TypeScript definition.

Any thoughts about stabilizing/syncing the docs with the release cycle?

New features always come with docs. That's a requirement.

[arve0]

Hm. I'll recreate my issue, give me some seconds.

[arve0]

OK, this is actually a bug. This code will reproduce:

var test = require('ava');
var r = require('request-promise');

test.before(() => console.log('before'));

test('failing test', (t) =>
  r('http://httpbin.org/status/400')
    .catch(err => t.true(err.statusCode === 200)));

test('another failing test', (t) =>
  r({
    uri: 'http://httpbin.org/status/400',
    method: 'POST',
    body: { should: 'be string' } // this will throw
  }).catch(err => t.true(err.statusCode === 200)));

test.after.always(() => console.log('after'));

Seems to be a race condition. after runs if running test serially or if one of the tests is omitted.

https://asciinema.org/a/8z74ozt2e1tahjlqlx3mnj774

[arve0]

Throws from a place ava is unable to catch? Might not really an issue, as this is misuse of request-promise, as body should be a string or json should be set to true.

[arve0]

Simpler test case:

var test = require('ava');

test.before(() => console.log('before'));

test.cb('another failing test', (t) => {
  setTimeout(() => {
    t.true(true);
    throw new Error('catch me');
  }, 1);
});

test.after.always(() => console.log('after'));

Output

before

   2 exceptions


   1. Uncaught Exception
   Error: catch me
    Timeout._onTimeout (test.js:8:11)


   ✖ Test results were not received from test.js

Is this expected?

[jamestalmage]

We shut the process down immediately if an uncaught exception happens, so after.always isn't guaranteed to fire in that case.

I guess we could trigger after.always in the case of uncaught exceptions.

[arve0]

Nice when using --watch, cleaning state in after and unintentionally writing something wrong in your test.

[jamestalmage]

You will always be able to create situations where your cleanup code fails to do what you want, even with after.always.

import test from 'ava';
import mkdirp from 'mkdirp';
import rimraf from 'rimraf';

test.cb(t => {
  setTimeout(() => {
    mkdirp('some/deeply/nested/directory');
  }, 20);

  setTimeout(() => {
    cb();
    throw new Error('woops');
  }, 10);
});

test.after.always.cb(t => {
  rimraf('some/deeply', t.end); // <= this is likely executing before the `mkdirp` line.
});

Instead of banking on the system being left in a clean state by a previous run, I recommend using before / beforeEach to ensure system state before running the test.

function cleanup(t) {
  if (fs.exists('some/deeply')) {
    rimraf('some/deeply', t.end);
  }
}

test.before.cb(cleanup);

// if leaving that directory hanging around bothers you:
test.after.cb(cleanup);

[arve0]

Adding context, the before/after is starting/stopping the rethink database, which is quite expensive (~2-3 seconds).

[arve0]

Went for checking if database and server are alive:

let db, server;
test.cb.before((t) => {
  let timeout = 0;
  if (child.spawnSync('pgrep', ['-lf', 'rethinkdb']).status === 1) {
    console.log('---- STARTING DATABASE ----');
    db = child.spawn('rethinkdb', [], { cwd: __dirname, stdio: 'inherit' });
    timeout += 3000;
  }
  if (child.spawnSync('pgrep', ['-lf', '(node|nodemon) index.js']).status === 1) {
    console.log('---- STARTING SERVER ----');
    server = child.fork('index.js', [], { cwd: __dirname, stdio: 'inherit' });
    timeout += 100;
  }
  // wait for server to start
  function wait () {
    request(HOST + 'captcha')
      .then(() => t.end())
      .catch(wait);
  }
  setTimeout(wait, timeout);
});

Drawback: not cross platform.

[novemberborn]

Uncaught exceptions crash the worker process. We can't really make any guarantees about whether after hooks are run. Documenting it would be good though, so 👍 @arve0 for your PR.