Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert changes on the TreeProcessor (#6006 and #5885) #6105

Merged
merged 1 commit into from
May 1, 2018
Merged

Revert changes on the TreeProcessor (#6006 and #5885) #6105

merged 1 commit into from
May 1, 2018

Conversation

mjesun
Copy link
Contributor

@mjesun mjesun commented May 1, 2018

The TreeProcessor refactor introduced a change on the behavior of beforeEach inside it. While this is an edge case, Facebook (and potentially anyone using inline requires) is relying in it.

Some fixes have been suggested, but for the moment we'll just revert the change. cc / @niieani

@mjesun mjesun merged commit 6640903 into jestjs:master May 1, 2018
@mjesun mjesun deleted the revert branch May 1, 2018 21:58
@niieani
Copy link
Contributor

niieani commented May 1, 2018

@mjesun this will break webpack. It had just merged the huge PR migrating its tests to jest.

I have identified the line which was causing the issue and fixed it here: #6100. Was this fix not enough or is there a different reason for doing the revert?

@SimenB
Copy link
Member

SimenB commented May 2, 2018

+1 for #6100 instead. The order was undocumented and unintuitive before the changes. An option seems like an acceptable workaround to me, until FB is able to fix their tests

@SimenB
Copy link
Member

SimenB commented May 2, 2018

and potentially anyone using inline requires

How does inline requires impact this? Jest itself uses inline requires with no issues before or after this change

@mjesun
Copy link
Contributor Author

mjesun commented May 2, 2018

Our internal transformer uses inline requires over test code. Some tests are using a helper utility file, which contains:

beforeEach(() => {
  jest.resetModules();
});

The require for this file is at the top level, but because of inline-requiring, the first time the file is loaded it happens inside an it. Think about the following scenario:

const myHelper = require('./my-helper');
const foo = require('foo');

beforeEach(() => {
  jest.mock('foo', () => {});
});

it('does magic', () => {
  myHelper.createAnotherStuff();
  foo.bar();
});

This test case passes before the change to the tree processor, but breaks now. The reason is that before the change, the order of the beforeEachs was:

  • First the jest.resetModules() inside the beforeEach of the helper function.
  • Then the jest.mock inside the beforeEach of the test.

With the change to the tree processor, the new order is:

  • First the jest.mock inside the beforeEach of the test.
  • Then the jest.resetModules() inside the beforeEach of the helper function.

Which essentially means that the later resetModules kills all pre-initialization. This will affect anyone using inline requires with helper files.

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants