-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Scripts: Add test-e2e script to wp-scripts #12437
Conversation
"lerna": "3.4.3", | ||
"lint-staged": "7.2.0", | ||
"lint-staged": "7.3.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to bump it to ensure it uses the latest version of jest-validate
. Otherwise, it shows some warnings for the configuration which is invalid.
When working on docs (132a1e5) I realized how much config options we offer which are not documented. I can't add it to the docs in the current shape. I hope it's going to be possible as soon as I start working on the follow-up tasks. I think that the next step should be to put all tests into their own package and starting from there we should be able to extract another package with tests helper and utils to reuse with |
38557b5
to
ffc2df5
Compare
ffc2df5
to
d0bd8d6
Compare
hasCliArg( '--config' ) || | ||
hasProjectFile( 'jest.config.js' ) || | ||
hasProjectFile( 'jest.config.json' ) || | ||
hasPackageProp( 'jest' ); | ||
|
||
module.exports = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing the fromScriptsRoot
export.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I think only spawnScript
needs it at the moment so we might keep it internal. I will double check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ony used by spawnScript
so we can leave it unexposed. Unless you think we should keep it exported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly either way, I'd perhaps expose it for coherency, but I'm fine the other way round. Just wanted to bring our attention in case it was important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can expose, it's fine 👍
I'm seeing the following response for Vimeo embed in demo test:
I have no idea where it comes from to be honest. |
cad71f1
to
3985ef6
Compare
3985ef6
to
e028f7f
Compare
Otherwise, npm will not hoist the `scripts` dependencies in the node_modules root directory, but within node_modules/@wordpress/scripts/node_modules which won't work. This means the @wordpress/scripts package (and other deps) need to be copied here for testing purposes. See WordPress/gutenberg#12437 (comment)
@gziolo This is great work, bravo! I followed these testing instructions that use a test plugin I've created. Calling I've noticed two gotchas, though:
|
It sounds like something which needs to be fixed before we merge this PR. We have it in the root |
Yes, we can fix that now. I will just copy matcher for unit tests which extends default with what we effectively use in Gutenberg. |
e028f7f
to
f2482e6
Compare
Otherwise, npm will not hoist the `scripts` dependencies in the node_modules root directory, but within node_modules/@wordpress/scripts/node_modules which won't work. This means the @wordpress/scripts package (and other deps) need to be copied here for testing purposes. See WordPress/gutenberg#12437 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works now on a plugin, with and without config. Bravo!
f2482e6
to
e9b3d58
Compare
.travis.yml
Outdated
@@ -15,6 +15,7 @@ cache: | |||
- $HOME/.composer/cache | |||
- $HOME/.phpbrew | |||
- $HOME/.npm | |||
- node_modules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is risky I think :P. I understand the reasoning but the way npm works (we often do rm -rf
locally) can create issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove, just wanted to speed up the things, see the number of commits added 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core still caches node_modules
, might be worth considering pulling it from there also https://github.com/WordPress/wordpress-develop/blob/master/.travis.yml#L7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting, Riad might be right that it can cause troubles for PRs where the lock file gets frequently updated
b9e1f0f
to
9c24d26
Compare
9c24d26
to
9a80ebc
Compare
* Scripts: Add test-e2e script * Docs: Add docs for test-e2e script added to scripts packages * Update package-lock.json * Refactor scripts based on feedback shared in the review * More fixes to wp-scripts test-e2e * Address feedback shared in the review * Fix runInBand flag usage
* Scripts: Add test-e2e script * Docs: Add docs for test-e2e script added to scripts packages * Update package-lock.json * Refactor scripts based on feedback shared in the review * More fixes to wp-scripts test-e2e * Address feedback shared in the review * Fix runInBand flag usage
process.env.JEST_PUPPETEER_CONFIG = fromConfigRoot( 'puppeteer.config.js' ); | ||
} | ||
|
||
const config = ! hasJestConfig() ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For end-to-end tests, should this be checking for jest.config.js
(or package.json
jest
)? Or only jest-puppeteer.config.js
? How should we expect to give people control over whether to override configuration for either unit tests or end-to-end tests?
I'm running into this in my Dones project, where I do choose to override the default configuration for unit tests, but unfortunately it seems to bleed over into end-to-end test scripts as well, where I would prefer to simply use the default. Or at least, it's not clear how I can configure for both unit tests and end-to-end tests.
It kinda feels like this should be limited to checking for the presence of jest-e2e.config.js
perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It kinda feels like this should be limited to checking for the presence of jest-e2e.config.js perhaps?
An interesting challenge and I'm surprised it didn't come up earlier :)
How about we add higher priority for non-standard:
jest-e2e.config.js
jest-unit.config.js
To give folks a way to target only one or both of those tools without the need for using --config
flag.
As a temporary workaround, you can use the non-standard name of the file and pass it as a config option to the test script. In fact, this is how it works in Gutenberg:
Line 227 in 6c80cc3
"test-e2e": "wp-scripts test-e2e --config packages/e2e-tests/jest.config.js", |
Line 232 in 6c80cc3
"test-unit": "wp-scripts test-unit-js --config test/unit/jest.config.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In retrospect, it seems a bit more obvious in retrospect in in observing that the default configurations folder includes variations of these two files (jest-e2e.config.js
, jest-unit.config.js
), yet only one is supported for end-users. It's an interesting exercise to consider for other potential issues. Is there a similar problem with .eslintrc-md.js
and .eslintrc.js
?
How about we add higher priority for non-standard:
I think this could be a good solution. I was worried if it might have to be a "Breaking Change" if we only start considering one of these two, but (a) IIRC jest.config.js
is a "standard" file supported by Jest and (b) it could be viewed as a rare, advanced use-case where one opts to configure both.
As a temporary workaround, you can use the non-standard name of the file and pass it as a config option to the test script. In fact, this is how it works in Gutenberg:
Yeah, I ended up using something like this, or at least observing that higher priority is given to --config
, also observing that the E2E configuration is quite small and I wasn't entirely on board with the specs
forced folder convention of the default configuration 😅
wp-scripts test-e2e --config {\"preset\":\"jest-puppeteer\"} e2e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request: #22477
Description
Part of #12313.
I mostly moved code between files and extracted new utils
hasBabelConfig
andhasJestConfig
to reuse in the newly introduced script.wp-scripts test-e2e
script to@wordpress/scripts
andpuppeteer
,jest-puppeteer
lint-staged
Follow-up task
How has this been tested?
Both:
npm run test-e2e
npm run test-unit
should still work the same.
To test zero-config stuff, it's possible to run:
./node_modules/.bin/wp-scripts test-e2e --showConfig
./node_modules/.bin/wp-scripts test-unit-js --showConfig
Types of changes
Checklist: