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

Imported jest in mock #13188

Merged
merged 6 commits into from Aug 30, 2022
Merged

Conversation

nicolo-ribaudo
Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo commented Aug 28, 2022

Summary

This PR:

  • Fixes the detection of jest.* refs to imported jest in non-top-level contexts
  • Transforms all the jest.* refs inside hoisted code to the "lazy" version

Fixes #12422

Test plan

Tests added


I have two questions:

  1. This test doesn't make sense to me, since imports are hoisted and thus the jest import is already hoisted, even without any transform:

    import {jest} from '@jest/globals';
    jest.mock('some-module', () => {
      jest.mock('some-module');
    });
               
    _getJestObj().mock('some-module', () => {
      _getJestObj().mock('some-module');
    });
    function _getJestObj() {
      const {jest} = require('@jest/globals');
      _getJestObj = () => jest;
      return jest;
    }
    import {jest} from '@jest/globals';

    What's the reason to transform it?

  2. Can we always transform jest to _getJestObj, so that we don't have to check if it's hoisted or not and we can simplify the plugin?

  3. Since require is cached by default, can we use require('@jest/globals').jest directly instead of _getJestObj()?

@SimenB
Copy link
Member

SimenB commented Aug 29, 2022

1. This test doesn't make sense to me, since imports are hoisted and thus the jest import is already hoisted, even without any transform:

Normally this would also transform the import to require - that transform is left out for brevity (original reproduction shows a more real use case with preset-env: https://github.com/mrazauskas/x-jest-globals/blob/62d5220129a591defce158c2526b3ba49d23d0bb/babel.config.js#L3). The test is mainly that you're allowed to access jest within the mock closure - i.e. buypass the "you cannot access non-global stuff here as it's hoisted" check

2. Can we always transform jest to _getJestObj, so that we don't have to check if it's hoisted or not and we can simplify the plugin?

Yes (I think). Caveat is it should only be hoisted within its function scope, not (necessarily) to the top

3. Since require is cached by default, can we use require('@jest/globals').jest directly instead of _getJestObj()?

Yeah, that should be fine.

@SimenB
Copy link
Member

SimenB commented Aug 29, 2022

@nicolo-ribaudo are there more work you'd want to do here, or is it ready to merge? 🙂

@nicolo-ribaudo
Copy link
Contributor Author

This is ready, but I'll follow up with another PR for those small simplifications :)

@SimenB
Copy link
Member

SimenB commented Aug 30, 2022

Awesome, thanks!

CHANGELOG.md Outdated
- `[jest-config]` [**BREAKING**] Make `snapshotFormat` default to `escapeString: false` and `printBasicPrototype: false` ([#13036](https://github.com/facebook/jest/pull/13036))
- `[jest-config]` [**BREAKING**] Remove undocumented `collectCoverageOnlyFrom` option ([#13156](https://github.com/facebook/jest/pull/13156))
- `[jest-environment-jsdom]` [**BREAKING**] Upgrade to `jsdom@20` ([#13037](https://github.com/facebook/jest/pull/13037), [#13058](https://github.com/facebook/jest/pull/13058))
- `[jest-environment-jsdom]` **BREAKING**] Upgrade to `jsdom@20` ([#13037](https://github.com/facebook/jest/pull/13037), [#13058](https://github.com/facebook/jest/pull/13058))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was probably not intended

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried editting on mobile, crashed chrome 😅 should probably move old releases to a separate changelog

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Aug 30, 2022

Note to self: trying to update a huuuuge changelog on mobile (while getting tattooed) does not work at all 😅

@SimenB SimenB merged commit 835a936 into jestjs:main Aug 30, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the imported-jest-in-mock branch August 30, 2022 15:47
@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 Sep 30, 2022
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.

[Bug]: referencing jest inside jest.mock block throws, if jest is imported from @jest/globals
3 participants