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

Implement support for transforming snapshot resolvers #8829

Merged
merged 7 commits into from
Apr 22, 2021
Merged

Implement support for transforming snapshot resolvers #8829

merged 7 commits into from
Apr 22, 2021

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Aug 15, 2019

Summary

Allows snapshotResolvers to be transformed, letting you use TypeScript, Flow, etc in your resolvers.

Test plan

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

Closes #8330
Supports #8810

@G-Rath
Copy link
Contributor Author

G-Rath commented Aug 15, 2019

@M4rk9696 @SimenB I have no idea why this isn't working.

I've done the same implementation for the tests as in #8751 & #8823, but for some reason it errors out 😕

(I'm half hoping it's a Windows/WSL thing)

g-rath@CINDY:/c/Users/G-Rath/workspace/projects-oss/jest/e2e/transform/transform-snapshotResolver$ node ../../../packages/jest-cli/bin/jest.js
 FAIL  __tests__/add.test.js
  ● Test suite failed to run

    Jest encountered an unexpected token

    This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.

    By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules".

    Here's what you can do:
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/en/configuration.html

    Details:

    /c/Users/G-Rath/workspace/projects-oss/jest/e2e/transform/transform-snapshotResolver/snapshotResolver.ts:8
    import {SnapshotResolver} from 'jest-snapshot';
           ^

    SyntaxError: Unexpected token {

      at ScriptTransformer._transformAndBuildScript (../../../packages/jest-transform/build/ScriptTransformer.js:537:17)

TypeError: _jestHasteMap(...).default.getCacheFilePath is not a function
    at ScriptTransformer._getFileCachePath (/c/Users/G-Rath/workspace/projects-oss/jest/packages/jest-transform/build/ScriptTransformer.js:287:50)
    at ScriptTransformer.transformSource (/c/Users/G-Rath/workspace/projects-oss/jest/packages/jest-transform/build/ScriptTransformer.js:421:32)
    at revertHook (/c/Users/G-Rath/workspace/projects-oss/jest/packages/jest-transform/build/ScriptTransformer.js:622:23)
    at Module._compile (/c/Users/G-Rath/workspace/projects-oss/jest/node_modules/pirates/lib/index.js:93:29)
    at Module._extensions..js (internal/modules/cjs/loader.js:787:10)
    at newLoader (/c/Users/G-Rath/workspace/projects-oss/jest/node_modules/pirates/lib/index.js:104:7)
    at Object.newLoader [as .ts] (/c/Users/G-Rath/workspace/projects-oss/jest/node_modules/pirates/lib/index.js:104:7)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Module.require (internal/modules/cjs/loader.js:690:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at ScriptTransformer.requireAndTranspileModule (/c/Users/G-Rath/workspace/projects-oss/jest/packages/jest-transform/build/ScriptTransformer.js:641:20)
    at createCustomSnapshotResolver (/c/Users/G-Rath/workspace/projects-oss/jest/packages/jest-snapshot/build/snapshot_resolver.js:78:17)
    at createSnapshotResolver (/c/Users/G-Rath/workspace/projects-oss/jest/packages/jest-snapshot/build/snapshot_resolver.js:50:7)
    at Object.buildSnapshotResolver (/c/Users/G-Rath/workspace/projects-oss/jest/packages/jest-snapshot/build/snapshot_resolver.js:40:20)
    at contexts.forEach.context (/c/Users/G-Rath/workspace/projects-oss/jest/packages/jest-core/build/TestScheduler.js:311:37)
    at Set.forEach (<anonymous>)
    at updateSnapshotState (/c/Users/G-Rath/workspace/projects-oss/jest/packages/jest-core/build/TestScheduler.js:307:18)
    at /c/Users/G-Rath/workspace/projects-oss/jest/packages/jest-core/build/TestScheduler.js:371:7
    at Generator.next (<anonymous>)
    at asyncGeneratorStep (/c/Users/G-Rath/workspace/projects-oss/jest/packages/jest-core/build/TestScheduler.js:131:24)
    at _next (/c/Users/G-Rath/workspace/projects-oss/jest/packages/jest-core/build/TestScheduler.js:151:9)
    at process._tickCallback (internal/process/next_tick.js:68:7)

 RUNS  __tests__/add.test.js

@Mark1626
Copy link
Contributor

@G-Rath Something which I do for debugging, is to check the cache to see what was the output of the transform. You can link jest-cli to a sample project, set cacheDirectory to a local path and see what's inside it

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.

Please update the changelog

e2e/transform/transform-snapshotResolver/package.json Outdated Show resolved Hide resolved
e2e/transform/transform-snapshotResolver/package.json Outdated Show resolved Hide resolved
packages/jest-snapshot/package.json Outdated Show resolved Hide resolved
packages/jest-snapshot/src/snapshot_resolver.ts Outdated Show resolved Hide resolved
packages/jest-snapshot/src/snapshot_resolver.ts Outdated Show resolved Hide resolved
@G-Rath
Copy link
Contributor Author

G-Rath commented Aug 15, 2019

@SimenB thanks, but this wasn't actually ready for review which is why I made it a draft PR :)

Right now I'm trying to get it just working, before refactoring, which is what I could use your assistance with.

@SimenB
Copy link
Member

SimenB commented Aug 15, 2019

It fails since snapshots runs inside the sandbox, and snapshotResolver is marked as an internal module. isInternalModule is true, which means willTransform is false: https://github.com/facebook/jest/blob/3ab2fc1b0ae3542bb8cdc927bf5459ba16a6420c/packages/jest-transform/src/ScriptTransformer.ts#L349-L352. It's set to internal since we come from https://github.com/facebook/jest/blob/3ab2fc1b0ae3542bb8cdc927bf5459ba16a6420c/packages/jest-jasmine2/src/setup_jest_globals.ts#L104 via https://github.com/facebook/jest/blob/3ab2fc1b0ae3542bb8cdc927bf5459ba16a6420c/packages/jest-jasmine2/src/index.ts#L144. I'm actually not sure how to best solve this...

@thymikee @jeysal thoughts/ideas?

@jeysal
Copy link
Contributor

jeysal commented Aug 15, 2019

There's already a localRequire passed to setup_jest_globals, which is effectively Runtime.requireModule, not Runtime.requireInternalModule. Calling that should break the cascade of internal module loading. This is already used for snapshot serializers. If we can initialize snapshot resolvers the same way, that should work I think.

@G-Rath
Copy link
Contributor Author

G-Rath commented Aug 15, 2019

I'm finding the jest codebase too overwhelming to figure out where the correct place to call Runtime.requireModule & transform the file.

Would you guys be able to expand on the diff you'd expect for this?

In particular:

This is already used for snapshot serializers.

From what I've been able to find, localRequire itself handles the transformation - is that correct? if not, where is the transformation for snapshot serializers actually happening?

If we can initialize snapshot resolvers the same way

What would you expect this to look like, and where? (both roughly)

@SimenB
Copy link
Member

SimenB commented Aug 16, 2019

It's my last code link, the .requireInternalModule(path.resolve(__dirname, './setup_jest_globals.js')) one. I think that's what @jeysal alluded to, but I'm on mobile so hard to verify

@jeysal
Copy link
Contributor

jeysal commented Aug 16, 2019

@G-Rath I think we'd need to localRequire the resolver in setup_jest_globals and pass that to buildSnapshotResolver instead of just a path. So yes it is a slightly larger refactoring.

@G-Rath
Copy link
Contributor Author

G-Rath commented Aug 17, 2019

I managed to cobble something together that seems to work 😄

@SimenB @jeysal would you guys mind giving it a once over and advising on the state of my implementation?

I'll look to mark it as "Ready for Review" tomorrow morning, but this time you're welcome to review before that ;)

Ignore the fact that there's no changelog: I'll do that tomorrow when I'm up.

The primary improvement that springs to mind is breaking the anonymous function into a util or static method:

static requireAndTranspileModule(config, path) {
  return new ScriptTransformer(config).requireAndTranspileModule(path);
}

However, something tells me you guys probably will be against that, so I've left it as is for now.

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.

Awesome that you made it work with just a small hint! Left a few comments on various parts of the diff.

e2e/transform/transform-snapshotResolver/babel.config.js Outdated Show resolved Hide resolved
e2e/transform/transform-snapshotResolver/tsconfig.json Outdated Show resolved Hide resolved
packages/jest-core/src/SearchSource.ts Outdated Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Aug 18, 2019

The primary improvement that springs to mind is breaking the anonymous function into a util or static method:

I'm down with that. 🙂 Or just a separate export from the default one? Mind sending a separate PR for that? We can land that now, while starting to transform the snapshot resolver is a breaking change so needs to wait a bit before landing

@G-Rath
Copy link
Contributor Author

G-Rath commented Aug 18, 2019

I'm down with that. 🙂 Or just a separate export from the default one? Mind sending a separate PR for that?

I've actually already done that 😄

@G-Rath
Copy link
Contributor Author

G-Rath commented Aug 18, 2019

Also, I'm not sure why the CI is failing now?

It seems to run fine but then just exists w/ error code 1 😕

As far as I know yarn shouldn't know this is a draft PR, which is the only reason I could think of it just bailing after everything passes, unless something else was going drastically wrong.

@jeysal
Copy link
Contributor

jeysal commented Aug 20, 2019

It seems to run fine but then just exists w/ error code 1

Wow that is very weird indeed, I haven't seen that happen before. Can't find anything in the diff that could cause such behavior. Started happening with 3950754 but might have previously just been hidden because there were proper errors.

@G-Rath
Copy link
Contributor Author

G-Rath commented Aug 20, 2019

Wow that is very weird indeed, I haven't seen that happen before

Just what I wanted to hear 😂

Started happening with 3950754 but might have previously just been hidden because there were proper errors.

That should be reverted, which I force-pushed. I'll have a play around to see if I've miss something, just in case.

might have previously just been hidden because there were proper errors.

I think that is the case - I might see if I can isolate what's going on via hacky ignores & such. Would it be alright if I opened up a PR to test the CI, rather than revert around things on here?

I think this PR is ready for review anyway - the unresolved comments sound like they can be resolved, so all that's left is to decide how we're going to handle the changelog.

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.

Would it be alright if I opened up a PR to test the CI

Sure. You can also enable CircleCI for your fork of Jest, although people have had problems in the past with Circle deciding to run the actual PRs on the forked repo as well after doing that and I'm not sure if that's fixed yet.

packages/jest-jasmine2/src/setup_jest_globals.ts Outdated Show resolved Hide resolved
@G-Rath G-Rath marked this pull request as ready for review October 26, 2019 09:26
@G-Rath
Copy link
Contributor Author

G-Rath commented Oct 26, 2019

@SimenB if you have some spare time after you're done dancing w/ Danger, I have no idea why this is failing 😭

Other than that, iirc it's good to go

@G-Rath G-Rath requested review from SimenB and jeysal October 26, 2019 09:33
@G-Rath
Copy link
Contributor Author

G-Rath commented Mar 15, 2021

So packages/jest-core/src/TestScheduler.ts has:

        const status = snapshot.cleanup(
          context.hasteFS,
          this._globalConfig.updateSnapshot,
          snapshot.buildSnapshotResolver(context.config),
          context.config.testPathIgnorePatterns,
        );

Which is fine, except it doesn't have a localRequire variable - that in theory is also fine, except I don't know all the details around localRequire to figure out what the right thing to pass in here is; it could be fine to just pass in require? 🤔

Both jest-jasmine2 & jest-circus get localRequire as a config property 😬

@G-Rath
Copy link
Contributor Author

G-Rath commented Mar 15, 2021

So createTranspilingRequire has become an async function, but this isn't async so probably going to be best to no longer use that as the default value.

@G-Rath
Copy link
Contributor Author

G-Rath commented Mar 15, 2021

@SimenB feel free to jump in if you've got any thoughts or comments - for now I'm just chipping away at the code seeing if I can get tests passing by blindly making things async.

buildSnapshotResolver is also being used in SearchSource, in a completely sync way so I think there might be some discussion required on the best way forward.

@G-Rath
Copy link
Contributor Author

G-Rath commented Mar 15, 2021

@SimenB so I'm thinking that for this to be possible buildSnapshotResolver will need to become async - does that sound ok to you?

I think the majority of call sites are already async, except for the aforementioned SearchSource, which'll also need to become async.

@SimenB
Copy link
Member

SimenB commented Mar 16, 2021

@SimenB so I'm thinking that for this to be possible buildSnapshotResolver will need to become async - does that sound ok to you?

yep 👍 Lots of "load this thing" will become async as it's needed for import() (i.e. ESM) 👍

@G-Rath
Copy link
Contributor Author

G-Rath commented Mar 17, 2021

Run yarn --immutable
➤ YN0000: ┌ Resolution step
Resolution step
➤ YN0000: └ Completed in 0s 639ms
➤ YN0000: ┌ Fetch step
Fetch step
➤ YN0000: └ Completed in 1s 594ms
➤ YN0000: ┌ Link step
Link step
➤ YN0000: └ Completed in 1m 20s
➤ YN0000: Failed with errors in 1m 23s
Error: Process completed with exit code 1.

well, that's fun - would be nice to know what the errors were😕

@G-Rath
Copy link
Contributor Author

G-Rath commented Mar 17, 2021

@SimenB this looks good for review - the two CI failings seem to be flakeness: Windows is failing because of the above (yarn is just erroring at install with no actual message), and the macOS fail is about a file being missing with "Runtime CLI › throws script errors"; I suspect if you re-run these checks it'll pass (or at least give a different result).

(this also gave me a new possible way we could use types in eslint-plugin-jest; checking for Promises)

@G-Rath G-Rath requested a review from SimenB March 19, 2021 02:58
@G-Rath
Copy link
Contributor Author

G-Rath commented Mar 30, 2021

The Windows build is failing due to not being able to compile node-sass apparently 🤔

@codecov-io
Copy link

codecov-io commented Mar 30, 2021

Codecov Report

Merging #8829 (09033d8) into master (4926564) will increase coverage by 0.01%.
The diff coverage is 63.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8829      +/-   ##
==========================================
+ Coverage   64.23%   64.24%   +0.01%     
==========================================
  Files         308      308              
  Lines       13512    13514       +2     
  Branches     3292     3293       +1     
==========================================
+ Hits         8679     8682       +3     
+ Misses       4120     4119       -1     
  Partials      713      713              
Impacted Files Coverage Δ
...us/src/legacy-code-todo-rewrite/jestAdapterInit.ts 0.00% <0.00%> (ø)
packages/jest-core/src/runJest.ts 51.19% <0.00%> (+0.60%) ⬆️
packages/jest-jasmine2/src/index.ts 0.00% <0.00%> (ø)
packages/jest-jasmine2/src/setup_jest_globals.ts 0.00% <0.00%> (ø)
packages/jest-core/src/SearchSource.ts 65.54% <100.00%> (ø)
packages/jest-core/src/TestScheduler.ts 68.58% <100.00%> (+0.40%) ⬆️
packages/jest-snapshot/src/SnapshotResolver.ts 96.66% <100.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4926564...09033d8. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Apr 2, 2021

The Windows build is failing due to not being able to compile node-sass apparently 🤔

#11229

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.

🚀

packages/jest-core/src/SearchSource.ts Outdated Show resolved Hide resolved
).concat(status.filesRemovedList);
});
const updateSnapshotState = async () => {
await Promise.all(
Copy link
Member

Choose a reason for hiding this comment

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

should these run sequentially rather than in parallel?

Or potentially, calling await snapshot.buildSnapshotResolver(context.config), first, then cleanup in a sync loop like before? I think that makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, none of the tests failed when it was running in parallel... 😅
But you're the boss - I'm happy to change to be sequential 🙂

'jest-silent-reporter',
{showPaths: true, showWarnings: true, useDots: true},
],
'default',
Copy link
Member

Choose a reason for hiding this comment

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

@G-Rath this 😀

@@ -35,12 +35,12 @@ beforeEach(() => {
roots: ['./packages/jest-resolve-dependencies'],
});
return Runtime.createContext(config, {maxWorkers, watchman: false}).then(
Copy link
Member

Choose a reason for hiding this comment

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

make the whole thing async-await instead of just the then

localRequire: Promise<LocalRequire> | LocalRequire = createTranspilingRequire(
config,
),
): Promise<SnapshotResolver> => {
const key = config.rootDir;
if (!cache.has(key)) {
Copy link
Member

Choose a reason for hiding this comment

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

since this is async, do we need to have some TOCTOU guard?

@@ -22,21 +24,31 @@ export const isSnapshotPath = (path: string): boolean =>
path.endsWith(DOT_EXTENSION);

const cache: Map<Config.Path, SnapshotResolver> = new Map();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const cache: Map<Config.Path, SnapshotResolver> = new Map();
const cache = new Map<Config.Path, SnapshotResolver>();

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.

thanks! I managed to let this linger long enough to get even more conflicts 🙈

@G-Rath
Copy link
Contributor Author

G-Rath commented Apr 22, 2021

... if the test breaks, I swear I won't be a happy camper... 😂

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.

Thanks!

forgot changelog 😅

@SimenB SimenB merged commit c8b1835 into jestjs:master Apr 22, 2021
@G-Rath G-Rath deleted the transform-snapshotResolver-field branch April 23, 2021 00:15
@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 24, 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.

Support transforming resolves (to allow for .ts-based snapshotResolver)
7 participants