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

Revert "lookup: temporarily use head for jest" #901

Merged
merged 3 commits into from
May 5, 2022

Conversation

BethGriggs
Copy link
Member

Reverts #899


It seems Jest failing under CITGM on all platforms with v28.0.1 (peerDeps related?)

@targos
Copy link
Member

targos commented Apr 26, 2022

It seems Jest failing under CITGM on all platforms with v28.0.1 (peerDeps related?)

It seems that a new commit is always necessary to fix the lockfile after a release:
jestjs/jest@6b38f3b
jestjs/jest@474cf26

If we remove head: true, we will always be 1 commit before it.

@SimenB

@SimenB
Copy link
Member

SimenB commented Apr 26, 2022

😅

From yarnpkg/berry#3486 (comment) we can probably pass --no-immutable as install in lookup.json?

We need the extra commit as we cannot use workspace:* due to lerna not supporting it. I will fix this at some point, but that point isn't soon, unfortunately. --no-immutable seems like a fine workaround in the meantime

lib/lookup.json Outdated Show resolved Hide resolved
Co-authored-by: Michaël Zasso <targos@protonmail.com>
@SimenB
Copy link
Member

SimenB commented Apr 26, 2022

Ah jeez, we've broken the scripts/remove-examples.mjs script when migrating it to ESM from CJS... jestjs/jest@b55add9

@SimenB
Copy link
Member

SimenB commented Apr 27, 2022

28.0.2 is out with the above fix. Can you re-trigger? 🤞

@BethGriggs
Copy link
Member Author

Thanks for looking into it. I've rerun (using this branch of citgm to pick up the scripts changes) but got a new error:

13:20:57 info: jest yarn:             | test suite started  
13:20:58 verbose: jest npm-test:      | node:internal/modules/cjs/loader:960                                                                                                                  
13:20:58 verbose:                     | const err = new Error(`Cannot find module '${request}'`);                                                                                             
13:20:58 verbose:                     | ^                                                                                                                                                     
13:20:58 verbose:                     |                                                                                                                                                       
13:20:58 verbose:                     | Error: Cannot find module '/home/iojs/tmp/citgm_tmp/ee21c513-03e5-4927-a894-d98f5681a49c/jest/node_modules/pretty-format/build/plugins/ConvertAnsi.js'
13:20:58 verbose:                     | at createEsmNotFoundErr (node:internal/modules/cjs/loader:960:15)                                                                                     
13:20:58 verbose:                     | at finalizeEsmResolution (node:internal/modules/cjs/loader:953:15)                                                                                    
13:20:58 verbose:                     | at resolveExports (node:internal/modules/cjs/loader:483:14)                                                                                           
13:20:58 verbose:                     | at Module._findPath (node:internal/modules/cjs/loader:523:31)                                                                                         
13:20:58 verbose:                     | at Module._resolveFilename (node:internal/modules/cjs/loader:925:27)                                                                                  
13:20:58 verbose:                     | at Function.resolve (node:internal/modules/cjs/helpers:108:19)                                                                                        
13:20:58 verbose:                     | at file:///home/iojs/tmp/citgm_tmp/ee21c513-03e5-4927-a894-d98f5681a49c/jest/jest.config.mjs:38:33                                                    
13:20:58 verbose:                     | at ModuleJob.run (node:internal/modules/esm/module_job:198:25)                                                                                        
13:20:58 verbose:                     | at async Promise.all (index 0)                                                                                                                        
13:20:58 verbose:                     | at async ESMLoader.import (node:internal/modules/esm/loader:409:24) {                                                                                 
13:20:58 verbose:                     | code: 'MODULE_NOT_FOUND',                                                                                                                             
13:20:58 verbose:                     | path: '/home/iojs/tmp/citgm_tmp/ee21c513-03e5-4927-a894-d98f5681a49c/jest/node_modules/pretty-format/package.json'                                    
13:20:58 verbose:                     | }                                                                                                                                                     
13:20:58 verbose:                     |                                                                                                                                                     

I could also reproduce locally with npx nodejs/citgm#revert-899-jest-master jest

@SimenB
Copy link
Member

SimenB commented Apr 27, 2022

Ah, nice! node_modules/pretty-format/build/plugins/ConvertAnsi.js should definitely exist, weird... Probably failing from head as well, then.

It's the result of require.resolve('pretty-format/ConvertAnsi'), so it being able to reach into build/plugins means it followed exports: https://github.com/facebook/jest/blob/f43871e37f8bf6dbe292ad2e52f4781868c4731b/packages/pretty-format/package.json#L19. Node internally then failing to find it to load seems.... very weird? 😅 Do you have any idea whart might be wrong? Something with the symlink of packages/pretty-format -> node_modules/pretty-format that yarn does as part of the workspace install?

@targos
Copy link
Member

targos commented Apr 27, 2022

I can also reproduce locally with:

Same error and stack.

@targos
Copy link
Member

targos commented Apr 27, 2022

There is no "build" folder in the pretty-format installation:

image

@SimenB
Copy link
Member

SimenB commented Apr 27, 2022

Aha - build:js needs to run first, but that needs to run after install, which needs to run after removing the examples 😅 Will figure something out

@targos
Copy link
Member

targos commented Apr 27, 2022

Running "build:js" before "remove-examples" works for me.

@richardlau
Copy link
Member

Aha - build:js needs to run first, but that needs to run after install, which needs to run after removing the examples 😅 Will figure something out

Is that not what

"scripts": ["remove-examples", "build:js", "test-ci-partial"],
is supposed to be doing?

Is the issue that install is run before remove-examples?

@targos
Copy link
Member

targos commented Apr 27, 2022

Here's the result of test-ci-partial on my computer:

> yarn test-ci-partial
.............
C:\Users\Targos\test\jjj\jest\packages\jest-resolve\src\__tests__\resolve.test.ts

  ● resolveModule › is possible to resolve node modules by resolving their realpath

    TypeError: The "from" argument must be of type string. Received undefined

      at Resolver.relative [as _throwModNotFoundError] (packages/jest-resolve/src/resolver.ts:427:18)

  ● resolveModuleAsync › is possible to resolve node modules by resolving their realpath

    TypeError: The "from" argument must be of type string. Received undefined

      at Resolver.relative [as _throwModNotFoundError] (packages/jest-resolve/src/resolver.ts:427:18)

...........Mercurial (hg) is not installed - skipping some tests
Mercurial (hg) is not installed - skipping some tests
Mercurial (hg) is not installed - skipping some tests
Mercurial (hg) is not installed - skipping some tests
Mercurial (hg) is not installed - skipping some tests
........Mercurial (hg) is not installed - skipping some tests
.....................................[SKIP] Does not work on Windows
.................................................................[SKIP] Does not work on jest-circus
.........................................................[SKIP] Does not work on jest-circus
..................................................................................................................[SKIP] Does not work on jest-circus
........................[SKIP] Does not work on jest-circus
.......................................................


Summary of all failing tests
 FAIL  packages/jest-resolve/src/__tests__/resolve.test.ts
  ● resolveModule › is possible to resolve node modules by resolving their realpath

    TypeError: The "from" argument must be of type string. Received undefined

      at Resolver.relative [as _throwModNotFoundError] (packages/jest-resolve/src/resolver.ts:427:18)

  ● resolveModuleAsync › is possible to resolve node modules by resolving their realpath

    TypeError: The "from" argument must be of type string. Received undefined

      at Resolver.relative [as _throwModNotFoundError] (packages/jest-resolve/src/resolver.ts:427:18)


Test Suites: 1 failed, 1 skipped, 384 passed, 385 of 386 total
Tests:       2 failed, 41 skipped, 4621 passed, 4664 total
Snapshots:   1819 passed, 1819 total
Time:        1116.317 s
Ran all test suites.

@SimenB
Copy link
Member

SimenB commented Apr 27, 2022

Ah, I thought the installation of the examples failed. If it's just running the tests then we can change the order (install will be slower, but probably fine?).

As for the failing test, sounds like an issue with symlinks

lib/lookup.json Outdated Show resolved Hide resolved
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
@SimenB
Copy link
Member

SimenB commented Apr 29, 2022

Looks like it passed? 🥳

@BethGriggs
Copy link
Member Author

Yes, passing now 🎉

@SimenB
Copy link
Member

SimenB commented May 5, 2022

Land it? 😀

@BethGriggs
Copy link
Member Author

Would someone in @nodejs/citgm mind landing this? (I guess I should request to be added to that team.)

@richardlau richardlau merged commit c6cc97f into main May 5, 2022
@richardlau richardlau deleted the revert-899-jest-master branch May 5, 2022 13:24
@richardlau
Copy link
Member

Would someone in @nodejs/citgm mind landing this? (I guess I should request to be added to that team.)

We could also add this repository to the list in nodejs/Release#557 and grant all releasers maintain access?

@SimenB
Copy link
Member

SimenB commented May 5, 2022

Thanks, folks! Happy that Jest is green on CITGM again. We do lots of weird stuff, good to get notice as early as possible about compat issues 😀

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.

4 participants