add after.always and afterEach.always hooks - fixes #474
#806
Conversation
|
By analyzing the blame information on this pull request, we identified @BarryThePenguin, @naptowncode and @sotojuan to be potential reviewers |
cgcgbcbc
changed the title
| }); | ||
| }); | ||
|
|
||
| test('after each not run if serial tests failed', function (t) { |
There was a problem hiding this comment.
after each => afterEach
|
We should add a test verifying what happens with I'm not even sure what that should be! |
|
@jamestalmage I've added tests verifying that |
|
Just FYI, we use the new GitHub "squash and merge" feature on almost every PR, so you don't need to self squash or use commit --amend, etc. It actually makes it harder for me to review just the changes you've made in response to my comments. (Not a big deal, though). |
| // add a always hook | ||
| if (metadata.always) { | ||
| if (type !== 'after' && type !== 'afterEach') { | ||
| throw new Error('"always" cannot be used with a ' + type + ' test'); |
There was a problem hiding this comment.
@cgcgbcbc - Can you add a few tests in test/test-collection.js for this throwing behavior?
|
@cgcgbcbc - You are real close. The implementation looks good to me, we just need a few more tests. 👍 |
|
@jamestalmage Tests added. The |
I don't think that's a problem. |
| @@ -418,9 +418,9 @@ test.todo('will think about writing this later'); | |||
|
|
|||
| AVA lets you register hooks that are run before and after your tests. This allows you to run setup and/or teardown code. | |||
|
|
|||
| `test.before()` registers a hook to be run before the first test in your test file. Similarly `test.after()` registers a hook to be run after the last test. | |||
| `test.before()` registers a hook to be run before the first test in your test file. Similarly `test.after()` registers a hook to be run after the last test. `test.after.always()` registers a hook to be run **always** after the last test, even if before hook/some tests fail. | |||
There was a problem hiding this comment.
How about:
Use
test.after.always()to register an after hook that is called even if tests or other hooks fail.
There was a problem hiding this comment.
Use
test.after.always()to register a hook that will always run once your tests and other hooks complete..always()hooks run regardless of whether there were earlier failures, so they are ideal for cleanup tasks.
|
I think you should check for if (metadata.always && type !== 'after' && type !== 'afterEach') {
throw new Error('"always" can only be used with after and afterEach hooks');
}
if (type !== 'test') {
if (metadata.exclusive) {
throw new Error('"only" cannot be used with a ' + type + ' test');
}
this.hooks[type + (metadata.always ? 'Always' : '')].push(test);
}Would be good to test this error with (Maybe we should change "test" in the "only" message to "hook" too.) @cgcgbcbc almost there! I am wondering though whether this should just be the default behavior. Will discuss further in #474. |
|
@cgcgbcbc - All that's really left here are a few minor nits on the docs, etc. If you can handle those, I think we're good to merge. If anything we've said is ambiguous and you can't get an answer from us right away, just use your best judgement. |
|
@jamestalmage Updated according to the comments |
sindresorhus
changed the title
after.always and afterEach.always hooks - fixes #474
|
Nice work @cgcgbcbc :) LGTM |
|
@jamestalmage should I open an issue in |
|
If one doesn't already exist, yes. |
This pull request aims at #474 .
The new tests added in
test/hooks.jspassed, but these changes lead to other tests' failure.Still working on figuring out why, comments are welcome!