Implement support for transforming snapshot resolvers #8829
Conversation
|
@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 Something which I do for debugging, is to check the cache to see what was the output of the transform. You can link |
|
@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. |
|
It fails since snapshots runs inside the sandbox, and |
|
There's already a |
|
I'm finding the jest codebase too overwhelming to figure out where the correct place to call Would you guys be able to expand on the diff you'd expect for this? In particular:
From what I've been able to find,
What would you expect this to look like, and where? (both roughly) |
|
It's my last code link, the |
|
@G-Rath I think we'd need to |
|
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: However, something tells me you guys probably will be against that, so I've left it as is for now. |
packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapterInit.ts
Outdated
Show resolved
Hide resolved
I'm down with that. 🙂 Or just a separate |
I've actually already done that 😄 |
|
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 |
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. |
Just what I wanted to hear 😂
That should be reverted, which I force-pushed. I'll have a play around to see if I've miss something, just in case.
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. |
There was a problem hiding this comment.
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.
|
@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 |
|
So Which is fine, except it doesn't have a Both |
|
So |
|
@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.
|
|
@SimenB so I'm thinking that for this to be possible I think the majority of call sites are already async, except for the aforementioned |
yep 👍 Lots of "load this thing" will become async as it's needed for |
well, that's fun - would be nice to know what the errors were😕 |
|
@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 |
|
The Windows build is failing due to not being able to compile |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
| ).concat(status.filesRemovedList); | ||
| }); | ||
| const updateSnapshotState = async () => { | ||
| await Promise.all( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.config.ci.js
Outdated
| 'jest-silent-reporter', | ||
| {showPaths: true, showWarnings: true, useDots: true}, | ||
| ], | ||
| 'default', |
| @@ -35,12 +35,12 @@ beforeEach(() => { | |||
| roots: ['./packages/jest-resolve-dependencies'], | |||
| }); | |||
| return Runtime.createContext(config, {maxWorkers, watchman: false}).then( | |||
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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(); | |||
There was a problem hiding this comment.
| const cache: Map<Config.Path, SnapshotResolver> = new Map(); | |
| const cache = new Map<Config.Path, SnapshotResolver>(); |
|
... if the test breaks, I swear I won't be a happy camper... 😂 |
|
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. |
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
snapshotResolverneeds to be transformed to pass is added.Closes #8330
Supports #8810