chore: more through system tests for react + bundle combinations #22329
Conversation
|
Thanks for taking the time to open a PR!
|
lmiller1990
force-pushed
the
lmiller1990/21965
branch
from
46ed28a
to
162d060
Compare
lmiller1990
force-pushed
the
lmiller1990/21965
branch
from
162d060
to
925eaf6
Compare
lmiller1990
force-pushed
the
lmiller1990/21965
branch
from
925eaf6
to
7b5008f
Compare
lmiller1990
changed the title
|
Test summary
Run details
View run in Cypress Dashboard ➡️ Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
I think we should choose a different name for the system-test project/project-fixture. What does the npm- prefix mean? That it is testing react with external deps? Something like react-advanced would make more sense to me.
Only blocking comment is the react resolve issue.
| '**/__snapshots__/*', | ||
| '**/__image_snapshots__/*', | ||
| 'examples/**/*', | ||
| // due to how we handle system-tests, tests with hooks |
There was a problem hiding this comment.
We can fix this with a change to webpackConfig.resolve i.e.
webpackConfig: {
...webpackConfig,
resolve: {
alias: {
react: require.resolve('react'),
'react-dom': require.resolve('react-dom'),
},
},
},
I know it's a hack for our system-tests, but having the hack gives us more coverage and I would feel better knowing the system test is actually using the correct version of react. I know we can do similar hack for vite to resolve from the project rather than our node_modules.
Also, I noticed the counter-hooks.cy.jsx has a compile error, it should be importing the component with the extension (same with input-accumulator.cy.jsx).
There was a problem hiding this comment.
I'll try the alias.resolve, nice idea.
As for the jsx extension, we actually don't need it here since we've got
resolve: {
extensions: ['.js', '.ts', '.jsx', '.tsx'],In the webpack.config.js.
| @@ -0,0 +1,31 @@ | |||
| { | |||
| "devDependencies": { | |||
| "@babel/core": "7.4.5", | |||
There was a problem hiding this comment.
Looks like we can remove a decent amount of these deps, namely:
"@bahmutov/cy-api": "1.4.2",
"@bahmutov/cy-rollup": "2.0.0",
"@cypress/code-coverage": "3.9.4",
"babel-plugin-istanbul": "6.0.0",
"babel-plugin-module-resolver": "4.0.0",
"cypress-plugin-snapshots": "1.4.4",
"module-resolver": "1.0.0",
| @@ -295,6 +295,7 @@ export async function scaffoldCommonNodeModules () { | |||
| 'lodash', | |||
| 'proxyquire', | |||
| 'react', | |||
| 'react-dom', | |||
There was a problem hiding this comment.
If we use resolve/alias to resolve the proper react/react-dom packages, do we need this change?
There was a problem hiding this comment.
I think if we have react here, we should include react-dom too - only caching one could lead to pretty confusing and hard to debug errors. Or, we should remove both. I saw react was here by itself, so I added react-dom.
There was a problem hiding this comment.
For consistency that makes sense. Ideally I think we should remove both but I imagine doing so would cause other tests to fail
|
Now #22876 is landing, I'm thinking the infra set up there is probably a better way to incrementally add tests to many projects at once. Gonna close this in favor. |
User facing changelog
N/A
Additional details
Review Notes
102 files! Scary - well, not really. I committed the files in an easy-to-review way.
npm/reacttoproject-fixtures/npm-react. No change here, just moving things around.npm-react-vite,npm-react-webpack). Basic config.Summary
As noted in. #21965 (comment), in b326693, a bunch of tests got straight up deleted, and
npm/reactmove from Vite to Webpack. The majority of the specs were duplicates, but we should keep some, as well as start moving morenpm/reacttests tosystem-tests, which lets us easily run them against multiple combinations of webpack/vite/react versions.During #21965, the main thing we lost was coverage of webpack + React - the most popular way to bundle and serve React apps.
In this PR, I moved a number of specs from
npm/react/component/basictosystem-tests/project-fixtures/npm-react. We now run these specs system-tests in:system-tests/project-fixtures/npm-react-webpacksystem-tests/project-fixtures/npm-react-viteTo verify some of the more complex examples (things like webpack/babel, CSS modules, stub/spy on component methods) work properly for both Vite and Webpack.
Steps to test
Just observe CI is passing!
How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?