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

Update to Jest 24 #6278

Merged
merged 16 commits into from
Mar 15, 2019
Merged

Update to Jest 24 #6278

merged 16 commits into from
Mar 15, 2019

Conversation

lorenzorapetti
Copy link
Contributor

@lorenzorapetti lorenzorapetti commented Jan 25, 2019

Things to notice before merging

  • There's currently a bug for which jest 24 crashes if it's called programmatically. Until a bugfix comes out, i've added require('jest-cli/build/cli') before require('jest') like this comment suggests. This has been fixed in version 24.1 🎉

  • The current version of jest-pnp-resolver doesn't work because jest changed the file names of jest-resolve. They already have the fix in their code but it isn't published yet. I've filed an issue, but in the meantime i've created pnpResolver.js just to make things work. Fixed in jest-pnp-resolver 1.1.0 🎉

  • Jest 24 comes with a new setupFilesAfterEnv option that replaces setupTestFrameworkScriptFile. It now accepts an array of files instead of a single one. I'm wondering if we should allow multiple setup files?

@rally25rs
Copy link

I think (based on the comments in the issue) that jest@24 would fix #2393 so it would be really nice to get this working.

setupFiles: [
isEjecting
? 'react-app-polyfill/jsdom'
: require.resolve('react-app-polyfill/jsdom'),
],

setupTestFrameworkScriptFile: setupTestsFile,
setupFilesAfterEnv: setupTestsFile ? [setupTestsFile] : [],

Choose a reason for hiding this comment

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

Maybe rather than this here you modify the assignment of setupTestsFile above to potentially allow multiple files to be assigned:

function setupTestsFiles(rootDir) {
    const setupTestsFiles = [];
    if(fs.existsSync(paths.testsSetup)){
      const setupTestsMatches = paths.testsSetup.match(/src[/\\]setupTests\.(.+)/);
      const setupTestsFileExtension = (setupTestsMatches && setupTestsMatches[1]) || 'js';
      setupTestsFiles.push(`${rootDir}/src/setupTests.${setupTestsFileExtension}`)
    }
    return setupTestsFiles
}

...
module.exports = (resolve, rootDir, isEjecting) => {
...

setupFilesAfterEnv: setupTestsFiles(rootDir),

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 can totally do this, although i think that most users would use setupFilesAfterEnv like:

setupFilesAfterEnv: [
  'jest-dom/extend-expect',
  'react-testing-library/cleanup-after-each'
]

rather than creating multiple file themselves.

IMO we should allow overriding setupFilesAfterEnv in package.json if we really want to support this.

@SimenB
Copy link
Contributor

SimenB commented Jan 30, 2019

Config for typescript can probably be simplified whenever this lands (jest uses babel-jest for ts automatically and finds tests). And if babel-core@bridge is only here for Jest, it can be removed

@mrmckeb
Copy link
Contributor

mrmckeb commented Feb 3, 2019

Just a side-note/reminder: Once this is done we'll merge in #4176.

@lorenzorapetti lorenzorapetti force-pushed the jest24 branch 2 times, most recently from d2fb553 to 089cdeb Compare February 6, 2019 13:03
@lorenzorapetti
Copy link
Contributor Author

I don't get why npx create-react-app --scripts-version=@latest test-app-dist-tag errors out with You have provided more than one argument for <project-directory>. in Travis 🤔

@lorenzorapetti lorenzorapetti changed the title WIP: Update to Jest 24 Update to Jest 24 Feb 6, 2019
@iansu
Copy link
Contributor

iansu commented Feb 6, 2019

This is related to a recent change. I think if you swap the arguments it should work. Please try that and let us know if that doesn't resolve the issue.

@lorenzorapetti
Copy link
Contributor Author

lorenzorapetti commented Feb 8, 2019

This is related to a recent change. I think if you swap the arguments it should work. Please try that and let us know if that doesn't resolve the issue.

It works now but there are 2 more failing builds that i can't get my head around. Can you help me with that? It appears that it fails only with latest node

@crubier
Copy link

crubier commented Feb 13, 2019

This is super welcome for us, we want to start a monorepo project with CRA + Typescript + Jest, and we will not start until we get jest 24 working nicely with this dream stack :-)

@ianschmitz
Copy link
Contributor

It works now but there are 2 more failing builds that i can't get my head around. Can you help me with that? It appears that it fails only with latest node

Yeah we're having some issues with the latest node builds in Travis on master as well.

@iansu
Copy link
Contributor

iansu commented Mar 13, 2019

Looks like we just have one failing test left: https://github.com/facebook/create-react-app/blob/master/test/fixtures/issue-5176-flow-class-properties/index.test.js

I'm not exactly sure why it's failing but let me know if you need any help looking into it.

@lorenzorapetti
Copy link
Contributor Author

Looks like we just have one failing test left: /test/fixtures/issue-5176-flow-class-properties/index.test.js@master

I'm not exactly sure why it's failing but let me know if you need any help looking into it.

I'm not quite sure how to takle this, it passes in my local machine (i'm on node 10.15).

Right now i'm trying to debug what's happening on travis

@lorenzorapetti
Copy link
Contributor Author

Aaaaand we have green lights 🎉🎉

@Domino987
Copy link

Awesome, when can we expect a release ?

@SimenB
Copy link
Contributor

SimenB commented Mar 14, 2019

You should wait for jestjs/jest#8088

@crubier
Copy link

crubier commented Mar 14, 2019

No, please please don't wait anymore lol, the pain is intense here without this merged, you have no idea!

I understand the problem with jestjs/jest#8088 , but this is a relatively rare bug that happens when you don't want to install watchman...

However having jest 24 (which supports typescript more easily) in create react app is a big deal for many of us.

@iansu
Copy link
Contributor

iansu commented Mar 14, 2019

We're going to merge this as is and release it in an alpha of version 3.0. I've created a follow up issue to make sure we update Jest before releasing the final version of 3.0.

@iansu
Copy link
Contributor

iansu commented Mar 15, 2019

@loryman The yarn.lock.cached file in react-scripts should not be part of your PR. Can you revert that file? Otherwise I think we're ready to merge this.

@lorenzorapetti
Copy link
Contributor Author

@loryman The yarn.lock.cached file in react-scripts should not be part of your PR. Can you revert that file? Otherwise I think we're ready to merge this.

Yeah sorry, just reverted it!

@crubier
Copy link

crubier commented Mar 15, 2019

We're going to merge this as is and release it in an alpha of version 3.0. I've created a follow up issue to make sure we update Jest before releasing the final version of 3.0.

Nice! Do you have an ETA on this v3 alpha release ? Thank you

@ianschmitz
Copy link
Contributor

Nice! Do you have an ETA on this v3 alpha release ? Thank you

We're hoping in the next couple days to have a first cut alpha out.

@iansu iansu merged commit 3be3576 into facebook:master Mar 15, 2019
@iansu
Copy link
Contributor

iansu commented Mar 15, 2019

Thanks!

@lorenzorapetti lorenzorapetti deleted the jest24 branch March 15, 2019 21:01
@lock lock bot locked and limited conversation to collaborators Mar 20, 2019
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.