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

feat: allow test environment to run with preset/transform #8751

Merged
merged 4 commits into from
Nov 15, 2020

Conversation

Mark1626
Copy link
Contributor

Summary

Fixes #8479. The exact same approach as #7562.

  • Using the Runtime create an instance of Transformer
  • Hijack the require and perform the transform
  • Remove the hook on require

As of now the logic for the transformation is repeated across jest-cli, jest-runner, jest-runtime packages. Can you give me some inputs on whether it can be extracted to a single location?

Test plan

Existing test cases should not be affected by the change, an additional test where the environment needs to be transformed to pass is added.

@SimenB
Copy link
Member

SimenB commented Jul 25, 2019

Can you give me some inputs on whether it can be extracted to a single location?

Hmm, maybe we could stick some abstraction for this into @jest/transform? E.g. requireAndTranspileModule<T extends string>(moduleName: T, config: Config.ProjectConfig): typeof import(T) which sets up pirates and the transform, requires the module, and cleans up? This is off the top of my head, but do you think that'd work?

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.

Code itself LGTM! Would love to reduce the code duplication as discussed

@SimenB
Copy link
Member

SimenB commented Jul 25, 2019

Don't worry about the OOM for circus on CI btw - it's not your fault

@jeysal jeysal added this to the Jest 25 milestone Jul 26, 2019
Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Code LGTM as well, left a few comments in addition to what Simen said.

e2e/__tests__/transform.test.ts Show resolved Hide resolved
*/

it('should add two numbers', () => {
expect(1 + 1).toBe(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should use something from the env here just to be sure that it was properly loaded

e2e/transform-linked-modules/package.json Show resolved Hide resolved
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.

Oops, forgot to submit

packages/jest-transform/src/ScriptTransformer.ts Outdated Show resolved Hide resolved
packages/jest-runtime/package.json Outdated Show resolved Hide resolved
packages/jest-core/src/runGlobalHook.ts Outdated Show resolved Hide resolved
packages/jest-core/src/runGlobalHook.ts Outdated Show resolved Hide resolved
@SimenB

This comment has been minimized.

@Mark1626

This comment has been minimized.

@SimenB

This comment has been minimized.

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.

This LGTM, needs to wait for us to start merging breaking changes

@Mark1626
Copy link
Contributor Author

@SimenB @jeysal Thanks for your patient reviews, learnt a lot from this PR

@Mark1626

This comment has been minimized.

@MaximeBernard
Copy link

@SimenB Why did the roadmap change from Jest26 to High priority future if the PR is good to go?

@SimenB
Copy link
Member

SimenB commented May 20, 2020

Feedback from FB that this was potentially very breaking. Code wise still good to go, I'll sync with peeps at FB to make sure we can land it in 27 🙂

@michaelgmcd
Copy link

michaelgmcd commented Sep 25, 2020

Hey @SimenB, any update on this? Just want to confirm it's on track for v27.

@SimenB
Copy link
Member

SimenB commented Sep 27, 2020

It is 👍

@Mark1626 Mark1626 force-pushed the feature/transform-env branch from c4d3387 to a48e7e9 Compare November 15, 2020 05:35
@Mark1626
Copy link
Contributor Author

@SimenB I rebased it, I had to squash the commits into a single commit to simplify the merge conflict

@Mark1626 Mark1626 force-pushed the feature/transform-env branch from 4661c8b to ab4f9fb Compare November 15, 2020 12:36
@zzarcon
Copy link

zzarcon commented Mar 24, 2021

Thanks so much for this change folks! Do you know in which version will this be available? thanks!

@SimenB
Copy link
Member

SimenB commented Mar 24, 2021

You can install jest@next to use it now. It'll be available in Jest 27 whenever it stabilizes (possibly over Easter holidays)

@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 10, 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.

allow "testEnvironment" to work together with transform/presets