Make test suite exit early if `beforeEach` hook fails (jest-jasmine2) #8654

lucasfcosta · 2019-07-07T15:55:09Z

Summary

This PR fixes #6527 by not running tests inside of a test suite if one of its beforeEach hooks fail.

This is already the default behaviour in jest-circus and has been confirmed to be the desired behaviour as per this comment.

Why this problem happens

Jest jasmine's Env will start building the hierarchy of blocks with children. This means adding the describe blocks to their respective parents to build a tree
If it finds any beforeEach hooks, jest-jasmine2 will add them to an array in the Suite.
Each it (Spec) is added as a child to the describe block they are in.
jest-jasmine2 will start executing tests through the treeProcessor.
For each test (it/Spec) the beforeEach and afterEach hooks will be put into an array in the order they should run and queueRunner will run the functions in the array sequentially.
If any of the functions in the promise chain throws an error, the onException callback will be invoked, but all the following functions will still run because the chain won't be discontinued.

How the fix was implemented

I essentially broke the single chain of promises with all the hooks and the test into two chains. The first chain contains only the beforeEach hooks and the second chain contains the test and the afterEach hooks. Then, if the chain for beforeEach fails I will turn on a flag which will be used to determine whether the continuation (the second chain with tests and afterEach hooks) should run.

Test plan

I have added extensive E2E tests to this to ensure that multiple scenarios would be covered.

I covered the following cases:

For the global context
- Not running tests if a global beforeEach hook fails
- Still running global afterEach hooks if a global beforeEach fails
- Still running global afterAll hooks if a global beforeEach fails
Inside a single suite
- Not running a single test if a beforeEach hook fails
- Not running any tests in the same suite if a beforeEach hook fails
- Trying to run all beforeEach hooks even if one fails
- Still running the afterEach hooks if beforeEach fails
- Still running the afterAll hooks if beforeEach fails
Inside nested suites
- Canceling tests only for the suite the beforeEach hook is in
- Still running the upper afterEach hooks if the nested beforeEach fails
- Still running the upper afterAll hook if the nested beforeEach fails

I don't know if the way I organised these tests is adequate; any ideas on how to improve this are more than welcome.

I thought about writing more tests for more scenarios, but then I came to the conclusion that whatever I could write would end-up being redundant since the basis of them is already covered.

I also made the runAgainstSnapshot function generic since that's what I do in each test anyway.

…js#6527)

lucasfcosta · 2019-07-07T16:29:30Z

e2e/__tests__/beforeEachAbortsTests.test.ts

+    runAgainstSnapshot('singleBlock/multipleTests.test.js');
+  });
+
+  test('runs all of the beforeEach hooks if one fails but does not run the tests', () => {


This test fails in jest-circus because it stops running the beforeEach hooks once one of them fails.

I didn't fix this because I'm not sure if that's the desired behaviour.

IMO if all afterEach and afterAll hooks still run I think that all beforeEach hooks should run too, otherwise the behaviour is not symmetric. Given that beforeAll/beforeEach hooks will usually setup tests and afterAll/afterEach will do clean-up I'd argue that if we're running all the "teardown" part we should run all the "setup" part.

I'm happy to either change the behaviour in jest-circus if you agree with the explanation above or update my PR to make jest-jasmine2 have the same behaviour as jest-circus if you prefer.

beforeEach and afterEach is just sugar for

function beforeEach() { console.log('before'); } function afterEach() { console.log('after'); } function test() { beforeEach(); expect(1).toBe(1); afterEach(); }

(they can be async and Jest waits, but conceptually this is what happens).

I'm not sure we should assign any semantic "setup" and "teardown" meaning to the lifecycle functions. So (IMO) we should just bail if before fails and not run after.

@thymikee @cpojer @scotthovestadt @jeysal @mattphillips do you agree or disagree?

I'm not sure if there's a good solution to that. Both have issues. before may start some listeners before failing and:

they won't be cleared by after causing a leak (may surface in watch mode)

after will try to run after failing before and fail as well, because the functionality expected to be there is not present

Since Jest already has a mechanism to listen for open connections after test run, I'd lean towards @SimenB solution. So don't treat after as a "teardown".

@lucasfcosta Just to make sure I’m understanding correctly , you are saying that if one before hook fails then the rest are ignored but all of the after hooks are still ran?

If this is the case then I agree, we probably do not want to run any after hooks that haven’t had an equivalentbefore hook already ran.

If we do run the after hook without running the equivalent before we can end up blowing up because something the after depends on isn’t there.

I see. I agree with you both. I think that the way @SimenB described it was very interesting to see how the mechanism is actually run but I'd be very careful when translating these concepts into alternative pieces of code as they might become inaccurate. In the excerpt above, for example, the beforeEach failing would demonstrate that a throw occurred and we're out of the normal execution flow, but if the same happened on an afterEach followed by another, there would be a different behaviour.

I think that you are correct especially due to the item 2 mentioned by @thymikee. Even if we treated after as a teardown it should indeed fail if its setup has not been made or was interrupted somehow.

tl;dr I'm convinced that the adequate behaviour should be as both of you preferred: a failing beforeEach should interrupt the next ones in the sequence.

I think the major points have already been mentioned. Side note: The before, after API that we've had even a long time before Jest seems flawed to me because of things like this (and because it does not help structure tests well). The API should look more like React's useEffect, associating a teardown operation with its setup by having it as a return value or second parameter.

Hi @mattphillips, sorry it seems like there was a bit of a race condition in this discussion 😆
I ended up posting without seeing your comment since you submitted after I started writing but before I finished 😅

Please see my last comment in this PR, thanks 😊

lucasfcosta · 2019-07-07T17:42:43Z

e2e/__tests__/__snapshots__/beforeEachAbortsTests.test.ts.snap

@@ -0,0 +1,435 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP


I also considered only using stdout’s console.logs to determine what has been run and what hasn’t, but I considered it was important to check the failures that are actually displayed.

An example of this alternative approach can be found here. It looks very nice and a lot more concise, but I’m not sure it would be the optimal approach for this PR itself.

I’d appreciate any guidance on how to make these more concise/elegant.

SimenB · 2019-07-09T09:56:33Z

Woah, what a great PR @lucasfcosta, thanks!

lucasfcosta · 2019-07-09T19:18:28Z

Thank you for the review, I've had a great time so far exploring the codebase and the repo. I learned a lot so far. Thanks for the great work @SimenB @thymikee and all the team 💖

I'll address the point you mentioned about interrupting the beforeEach sequence as soon as I can.

lucasfcosta · 2019-07-10T19:31:59Z

Hello, everyone 😊

I was thinking about this on my way back home today, and I actually think that I'd prefer to go with the current approach and change jest-circus instead because:

Reason 1

The current execution flow for each test is beforeEach[] -> test -> afterEach[]
If any of the hooks or the test fails, the subsequent "tasks" will still run
Changing the current behaviour of jest-jasmine2 so that the beforeEach hooks will stop running if one of them fails would be a more unexpected change. That's very different from what currently happens and what people have requested is just that the test won't run if a beforeEach hook fails, which will be a goal we accomplish with this PR as is.

Reason 2

So far the vast majority of users uses jest-jasmine2 so I think that it would be better to adapt jest-circus to have a behaviour which is more similar to jest-jasmine2 in order to facilitate the transition than the other way around.

Reason 3

Semantically speaking, it still seems to me that the order in which beforeEach hooks are declared shouldn't matter since they are "hooks" and therefore should be "atomic".
Following the same logic, one could argue that we should also fail the afterEach sequence if one of the previous afterEach fails. I think the behaviour of these two hooks should be consistent.

I definitely buy @thymikee's point that afterEach is not really a "teardown", but due to the reasons above I think that going with the current approach would be more desirable.

I don't think there's a solution which is clearly better here, but I think that going with the less risky and drastic one would be desirable (especially considering jest-circus is not yet the default runner).

TL;DR: Keeping it as is would be a change which is less different from current behaviour but still addresses user's needs and since the vast majority of users is still in jest-jasmine2 I think that it's better to change jest-circus instead to facilitate the transition.

SimenB · 2019-07-11T08:26:08Z

Thanks for the detailed writeup!

Reason 1

Not sure I follow the reasoning. Just because the behavior will be different doesn't mean it'll be wrong or worse. How it works currently shouldn't trump any design decisions we make if we think it'll improve clarity of test's lifecycles.

Reason 2

I disagree that Jasmine's behavior should "win" just because it's the current default behavior. We should figure out what behavior is desirable, regardless of what's currently in either Circus or Jasmine, and implement that in Circus. At that point we can figure out what makes sense to also implement in Jasmine.

Reason 3

While I agree they should be atomic, I can guarantee you that a lot of the lifecycle methods people define are not. And yes, I also think we should bail on subsequent afterEach if one fails. I find multiple lifecycle methods to be confusing to reason about, but it is what it is 🙂

I really like @jeysal's idea in a comment above here, with co-locating setup and teardown. I think that'd better encourage correct usage. We could possibly pass some sort of test state into the teardown so it could know if everything went well or if something went wrong? This PR is not really the place to discuss it, but I think it'd be super interesting to explore an alternative lifecycle API.

(random idea to end this rambling response with)

const testWithSetup = jest.setup(
  async () => {
    const context = await fetchData();

    return context;
  },
  context => {
    // do some teardown here, if you want
  },
);

testWithSetup('title', context => {
  expect(context.prop).toBeDefined();
});

And maybe a setupOnce or something that would only run the provided function once, instead of once per test.

lucasfcosta · 2019-07-11T20:38:51Z

Hi @SimenB, thank you for taking the time to write such a well thought and detailed answer too 💖

I agree with what you said, but I should've been more explicitly about the fact that my argument is focused on getting what's the best solution for now, and have a separate discussion, in length, about what the adequate behaviour should be.

Since this PR solves the most acute problem described in the issue, I think it should be the least drastic as possible so that we can get the problem solved in the meantime while we discuss a better overall approach. I also quite like what @jeysal proposed, but we would also need to discuss that separately.

To address a few of the things you mention specifically:

How it works currently shouldn't trump any design decisions we make if we think it'll improve clarity of test's lifecycles.

I 100% agree with this, but I think that running all the beforeEach hooks is what's more expected by the users at the moment since it's more similar to the current behaviour. We should, of course, discuss what the future of hooks will be, but that's going to require more time and maybe a mechanism which that's very different from what we currently have. Thus I'd advocate for discussing it separately (I'm happy to open an issue for that and summarise the discussion we've had in this PR so far).

I disagree that Jasmine's behavior should "win" just because it's the current default behavior.

I also agree with this. The behaviour which people agree is the more desirable should win, regardless of which runner it's in, but at this moment I reckon it's more valuable to have a fix which is as similar to the current approach as possible to keep users happy while we think about better alternatives.

While I agree they should be atomic, I can guarantee you that a lot of the lifecycle methods people define are not. And yes, I also think we should bail on subsequent afterEach if one fails. I find multiple lifecycle methods to be confusing to reason about, but it is what it is

I also feel very inclined to say the same, and I think your reasoning is 100% correct, but I think it needs further discussion so that we can consider all the minutiae involved and maybe solving this problem will require a completely different approach as we've mentioned.

@SimenB sorry I should've made more clear my point about this being more related to the current context than to what the final behaviour should be. Thank you very much for the quality feedback. Please let me know what you think about the points above.

tl;dr: I don't think we can get to a solution which will address the semantics of hooks as a whole without further discussion in a separate issue and therefore I think we should go with the simplest approach and which includes the least drastic changes while we discuss what the final desired behaviour should be.

@ all

Given this, the approach I'd advocate for would be:

Open a separate issue to evaluate the behaviour of hooks and when their execution should be interrupted (in it we can also explore a bit of @jeysal's idea and come up with a separate proposal once we know what user's pain points are)
Update jest-circus behaviour to conform with the new jest-jasmine2 behaviour

Please let me know what's preferred. I, of course, don't mind if you disagree with the approach described above and I'd be happy to implement the changes you think are the most desirable.

I'm looking forward to hearing from you.

Thanks everyone for your kindness and such great input and discussion ✨

Best,
Lucas

lucasfcosta · 2019-07-27T12:43:09Z

Hello everyone,

I know this has been a long discussion and that you are all very busy especially given all that happens in Jest, but I just thought I'd ping some of you to check whether there's anything I can do to push this forward. If there's anything I can help you with please just let me know and I'll do my best to answer ASAP.

I'd still advocate for this approach, but I'm happy to change the behaviour of jest-jasmine2 instead in case that's what you prefer.

I'd be happy to follow what you prefer because I think that the most important thing to do atm is getting #6527 fixed as it seems to have a reasonable impact on many users.

All the best,
Lucas

jeysal · 2019-07-28T22:10:07Z

I'm with @lucasfcosta on the better short-term solution. The after hook failing because the before hook didn't perform setup correctly doesn't seem so bad (user can probably identify the causation), but potential confusion why hooks don't run or missing out on cleanup causing leaks does.

lucasfcosta · 2019-08-10T19:06:52Z

@jeysal I'm then going to proceed with changing jest-circus so that we can push this forward. We may want to revert this in the future, but, in the meantime, I think it's better to get the fix in and have consistent behaviour.

We can then decide later what's the actual behaviour which we will implement for both runners.

If anyone disagrees with this please let me know.

Thank you all 😊

fungiboletus · 2020-04-22T13:12:41Z

Hello, what is missing to get this merged ?

lucasfcosta · 2020-04-24T16:42:59Z

Hi @fungiboletus, in the past few months I've been writing a book and I've been quite busy with other professional matters, so I doubt I'll be able to push this forward before September. I'd be happy to introduce others to the code and explain my concerns on this PR so that they can continue the work.
I'm sorry for that.
If anyone's interested in pushing this forward, please send me an email at opensource@lucasfcosta.com and we can book 30-60 minutes to go through this.

github-actions · 2022-09-08T18:56:52Z

This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions · 2022-10-08T19:05:41Z

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

piranna · 2022-10-09T04:03:32Z

What's missing to get this merged?

SimenB · 2022-10-09T07:01:43Z

Dunno, haven't looked at it in a loooong time 😅

github-actions · 2023-10-09T07:04:31Z

This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions · 2023-11-08T08:04:57Z

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

github-actions · 2023-12-09T00:08:13Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

facebook-github-bot added the cla signed label Jul 7, 2019

lucasfcosta force-pushed the beforeEach-aborts-tests branch 2 times, most recently from f8c024f to 58c95c0 Compare July 7, 2019 16:00

fix: make test suite exit early if beforeEach hook fails (closes jest…

0b69ab0

…js#6527)

lucasfcosta force-pushed the beforeEach-aborts-tests branch from 58c95c0 to 0b69ab0 Compare July 7, 2019 16:01

lucasfcosta commented Jul 7, 2019

View reviewed changes

lucasfcosta mentioned this pull request Jul 14, 2019

beforeAll async hook hides errors #8688

Closed

jeysal mentioned this pull request Jul 29, 2019

Feature: add aroundAll, aroundEach funcitons #4543

Closed

This was referenced Apr 6, 2020

Only the first test case respects setting jest.setTimeout() playwright-community/jest-playwright#100

Closed

When tests are stopped by timeout, pending promises continue execution #9685

Closed

snyk-bot mentioned this pull request Jul 17, 2020

[Snyk] Security upgrade eslint from 3.19.0 to 4.0.0 joseroubert08/jest#34

Open

jeysal mentioned this pull request Sep 27, 2020

Return cleanup functions from beforeAll and beforeEach #10453

Closed

daneryl mentioned this pull request Mar 29, 2021

Research if failed BeforEach jest tests can prevent running the later specs huridocs/uwazi#2937

Closed

lucasfcosta mentioned this pull request Jan 10, 2022

Add summary to journeys which don't emit journey:end (early node subprocess exits) elastic/beats#29606

Merged

10 tasks

snyk-bot mentioned this pull request Jan 12, 2022

[Snyk] Security upgrade eslint from 3.19.0 to 4.0.0 joseroubert08/jest#76

Open

github-actions bot added the Stale label Sep 8, 2022

SimenB mentioned this pull request Sep 9, 2022

[Feature]: Extensible, type-friendly and explicit test/expect API #13209

Closed

github-actions bot closed this Oct 8, 2022

SimenB reopened this Oct 9, 2022

SimenB removed the Stale label Oct 9, 2022

github-actions bot added the Stale label Oct 9, 2023

github-actions bot closed this Nov 8, 2023

github-actions bot locked as resolved and limited conversation to collaborators Dec 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make test suite exit early if `beforeEach` hook fails (jest-jasmine2) #8654

Make test suite exit early if `beforeEach` hook fails (jest-jasmine2) #8654

lucasfcosta commented Jul 7, 2019

lucasfcosta Jul 7, 2019

SimenB Jul 9, 2019

thymikee Jul 9, 2019 •

edited

Loading

mattphillips Jul 9, 2019

lucasfcosta Jul 9, 2019 •

edited

Loading

jeysal Jul 10, 2019

lucasfcosta Jul 10, 2019

lucasfcosta Jul 7, 2019 •

edited

Loading

SimenB commented Jul 9, 2019

lucasfcosta commented Jul 9, 2019 •

edited

Loading

lucasfcosta commented Jul 10, 2019

SimenB commented Jul 11, 2019

lucasfcosta commented Jul 11, 2019 •

edited

Loading

lucasfcosta commented Jul 27, 2019

jeysal commented Jul 28, 2019

lucasfcosta commented Aug 10, 2019

fungiboletus commented Apr 22, 2020

lucasfcosta commented Apr 24, 2020

github-actions bot commented Sep 8, 2022

github-actions bot commented Oct 8, 2022

piranna commented Oct 9, 2022

SimenB commented Oct 9, 2022

github-actions bot commented Oct 9, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Dec 9, 2023

Make test suite exit early if beforeEach hook fails (jest-jasmine2) #8654

Make test suite exit early if beforeEach hook fails (jest-jasmine2) #8654

Conversation

lucasfcosta commented Jul 7, 2019

Summary

Why this problem happens

How the fix was implemented

Test plan

lucasfcosta Jul 7, 2019

Choose a reason for hiding this comment

SimenB Jul 9, 2019

Choose a reason for hiding this comment

thymikee Jul 9, 2019 • edited Loading

Choose a reason for hiding this comment

mattphillips Jul 9, 2019

Choose a reason for hiding this comment

lucasfcosta Jul 9, 2019 • edited Loading

Choose a reason for hiding this comment

jeysal Jul 10, 2019

Choose a reason for hiding this comment

lucasfcosta Jul 10, 2019

Choose a reason for hiding this comment

lucasfcosta Jul 7, 2019 • edited Loading

Choose a reason for hiding this comment

SimenB commented Jul 9, 2019

lucasfcosta commented Jul 9, 2019 • edited Loading

lucasfcosta commented Jul 10, 2019

Reason 1

Reason 2

Reason 3

SimenB commented Jul 11, 2019

lucasfcosta commented Jul 11, 2019 • edited Loading

lucasfcosta commented Jul 27, 2019

jeysal commented Jul 28, 2019

lucasfcosta commented Aug 10, 2019

fungiboletus commented Apr 22, 2020

lucasfcosta commented Apr 24, 2020

github-actions bot commented Sep 8, 2022

github-actions bot commented Oct 8, 2022

piranna commented Oct 9, 2022

SimenB commented Oct 9, 2022

github-actions bot commented Oct 9, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Dec 9, 2023

Make test suite exit early if `beforeEach` hook fails (jest-jasmine2) #8654

Make test suite exit early if `beforeEach` hook fails (jest-jasmine2) #8654

thymikee Jul 9, 2019 •

edited

Loading

lucasfcosta Jul 9, 2019 •

edited

Loading

lucasfcosta Jul 7, 2019 •

edited

Loading

lucasfcosta commented Jul 9, 2019 •

edited

Loading

lucasfcosta commented Jul 11, 2019 •

edited

Loading