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

fix: vars starting with 'mock' in hoisted context #3400

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

smbkr
Copy link

@smbkr smbkr commented Apr 6, 2022

Summary

This PR attempts to address #3292, where variable whose name starts with mock cannot be used in a hoisted mock context.

In plain jest and jest+babel, one can reference a variable that is not yet initialised when creating a mock which jest will hoist by starting the name with mock, e.g. mockMyFoo. Presently this throws a ReferenceError when using ts-jest.

Test plan

So far I've added an e2e test that demonstrates this bug.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@smbkr
Copy link
Author

smbkr commented Apr 6, 2022

Opening this as a draft as I'd like to help here, but unsure how to proceed. So far, I've written a failing e2e test to demonstrate this bug.

Initially I started looking at src/transformers/hoist-jest.ts after some guidance from @ahnpnl on the open issue, however the test src/transformers/hoist-jest.spec.ts already checks that variables starting with mock are not causing issues when transforming due to being out of scope. See lines 151:153.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Apr 6, 2022

The unit test data was wrong for MockMethods. It is just a constant and we didn’t put any jest.mock which uses that constant. We copied the test data from Jest which misses this line https://github.com/facebook/jest/blob/777e32687775b9287650c85ade4f9fdda1405fe1/e2e/babel-plugin-jest-hoist/__tests__/integration.test.js#L79

Also e2e test data we reuse the same test data Jest has to keep consistency. If you update e2e test, you should take from this https://github.com/facebook/jest/blob/main/e2e/babel-plugin-jest-hoist/__tests__/integration.test.js

if I’m not wrong in e2e we have copy 1-1 from jest for test data.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Apr 6, 2022

There is one solution I can think of is: including the variables starting with mock into the list of statements to sort, see this

const sortStatements = (statements: _ts.Statement[]): _ts.Statement[] => {

If we do that way, we can ensure those variables will be always above jest.mock. The normal order for this case would be:

  • variables starting with mock comes from import statement
  • jest mock stuffs
  • import or require statements

However, if those constants come from import or require statements, we need to ensure a different order:

  • jest mock other stuffs
  • import or require statements
  • variables starting with mock comes from import or require statements
  • jest mock apis

@smbkr
Copy link
Author

smbkr commented Apr 8, 2022

Thanks @ahnpnl, I'm planning to continue looking at this when time permits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants