Make it easier to write reliable cleanup tasks

[jamestalmage]

See: #918 (comment)

after.always has some issues when used as a cleanup task. Specifically, it won't run if:

• There are test failures and --fail-fast is used.
• There are uncaught exceptions thrown.

I've advocated using .before or .beforeEach to ensure state is clean before running, but that means state is left on disk after the test run. It's easy enough to get around that:

function cleanup() {
  if (temDirExists()) {
    removeTempDir();
  }
}

test.before(cleanup);
test.after.always(cleanup);

Still, it might be nicer if we had a modifier that allowed you to do it a little cleaner:

// runs as before and after
test.cleanup(cleanupFn);

// runs as beforeEach and after (not sure afterEach makes much sense?)
test.cleanupEach(cleanupFn);

Or maybe we introduce a .and modifier:

test.before.and.after(cleanupFn);
test.beforeEach.and.after(cleanupFn);
test.beforeEach.and.afterEach(cleanupFn);

I think the second gives you a little more flexibility and is clearer without reading the docs. The first is probably simpler to implement (though I don't think the second would be very hard)

There is a $82.00 open bounty on this issue. Add more on Issuehunt.

• Checkout the Issuehunt explorer to discover more funded issues.
• Need some help from other developers? Add your repositories on Issuehunt to raise funds.

[novemberborn]

cleanup better communicates intended use. .and. feels overly verbose.

Agreed that afterEach doesn't make sense for .cleanupEach.

[sotojuan]

Another vote for cleanup. .and. reminds me of Mocha/Chai—cleanup just looks for AVA-ish if that makes sense.

[sindresorhus]

I prefer cleanup too.

Can we make it cleanup even on uncaughtException? I'd like cleanup to work no matter what happens.

[jamestalmage]

I lean toward and, though I see problems with it as well.

The only reason is that I think it makes it more obvious what is going on without reading the AVA docs. The notion that cleanup is going to happen both before and after is going to surprise people. They are going to write rmdir code without checking to make sure the directory exists, and be surprised when it fails. I think and better optimizes for test readability and being a self documenting API. cleanup seems only to optimize for characters typed.

My concern with and is that it might suggest combinations we don't want to support.

Can we make it cleanup even on uncaughtException? I'd like cleanup to work no matter what happens.

We can try. There is no guarantee it would work.

The only way to guarantee cleanup works would be to kill the first process immediately, then relaunch and run just the cleanup method. That falls apart if you use something like unique-temp-dir and store a reference for cleanup (not a good example - no need to clean up temp).

[vadimdemedes]

Hmm, wasn't .always supposed to serve this exact same purpose? To always run the clean up task, regardless of previous failure? Perhaps it's worth fixing the existing modifier, instead of introducing a new one. How would we explain the difference between the two to the user?

[sindresorhus]

@vdemedes I would prefer that too, but I remember there being some good points about why .always can't actually be always. Can't remember where we discussed it. @jamestalmage @novemberborn ?

[sindresorhus]

My concern with and is that it might suggest combinations we don't want to support.

Not just that, but now we're introducing combinatory methods, making the test syntax harder to understand. Starting to get bit too DSL'y.

[novemberborn]

Hmm, wasn't .always supposed to serve this exact same purpose? To always run the clean up task, regardless of previous failure?

That was our assumption, yes. The question though is how --fail-fast is supposed to behave. The fastest way to fail is to forcibly exit the process. Trying to run just the always hook of the failing test whilst other asynchronous tests are active is tricky. For debugging purposes it can be useful if test state remains after a failure, --fail-fast would provide that behavior.

After an uncaught exception there are no guarantees as to what code may still be able to run. Here too we end up forcibly exiting the process.

Note that in both cases we first do IPC with the main process before actually exiting. I'm not quite sure how much code still has a chance to run. It's possible always.after is entered but if it does anything asynchronous then that gets interrupted. This needs some research.

We should decide what guarantees we want from --fail-fast and uncaughtException.

Also, our understanding of "test cleanup" has evolved to the point where we know see the best strategy of ensuring a clean test environment is to clean up before you run your test, and then clean up after to avoid leaving unnecessary files and test databases lying around. Hence the proposal for a .cleanup hook.

Note that .always.after is still useful in other scenarios, e.g. #840.

[sindresorhus]

After an uncaught exception there are no guarantees as to what code may still be able to run. Here too we end up forcibly exiting the process.

Maybe we could rerun the process and only run the .always.after hooks.

[jamestalmage]

See #928 (comment)

That falls apart if you use something like unique-temp-dir and store a reference for cleanup (not a good example - no need to clean up temp

[catdad]

Just my two cents, since I was welcomed to comment. I have only used ava for one project so far, but I immediately ran into this issue and it confused me very much as a user and avid code tester.

My expectation, when splitting my code up into a before, test, and after, is that I am doing build-up and tear-down that needs to happen in order to test a small piece of functionality, but does not otherwise relate to the test code itself. In order for my tests to be predictable, I expect that these blocks always run, no matter what. From an end-user perspective, having an after, after.always, and cleanup is just confusing and redundant. Of course I want to clean up, and of course I want that to always happen. What are the use cases for anything else? (Yes, I understand technical debt and backwards compatibility, but they should only be considerations in design, not driving forces.)

Also, I see some bad advice and rhetoric happening here and in #918, suggesting that cleanup should happen before and after, allowing you to re-run tests even in a dirty environment. While I don't disagree that it is a good idea to check that your environment is exactly as desired before a test, there has to be a stick in the ground saying that unit tests must not permanently alter the environment, even if there are other tools in the toolchain (such as .gitignore) which will handle that for you. And to the extent that that is the fault of the test framework, it should be treated as an egregious and urgent bug. As an end-user, I should not have to choose between speed and sane, repeatable tests.

With that said, I would support the option of a .cleanup hook that ava transparently runs both before and after (and always run it after), provided that the use cases are fully considered. There might potentially be issues with running unexpected cleanup code before the tests, as that is rather unorthodox among the other test frameworks.

[novemberborn]

@catdad

While I don't disagree that it is a good idea to check that your environment is exactly as desired before a test, there has to be a stick in the ground saying that unit tests must not permanently alter the environment, even if there are other tools in the toolchain (such as .gitignore) which will handle that for you. And to the extent that that is the fault of the test framework, it should be treated as an egregious and urgent bug.

A crash due to a bug in AVA would be something we'd fix, yes. But what if the crash is caused by the code being tested? There is no 100% reliable way to recover from that and undo changes to the environment. Similarly the --fail-fast feature is designed to leave the environment in the state in which the failure occurred.

In other words, unless your tests always pass, there will be moments where a test run leaves the environment in a different state. AVA can't know that's the case since it doesn't understand before/after code. At least the cleanup hook makes it easier to sanitize your environment before test runs (in case it was left in a dirty state), and after test runs (because cleaning up is nice).

There might potentially be issues with running unexpected cleanup code before the tests, as that is rather unorthodox among the other test frameworks.

We should clearly document the intent of the hook. But AVA is not afraid of being unorthodox 😉

[catdad]

I have never seen another test framework do this, and I have used quite a few. I have had code throw errors, both intentional and unintentional, both synchronously and asynchronously, have typos, and flat out be running invalid JavaScript, and the test framework catches that and still runs the after method. While it may not be 100% reliable, as you say, AVA is very far from approaching that percentage, and it seems to be on purpose. I would prefer that my test framework take an extra few milliseconds to clean up after itself (or run the code that I conveniently wrote for it) than to fail as fast as possible and have me need to clean up manually.

AVA may not understand the before/after code itself, but no environment does. They just understand that they have to run that code. Whether the code does cleanup, just says "hi", or does absolutely nothing, it is not up to AVA to decide whether or not to run it. The dev wrote that code in the test for a reason, and has the expectation that the code will be run. If after doesn't always run, then what really is the point of after?

We should clearly document the intent of the hook. But AVA is not afraid of being unorthodox

I have actually seen this cleanup problem in test suites written by highly active contributors to AVA. 😉 Being unorthodox is great as long as it adds value. In this case, it looks like it's actually taking away value.

[novemberborn]

@catdad It's only --fail-fast and crashes that prevent always.after from running. The benefit of cleanup over after is that it helps deal with those scenarios, too.

[sholladay]

This bit me today. A test failed and temporary files were left all over simply because I had .afterEach instead of .always.afterEach. I don't see the logic in skipping .afterEach when a test fails. Puking on the environment, if this is even a desired feature, should be opt-in.

--fail-fast feature is designed to leave the environment in the state in which the failure occurred

Yikes. That seems like a major mixing of concerns. This feature is named --bail in other test frameworks and the thing that is bailed on is tests, not cleanup hooks. The command to do what you mention would be --bail --no-cleanup.

[novemberborn]

@sholladay I like your suggestion of having a --no-cleanup flag! @avajs/core?

[vadimdemedes]

I think --no-cleanup may be a wrong way to approach this issue. I agree with this statement of @sholladay:

I don't see the logic in skipping .afterEach when a test fails.

If it would be totally up to me, I'd consider removing .always and making it a default behavior for .afterEach.

[novemberborn]

I don't see the logic in skipping .afterEach when a test fails.

If it would be totally up to me, I'd consider removing .always and making it a default behavior for .afterEach.

We previously discussed this in #474 (comment). The concern was that the after / afterEach hooks may have assumptions on the tests having passed / reached a certain state. Hence the .always modifier which acts as an explicit opt-in to having the callback called.

[vadimdemedes]

Yeah... Looks like there's no win-win solution at the moment for this.

[sholladay]

I think --no-cleanup may be a wrong way to approach this issue.

I would ask "why", but it's going off on a bit of a tangent. I haven't personally needed something like that. But it does exist elsewhere and cleanly solves the "leave the environment in the state in which the failure occurred" story, even though I think that is a bad idea 95% of the time. If it needs to be a thing, it can be an option. I don't think it should be default.

after / afterEach hooks may have assumptions on the tests having passed / reached a certain state

This sounds hypothetical to me. I would bet money that the vast majority of cleanup hooks are doing the equivalent of rm -rf foo.

I know AVA likes to pave its own path, and to good effect. But I think Intern really gets this right. It makes strong guarantees that if a test runs, its hooks run.

[novemberborn]

I know AVA likes to pave its own path, and to good effect. But I think Intern really gets this right. It makes strong guarantees that if a test runs, its hooks run.

Yea that makes sense to me too. #840 discusses providing the test status to an afterEach hook. Combine the two and we can do away with .always. Users who need to guard against test execution state can either access the state or use another guard of their choosing.

I think --no-cleanup may be a wrong way to approach this issue.

I would ask "why", but it's going off on a bit of a tangent. I haven't personally needed something like that. But it does exist elsewhere and cleanly solves the "leave the environment in the state in which the failure occurred" story, even though I think that is a bad idea 95% of the time. If it needs to be a thing, it can be an option. I don't think it should be default.

I think this is related, actually.

We have issues with bringing test execution to a halt when --fail-fast is enabled, see #1158. Always running after hooks makes that more complicated. Perhaps --fail-fast can imply --serial (while still allowing for test file concurrency). We can then add a --no-cleanup (only allowed together with --fail-fast) which is the only case in which after hooks do not run.

I like how this simplifies the mental model for most use cases.

[sholladay]

Is there any chance this could make it in for 1.0.0? Needing to remember .always is among my least favorite things about AVA currently. And removing it is a breaking change.

[novemberborn]

I'd like to focus on outstanding Babel interoperability issues.

But don't worry we won't shy away from making breaking changes when necessary, and we can support a deprecation path.

[sholladay]

Okay, that makes sense. The Babel interop is clearly pretty complex and a lot to keep track of. FWIW, though, it's been working pretty well for my own use cases. 😃

I'll see if I or someone on my team can contribute to this when you think the time is right. Seems like a good first step is to expose the test pass/fail/error status to the hooks? That doesn't even have to be a breaking change necessarily.

But don't worry we won't shy away from making breaking changes when necessary, and we can support a deprecation path.

That is great to hear. 🎹 👂

[IssueHuntBot]

@issuehuntfest has funded $80.00 to this issue. See it on IssueHunt

[IssueHuntBot]

@rororofff has funded $2.00 to this issue.

---

• Submit pull request via IssueHunt to receive this reward.
• Want to contribute? Chip in to this issue via IssueHunt.
• Checkout the IssueHunt Issue Explorer to see more funded issues.
• Need help from developers? Add your repository on IssueHunt to raise funds.

[altras]

Meanwhile has somebody found a workaround for this missing feature? Like doing a global try ... catch for the whole test file?

It's very annoying to do manual cleanup 😊

[novemberborn]

Like doing a global try ... catch for the whole test file?

Tests start running asynchronously so that won't work unfortunately.

[sholladay]

I wonder if test() could return a Promise and then when we get top-level await you could wrap all tests in a try / catch? That would be interesting!

[novemberborn]

I wonder if test() could return a Promise and then when we get top-level await you could wrap all tests in a try / catch? That would be interesting!

You'd have to assign all those promises to an array and await them at the end, though, since AVA requires you to declare all tests at once. It also means errors are attributed to "well the process had an exception" rather than a specific cleanup task.

[ulken]

@novemberborn Due to the age of this issue and the introduction of t.teardown(), is this still desired?

[novemberborn]

I've been thinking about this for a while now. I think as part of #2435 I'd like to have a "set-up" lifecycle thing which can return a teardown function. It's a different use case from "run this before" and "run this after".

I've assigned this to myself for now.

[mikob]

@novemberborn Is this still relevant? Curious why it was removed from priorities. I'm considering doing a PR with a cleanup method for the API.

I'm still struggling to have cleanup code run consistently. I would like cleanup to be run on test success, test failure, uncaught exceptions, timeout, and SIGINT (ctrl-c) - and ideally all the other kill signals. I have tried after.always, afterEach.always and t.teardown. My use case is closing webdriverio clients (that eat lots of ram) when tests aren't running.

I've considered doing a temporary workaround by catching the node process exit signal, but how would I get access to the ava context from there which has the webdriverio client instances?

I like your suggestion of having a --no-cleanup flag! https://github.com/orgs/avajs/teams/core?

I think this is unnecessary as it could be done trivially in user-space with an if statement in the cleanup hook that checks a user-defined env. var e.g. SKIP_CLEANUP.

[novemberborn]

Curious why it was removed from priorities.

I don't know @mikob, that was 3 years ago! 😄

I would like cleanup to be run on test success, test failure, uncaught exceptions, timeout, and SIGINT (ctrl-c) - and ideally all the other kill signals.

I think this is the tricky bit. At some point the worker process/thread needs to exit, especially if there's been a fatal error. Within the API that AVA provides there'll always be ways in which cleanup cannot occur.

My use case is closing webdriverio clients (that eat lots of ram) when tests aren't running.

I wonder if AVA 4's shared workers feature could be used for this. It can track the comings and goings of test workers and runs separate from the tests. The tricky thing may be to expose the clients to the tests.

I've considered doing a temporary workaround by catching the node process exit signal, but how would I get access to the ava context from there which has the webdriverio client instances?

Could you hook that up when you create the clients?

[mikob]

I don't know @mikob, that was 3 years ago! 😄

Haha, fair enough!

Within the API that AVA provides there'll always be ways in which cleanup cannot occur.

I don't think we need a 100% guarantee. It's more a convenience and responsibility thing. I feel it's fairly common for a test to throw unhandled exceptions, after all, they are testing. And SIGINT when we quit tests prematurely.

It can track the comings and goings of test workers and runs separate from the tests. The tricky thing may be to expose the clients to the tests

It would make more sense to have the setup/cleanup handled by AVA apis symmetrically IMO. Also I'm doing 1 client per test. Since I create a webdriverio client in beforeEach, a cleanup or afterEach should tear it down.

Could you hook that up when you create the clients?

I'm talking about process.on("SIGINT" and the clients are created in beforeEach (need one per test) not sure how to access all the workers contexts from within the process.on callback.

[novemberborn]

@mikob re-reading the conversation here I think #928 (comment) sums up a direction we can take this.

But I don't think that would solve your problem.

Do you get PIDs for the clients? You could send those to a shared worker that makes sure they get shut down, yet still instantiate within the test worker itself.

[bitjson]

To add another case here: these end to end tests spawn multiple instances of the tested software to test how they interact. There's significant setup, and tests are all serial.

If a test times out, the test process ends but leaves these spawned processes running (which might continue logging to files, holding ports open, and otherwise interfering unpredictably with future test runs) – this happens even though Execa should be cleaning them up – I think because AVA is killing the timed-out tests with SIGKILL?

Even disabling --fail-fast and attempting to manually kill the processes using after.always doesn't get rid of them:

test.after.always(async () => {
  const processes = [p1, p2, p3];
  processes.forEach((p) => {
    if (p !== undefined) {
      p.kill('SIGKILL');
    }
  });
});

The current behavior is pretty surprising – I've thought these were unrelated issues with the tested software for a while now, and I only just now realized it's all related to this AVA issue. A more predictable behavior here would be much appreciated 🙏

[novemberborn]

Child processes are killed with SIGTERM, worker threads (the default behavior) are terminated… and I'm not sure what / how that impacts Execa.

Clean-ups when tests timeout are pretty tricky since the worker thread itself may be locked up.