Jest: #5585 breaks pre-established "beforeEach" order

[mjesun]

When introduced #5885, the order of beforeEachs was inverted. Take the following test for instance:

describe('test', () => {
  beforeEach(() => {
    console.log('BeforeEach');
  });

  it('foo', () => {
    console.log('It Foo');

    beforeEach(() => {
      console.log('BeforeEach Inline Foo');
    });
  });

  it('bar', () => {
    console.log('It Bar');

    beforeEach(() => {
      console.log('BeforeEach Inline Bar');
    });
  });
});

The order should be:

It Foo
BeforeEach Inline Foo
BeforeEach
It Bar

But it is instead:

It Foo
BeforeEach
BeforeEach Inline Foo
It Bar

One might argue about beforeEachs inside its, but the reality is that this is supported and the current order is altered. We need to get some sort of fix or rather revert the PR.

[SimenB]

Why should we support beforeEach inside of tests? Seems super wonky. Is this a case of "we're doing it in thousands of tests at FB, so can't change it"? If so, is it possible to set up a codemod fixing it? If anything, I'd expect it to break completely and not be supported. The fact ordering changed for something that's not documented shouldn't be cause for revert, IMO.

Also, where is BeforeEach Inline Bar? the whole thing seems buggy :P

/cc @niieani

[mjesun]

Well, we've altered the process of adding on-the-fly beforeEachs into the queue. As explained, it is arguable that this is or isn't a desired thing, but it's currently supported and broken, so we should fix it.

BeforeEach Inline Bar does not appear because it would be a beforeEach executed after the following test case. Since bar is the last one, it will never get called.

[mjesun]

On a side note, I published 23.0.0-alpha.5r, which is a clone of alpha.5, but with #5885 reverted, so that I could unblock myself internally.

[mjesun]

On another side note: this happens with inlined requires; I just oversimplified the code to give a simple test case; but real-life scenarios are way more complex than this.

[niieani]

See my review that identifies the change here: https://github.com/facebook/jest/pull/5885#pullrequestreview-111380249

Changing the order consistently would require moving line 64 between 59 and 60:

https://github.com/facebook/jest/blob/95f7813a8cc8f9988da2a9727bd7f8845348edfc/packages/jest-jasmine2/src/tree_processor.js#L59-L70

We could also parametrize it for the top-suite case so its evaluated early, but I don't think keeping inconsistent behavior is the way to go.

Please advise on how to continue.

[rickhanlonii]

it's currently supported and broken, so we should fix it

@mjesun is it broken in the 22.x range or is it only in the 23 alpha?

[niieani]

@rickhanlonii this is in the latest alpha only.

[mjesun]

I'm OK with moving the wrapChildren to the outer scope; I can also perform a quick test with your change internally and see if everything results in the expected behavior. 🙂

We could also parametrize it for the top-suite case so its evaluated early, but I don't think keeping inconsistent behavior is the way to go.

No, you are right, whatever the behavior is, it should always be the same.

[mjesun]

@niieani any updates regarding the change? We're going to release alpha.7 soon and it'd be nice to include that fix 🙂

[niieani]

Hey @mjesun, I think we misunderstood each other 😄

I was waiting for your quick test:

I can also perform a quick test with your change internally and see if everything results in the expected behavior

Did you try and see if that change works as intended?

[mjesun]

Oh, I was waiting for the change 😅I need to merge and create alpha.7, then I can test it internally. I agree it was confusing 🙂

[mjesun]

So... are you finally doing the fix? I need to revert if not :(

[niieani]

@mjesun Oh man, I thought testing it internally meant you'll do it ;) So once again we misunderstood each other. Here's a PR: https://github.com/facebook/jest/pull/6006

Looking forward to seeing it that fixes the issue.

[mjesun]

Thanks for the fix! 🙂