Adds native support for PnP to jest #8094

arcanis · 2019-03-09T13:01:02Z

Summary

PnP currently needs an extra plugin to work properly. Adding native would remove friction and strictly improve users' life (since PnP users can't really use Jest without the plugin anyway).

Test plan

I went to implement a e2e test but it proved challenging given the current test infra. The problem is that the tests run on the local Jest build (which makes sense), which doesn't use PnP. It makes it difficult to spawn a e2e test that would use it 😕

So I guess for now the main test is that it doesn't break non-PnP users.

arcanis · 2019-03-09T13:03:01Z

Note that I've directly used the jest-pnp-resolver to make the diff simpler, but I'd be open to transfer the repo to the Jest org in a follow-up 🙂

SimenB · 2019-03-09T13:07:02Z

Exciting!

Note that I've directly used the jest-pnp-resolver to make the diff simpler, but I'd be open to transfer the repo to the Jest org in a follow-up 🙂

Yeah, that makes sense to me as a follow-up. I guess it's stable enough to not be hindered by Jest's slower release process?

I went to implement a e2e test but it proved challenging given the current test infra. The problem is that the tests run on the local Jest build (which makes sense), which doesn't use PnP. It makes it difficult to spawn a e2e test that would use it 😕

Is it possible to package up a tarball for PnP? We have bunch of tests that sets up temp directories in /tmp, runs yarn etc.

SimenB · 2019-03-09T13:09:01Z

From CI:

error TS7016: Could not find a declaration file for module 'jest-pnp-resolver'. '/home/circleci/jest/node_modules/jest-pnp-resolver/index.js' implicitly has an 'any' type.
  Try `npm install @types/jest-pnp-resolver` if it exists or add a new declaration (.d.ts) file containing `declare module 'jest-pnp-resolver';

Maybe add a typings file to the project? If not, you can add a .d.ts file (similar to how e.g. node-notifier is currently)

SimenB · 2019-03-09T13:11:50Z

Another cool integration test would be that the project is usable with PnP in general, not just the resolver. I have nefarious plans to split up jest-config, and it would be nice to have a test yelling at me if I mess up the dependency tree. A test using jest-pnp-resolver shows that jest can be used to test PnP projects, but a separate one showing that a version of Jest installed in a PnP project can start at all is something else (I think that makes sense! 😀)

arcanis · 2019-03-09T13:31:19Z

Found a way to make a test! I was worried that mixing PnP (the test env) and non-PnP (the Jest repo) wouldn't work, but PnP is smart enough to fallback to the regular Node resolution for the relevant parts of the tree when it detects this situation 🙂

Yeah, that makes sense to me as a follow-up. I guess it's stable enough to not be hindered by Jest's slower release process?

I think so, yep. I haven't had to change the API in a while and I don't expect this to be very frequent. Most changes are in the PnP implementation, not its public interface.

Another cool integration test would be that the project is usable with PnP

You mean the whole Jest repo? If you're interested, we could look into making you early adopters of Yarn v2, that would be win-win! There are still probably some rough edges, but it would be instructive and I'd be happy to fix them.

SimenB · 2019-03-09T13:35:06Z

You mean the whole Jest repo? If you're interested, we could look into making you early adopters of Yarn v2, that would be win-win!

I meant installing jest in some location and running it to ensure dependencies are correct.
But migrating to yarn v2 would be even cooler and assert on the same thing. I'm definitely down!

jeysal · 2019-03-09T13:57:36Z

If you're interested, we could look into making you early adopters of Yarn v2

Hell yeah 🔥
I made a quick attempt at using PnP in #7726 but there were issues in many different places so I decided to not invest further time in it.
I guess if PnP works, that should already be half the battle to make v2 work and uncover quite a few issues a large infrastructure project like Jest might have with it?
@arcanis will you be at FB London when the Jest summit takes place March 18-20? Even if Yarn v2 is still too alpha by then, we could probably get PnP working a lot quicker than I could on my own 😅

arcanis · 2019-03-09T14:03:26Z

@arcanis will you be at FB London when the Jest summit takes place March 18-20? Even if Yarn v2 is still too alpha by then, we could probably get PnP working a lot quicker than I could on my own 😅

That's the week I'm in Menlo Park 😭😭

But I'd definitely be up to schedule a few hours for a peer coding session to get a better sense of the blockers together!

jeysal · 2019-03-09T14:18:13Z

That's the week I'm in Menlo Park

Damn, unlucky ^^

But I'd definitely be up to schedule a few hours for a peer coding session to get a better sense of the blockers together!

Sounds great! I'll drop you a DM / mail next week or so :)

jeysal

LGTM once CI passes - Node 6 is probably just a trailing comma thing as usual? 😅

arcanis · 2019-03-09T14:28:24Z

Indeed - should be fixed once #8095 is merged 👍

jeysal · 2019-03-09T18:41:39Z

BTW, mind explaining to me why #8095 will fix this? Was there a bug in Yarn 1.13 when using Node 6?

arcanis · 2019-03-09T18:46:59Z

Not exactly a bug, but PnP wasn't originally tested on Node 6 so the generated .pnp.js file used trailing commas. It got fixed in yarnpkg/yarn#6871.

arcanis · 2019-03-09T18:49:21Z

Which reminds me btw that Yarn 2 will drop compatibility with Node 6, since its official EOL is in April. Will that be a problem?

https://github.com/nodejs/Release/blob/master/schedule.svg

jeysal · 2019-03-09T18:52:07Z

Not exactly a bug, but PnP wasn't originally tested on Node 6 so the generated .pnp.js file used trailing commas. It got fixed in yarnpkg/yarn#6871.

Ah okay, thanks! I couldn't really interpret the CI error 😅

Which reminds me btw that Yarn 2 will drop compatibility with Node 6, since its official EOL is in April. Will that be a problem?

I think we intend to drop it in the next major after the EOL as well, so that should be fine. cc @SimenB

SimenB · 2019-03-09T18:52:12Z

Timing wise, maaaaaybe. But I wanna press pretty aggressively for Node 8 (so we can stop transpiling async, upgrade jsdom and use ink), so there shouldn't be a huge issue. But I'm very much a fan of moving to node 8 quickly

SimenB · 2019-03-09T18:53:43Z

Windows is failing

Error: Cannot find module 'D:\a\1\jest\e2e\pnp/.pnp.js'

codecov-io · 2019-03-09T20:24:17Z

Codecov Report

Merging #8094 into master will decrease coverage by <.01%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #8094      +/-   ##
==========================================
- Coverage   62.46%   62.45%   -0.01%     
==========================================
  Files         263      264       +1     
  Lines       10371    10374       +3     
  Branches     2514     2514              
==========================================
+ Hits         6478     6479       +1     
- Misses       3317     3318       +1     
- Partials      576      577       +1

Impacted Files	Coverage Δ
packages/jest-resolve/src/defaultResolver.ts	`64.06% <0%> (-2.07%)`	⬇️
e2e/pnp/lib/index.js	`100% <100%> (ø)`

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 562ac18...80b65fb. Read the comment docs.

SimenB · 2019-03-09T20:40:33Z

Still same error on azure :/

arcanis · 2019-03-09T20:49:43Z

@SimenB I'll disable the test on Windows. It's hard to tell without starting a bunch of other runs just to get extra logs, but I think it's simply that on this specific Azure setup the Yarn cache (probably on the user folder in C:) is on a different drive than the git clone (which seems to be in D:). Since Yarn cannot generate relative paths between the two, the PnP codepath is disabled. It shouldn't affect end-users.

(For the record a more long-term fix is to configure the Yarn cache to be stored on the same disk, for example in the git repository itself, but I think that's a bit heavy for this PR)

SimenB · 2019-03-09T20:59:52Z

Sounds good

arcanis · 2019-03-09T21:42:53Z

All green 👍

SimenB · 2019-03-09T22:29:20Z

e2e/__tests__/pnp.test.ts

+
+it('sucessfully runs the tests inside `pnp/`', () => {
+  // https://github.com/facebook/jest/pull/8094#issuecomment-471220694
+  if (process.platform !== 'win32') {


we have a skipOnWindows helper

import {skipSuiteOnWindows} from '@jest/test-utils'; skipSuiteOnWindows();

I can try to add it via GH

billyvg · 2019-03-13T22:45:42Z

I'm having issues with pnp + a setupFile that has imports that depend on a modulePath -- anyone else have a similar issue? You can repo here: https://github.com/getsentry/sentry/tree/build/travis/yarn-pnp

I can submit as a new issue if that's better

SimenB · 2019-03-14T07:46:11Z

A new issue, yes 🙂

github-actions · 2021-05-11T21:06:53Z

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.

Adds native support for PnP to jest

c8df3a7

facebook-github-bot added the cla signed label Mar 9, 2019

arcanis added 3 commits March 9, 2019 13:25

Adds a test

ca6d13d

Runs prettier

256612e

Adds an entry into the changelog

283d11d

arcanis added 2 commits March 9, 2019 13:41

Uses jest-pnp-resolver 1.2.1 w/ ts types

b7524cf

Fixes headers

a3d5e27

jeysal approved these changes Mar 9, 2019

View reviewed changes

arcanis added 2 commits March 9, 2019 19:07

Merge remote-tracking branch 'upstream/master' into jest-pnp-builtin

35df709

Stops execa from "preferLocal"

86b09af

arcanis force-pushed the jest-pnp-builtin branch from 1cd2475 to 86b09af Compare March 9, 2019 20:01

Disables the pnp test on windows

29cd699

arcanis force-pushed the jest-pnp-builtin branch from 75c2eb6 to 29cd699 Compare March 9, 2019 21:12

SimenB reviewed Mar 9, 2019

View reviewed changes

Update pnp.test.ts

80b65fb

SimenB merged commit c929ee3 into jestjs:master Mar 9, 2019

lorenzorapetti mentioned this pull request Mar 11, 2019

Update to Jest 24 facebook/create-react-app#6278

Merged

eps1lon mentioned this pull request Jul 30, 2019

docs: Add notice about jest 24.4 arcanis/jest-pnp-resolver#9

Merged

snyk-bot mentioned this pull request Dec 15, 2019

[Snyk] Upgrade babel-jest from 24.0.0 to 24.9.0 jakeherp/burger-builder#14

Open

This was referenced Mar 21, 2020

[Snyk] Upgrade: jest, jest-cli lwojcik/sc2pte-panel-frontend#88

Closed

[Snyk] Upgrade jest from 24.0.0 to 24.9.0 DavidKindler/burger-builder#47

Merged

G-Rath mentioned this pull request Aug 1, 2020

test: allow more tests to run on windows #10351

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds native support for PnP to jest #8094

Adds native support for PnP to jest #8094

arcanis commented Mar 9, 2019 •

edited

Loading

arcanis commented Mar 9, 2019

SimenB commented Mar 9, 2019

SimenB commented Mar 9, 2019

SimenB commented Mar 9, 2019 •

edited

Loading

arcanis commented Mar 9, 2019

SimenB commented Mar 9, 2019

jeysal commented Mar 9, 2019

arcanis commented Mar 9, 2019

jeysal commented Mar 9, 2019

jeysal left a comment

arcanis commented Mar 9, 2019

jeysal commented Mar 9, 2019

arcanis commented Mar 9, 2019

arcanis commented Mar 9, 2019

jeysal commented Mar 9, 2019

SimenB commented Mar 9, 2019

SimenB commented Mar 9, 2019

codecov-io commented Mar 9, 2019 •

edited

Loading

SimenB commented Mar 9, 2019

arcanis commented Mar 9, 2019 •

edited

Loading

SimenB commented Mar 9, 2019

arcanis commented Mar 9, 2019

SimenB Mar 9, 2019

SimenB Mar 9, 2019 •

edited

Loading

billyvg commented Mar 13, 2019

SimenB commented Mar 14, 2019

github-actions bot commented May 11, 2021

Adds native support for PnP to jest #8094

Adds native support for PnP to jest #8094

Conversation

arcanis commented Mar 9, 2019 • edited Loading

Summary

Test plan

arcanis commented Mar 9, 2019

SimenB commented Mar 9, 2019

SimenB commented Mar 9, 2019

SimenB commented Mar 9, 2019 • edited Loading

arcanis commented Mar 9, 2019

SimenB commented Mar 9, 2019

jeysal commented Mar 9, 2019

arcanis commented Mar 9, 2019

jeysal commented Mar 9, 2019

jeysal left a comment

Choose a reason for hiding this comment

arcanis commented Mar 9, 2019

jeysal commented Mar 9, 2019

arcanis commented Mar 9, 2019

arcanis commented Mar 9, 2019

jeysal commented Mar 9, 2019

SimenB commented Mar 9, 2019

SimenB commented Mar 9, 2019

codecov-io commented Mar 9, 2019 • edited Loading

Codecov Report

SimenB commented Mar 9, 2019

arcanis commented Mar 9, 2019 • edited Loading

SimenB commented Mar 9, 2019

arcanis commented Mar 9, 2019

SimenB Mar 9, 2019

Choose a reason for hiding this comment

SimenB Mar 9, 2019 • edited Loading

Choose a reason for hiding this comment

billyvg commented Mar 13, 2019

SimenB commented Mar 14, 2019

github-actions bot commented May 11, 2021

arcanis commented Mar 9, 2019 •

edited

Loading

SimenB commented Mar 9, 2019 •

edited

Loading

codecov-io commented Mar 9, 2019 •

edited

Loading

arcanis commented Mar 9, 2019 •

edited

Loading

SimenB Mar 9, 2019 •

edited

Loading