Ava: `failFast` causes process to exit before `always.afterEach` hooks complete

[bundacia]

Description

If I try and do something asynchronous (or just slow?) from an always.afterEach() function while the failFast flag is set in my package.json the process won't wait for it to complete when the test fails. My specific case if is that I'm trying to do some database cleanup after each test and even though the afterEach hook gets called it doesn't seem to be given a chance to finish before the process exits.

I've tried to distill this down to a simple example:

import test from 'ava'

test.afterEach.always('CLEANUP', async t => {
  console.log('START', t.title)

  await new Promise(resolve => setTimeout(resolve, 5000))

  console.log('END', t.title)
})

test('Test', async t => {
  console.log('START', t.title)
  t.plan(1)

  t.is(1, 2)
  console.log('END', t.title)
})

This is the output I get:

START Test
END Test
  ✖ Test 1 === 2
START CLEANUP for Test

  1 test failed [10:42:18]


  1. Test
  AssertionError: 1 === 2
    _callee2$ (cleanup-test.js:15:5)
    Test.fn (cleanup-test.js:11:1)

I would expect to see END CLEANUP printed

Environment

Node.js v5.11.0
darwin 15.3.0

(Originally asked on SO: From http://stackoverflow.com/questions/37118762/ava-aftereach-callback-not-completing)

[novemberborn]

Fail fast causes the worker to be torn down without regard for test hooks.

I think we should add a gracefulStop() (or something like that) to the runner, which prevents new serial tests from being started and waits for any asynchronous tests to complete, including their after hooks. Any further test results should be discarded.

Tests won't fail as fast as before, but cleanup will be more reliable.

[jamestalmage]

I'm of the opinion that using after / afterEach for cleanup is a bad plan. I think you should have a conditional cleanup in a before / beforeEach instead.

It's impossible to provide both fast failure and reliable post-test hooks, and I would rather not crippple the fast fail behavior in favor of something I think is an antipattern

[sindresorhus]

I'm of the opinion that using after / afterEach for cleanup is a bad plan.

Why? I've always done cleanups in the after hooks too.

I think you should have a conditional cleanup in a before / beforeEach instead.

It doesn't sound very atomic to have to clean up after the previous test in the before hooks.

[jamestalmage]

I've always done cleanups in the after hooks too.

I did for a long time too, but things just work better the other way. I have more than once gotten myself into a situation where failing tests leave bad state on the file system that would be cleaned up in an after hook, but after hooks never get called because tests don't complete. Having after.always kinda solves the problem, but it still requires you run all your tests (which will fail because of dirty state) before the after hook gets run. This means you need to do a run you know will fail, just to cleanup state (or manually cleanup).

Cleaning up in a beforeEach also has a distinct advantage over always.afterEach when using fail-fast, in that the offending state is left in tact so you can examine it.

It doesn't sound very atomic to have to clean up after the previous test in the before hooks

Don't think of it as cleanup of the past state, think of it as ensuring a start state for the current one. Relying on the test before you to properly cleanup after itself is what isn't atomic.

[bundacia]

For what it's worth I've moved my cleanup to the before hook for basically the exact reasons @jamestalmage points out here.

[novemberborn]

Cleaning up in a beforeEach also has a distinct advantage over always.afterEach when using fail-fast, in that the offending state is left in tact so you can examine it.

fail-fast failing so fast that your code doesn't have a chance to clean up actually sounds like a feature. @vdemedes @sindresorhus @jamestalmage @sotojuan what do you think? If you agree we should document it as such.

[sindresorhus]

That's a good point, but definitely needs good docs on that as it can be surprising behavior.

I guess what I'd like is a hook that is run both before and after, so I can ensure it's cleaned up no matter what. Not a big problem for me though, as I've learned to use temp files for fixtures instead of putting them in the current directory, and they don't need cleaning up.

[vadimdemedes]

@novemberborn I think fail-fast should quit asap. If we allow cleaning up or running after hooks, other parallel tests will continue to execute. I agree with you!

[jamestalmage]

I've learned to use temp files for fixtures instead of putting them in the current directory, and they don't need cleaning up

Yes, that's pretty much what I always do now. Especially with AVA, since you need unique temp dirs/files for each test if you want to take advantage of concurrency. The only downside is that it is harder to find and inspect that temp location (requires adding a logging statement and cd-ing into the system temp folder).

[novemberborn]

858 documents this. The issue is now closed since it's expected (and one might say desired) behavior.