Add chainable .always method

[therealklanni]

Implement chainable always method.

From @jamestalmage's comment below:

What if it's always, and chainable:

test.always - even if a beforeEach fails.
test.afterEach.always - even if a test fails.
test.after.always - the OPs request.

Not sure the first one makes sense.

---

Original message

Near as I can figure, the .after() hook is only being ran if the tests have passed. I have a test file something like this (simplified/pseudo for clarity)

// ... imports

test('create a file', ...) // passes

test('do something else', ...) // fails

// ... other tests

test.after('clean up created files', ...) // never runs

[jamestalmage]

Yep. That is the currently defined behavior. Your use case makes sense, so maybe we should change it.

The current behavior is useful in other situations though (if after actually requires successful execution of your tests, etc).

Perhaps a finally test type is in order (one that always runs, no matter the results from before)

[therealklanni]

Yeah, a .finally method would make sense. I had to check the docs last night when I realized .after wasn't working (the way I thought it was meant to) to see if I had missed some other method.

So I would assume that the appropriate way to handle it would be to make .finally be the one that runs at the end no matter what. Fair assumption?

[jamestalmage]

Either that or just change after to do what you expect. Let's see what everyone thinks.

[vadimdemedes]

I think it's definitely a valid request and has its right to be in the core. And .finally() looks good too. But unfortunately .finally() for "each" does not: finallyEach(), meh.

[vadimdemedes]

What if we make this a default behavior?

[jamestalmage]

What if it's always, and chainable:

• test.always - even if a beforeEach fails.
• test.afterEach.always - even if a test fails.
• test.after.always - the OPs request.

Not sure the first one makes sense.

[vadimdemedes]

I like this idea too.

Not sure the first one makes sense.

Nope, it does not :)

[therealklanni]

Seconded @vdemedes's last comment

[therealklanni]

@sindresorhus can we get some help wanted etc labels on this? 😄

[sindresorhus]

I like this idea too.

👍

@therealklanni Done. Sorry about the delay. I've been traveling.

[therealklanni]

No worries, thank you

[therealklanni]

@sindresorhus I started working on this a few weeks ago. Keep meaning to get back around to it, but I'm just swamped lately. Rather than leaving it stagnant any longer I wanted to let you know that I probably won't have time to work on it (certainly not any time soon). I just have too much going on right now, sorry.

[novemberborn]

No worries @therealklanni!

[cgcgbcbc]

seems that class Sequence must be modified for this feature, I'll try to work on this issue, any advices?

[jamestalmage]

I'm not sure Sequence will need to be modified.

Sequence takes a list of tasks and a boolean bail argument.

We currently build a list like this:

+ bailing sequence
  - beforeEach
  - beforeEach
  - test
  - afterEach

I think you could just wrap that in an non-bailing sequence:

+ non-bailing sequence
  + bailing sequence
    - beforeEach
    - beforeEach
    - test
    - afterEach
  - afterEach.always (afterEach is promoted to the non-bailing wrapper if it has a `always` modifier)

[jamestalmage]

This sequence building takes place in test-collection.js.

Chainable methods are defined here.

[cgcgbcbc]

Thanks for your advice! I'll try to wrap them as follows:

1. wrap the final return Sequence: wrap it in a new Sequence followed by after.always as non-bailing if necessary
2. wrap serialTests, concurrentTests: wrap them in a new Sequence followed by afterEach.always as non-bailing if necessary

As for the chain modification, I'll add a new item always: {always: true} (default false) and then modify the add(https://github.com/sindresorhus/ava/blob/master/lib/test-collection.js#L67) method for adding them to a proper hookset.

[jamestalmage]

I think you are on the right track.

[cgcgbcbc]

Hi @jamestalmage , I've opened a pull request #806, however it leads to some failure in other tests, could you have a look and give some advices? Thank you!

[cgcgbcbc]

@jamestalmage I've fixed the mistake, now it should work as desired

[jamestalmage]

Should an afterEach.always execute if a beforeEach failed? Or should the always only apply if we made it to the actual test function?

[jfmengels]

It should probably execute even if a beforeEach fails I think. Maybe you opened a DB connection or wrote a file, and you want to clean that part up even if something else failed in the beforeEach. You could catch it in the beforeEach I guess, but then you'd duplicate the clean up in both places.

[novemberborn]

Sorry to bring this up again, but should always just be the default behavior? If for some reason your after hook should only run if tests succeed then you can keep track of that yourself. I imagine that'd be rare though.

[jamestalmage]

If for some reason your after hook should only run if tests succeed then you can keep track of that yourself

There are plenty of scenarios where you wouldn't want afterEach running, particularly if you are using mocks and want to call someMock.verify().

should always just be the default behavior?

I am not sure. That certainly makes sense in a serial test runner, where it is very likely you are using afterEach to cleanup some state so it doesn't mess with the next test. For concurrent tests however, there really shouldn't be a whole lot of cleanup todo - you need to avoid messing with state anyways.

[novemberborn]

@jamestalmage in your example, do you mean verifying mocks in your afterEach? Hadn't considered that.

I'm struggling with what to name the "only if there were no failures" after hook. always is easier. I'm just concerned that users may use after for cleanup where they really should have used always.after, but don't notice until a test starts to fail in CI. It's hard to test your tests 😉

[jamestalmage]

Some mock libraries allow you to set up expectations, run your test, and then verify all expectations were actually called. Think of the verification step as the mocking equivalent of t.plan(). One example is nock. They have methods like nock.isDone() (or nock.done() which is the throwing equivalent).

I'm just concerned that users may use after for cleanup where they really should have used always.after

Yeah, that's a potential problem, but I'm not sure how to avoid it.

RE: naming

after and always.after is pretty clear naming once you understand the default behavior of after. I'm not sure of a better naming scheme if we flip the default behavior. afterEach.success?

[novemberborn]

after and always.after is pretty clear naming once you understand the default behavior of after. I'm not sure of a better naming scheme if we flip the default behavior. afterEach.success?

Yea I think I prefer after and always.after.

Given that these run after the normal after hooks, maybe .finally would be better? But maybe that's too strong.

[jamestalmage]

I think finally helps communicate it comes last, don't know that it helps communicate the always part though I think people would catch the try/finally parallel.

I think some users might be confused if they specified .after.always followed by always but they ran in the opposite order. finally would make that clear.