-
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
Improve e2e testing docs and add cli args. #14717
Improve e2e testing docs and add cli args. #14717
Conversation
packages/scripts/scripts/test-e2e.js
Outdated
WP_PASSWORD: "--wordpress-password" | ||
}; | ||
|
||
Object.entries(configsMapping).forEach(([key, value]) => { |
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.
Is this readable enough on first glance?
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.
Yes, it looks good. You can use more specific names instead key and name, e.g. envKey and argName or something like that.
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.
Adjusted, thx.
There are 24 warnings reported by ESLint: It reports also that most of them can be fixed by running npm run lint-js — --fix |
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.
Let's add one more tweak as explained in my comment and this is ready to land :)
Thanks for addressing all comments.
process.env[ key ] = getCliArg( value ); | ||
} | ||
} ); | ||
|
||
jest.run( [ ...config, ...runInBand, ...getCliArgs( cleanUpPrefixes ) ] ); |
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.
const cleanUpPrefixes = [ '--puppeteer-' ];
At the moment cleanUpPrefixes
doesn't filter out --wordpress-
. It should be a good practice to ensure they are never passed to Jest
as we discovered some issues with --interactive
in the past.
That's why @draganescu added logic which allows to filter out CLI args starting with a given pattern.
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.
Didn't knew that, had adjusted, thx.
packages/scripts/scripts/test-e2e.js
Outdated
@@ -50,4 +50,16 @@ if ( hasCliArg( '--puppeteer-interactive' ) ) { | |||
process.env.PUPPETEER_SLOWMO = getCliArg( '--puppeteer-slowmo' ) || 80; | |||
} | |||
|
|||
const configsMapping = { | |||
WP_BASE_URL: '--wordpress-host', |
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.
Do you think we should align with the env variable name? --wordpress-base-url
maybe. I forgot that WordPress can be also installed in subfolder and seeing the actual implementation made me think :)
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.
Yes, that's better, since there is no need to think twice why the argument name is describing a different thing than expected from env key.
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.
Awesome work. Thanks for your contribution 👍
Description
Fixes to: #14494
I added
local
arg tonpm run test-e2e
command.How has this been tested?
It was tested locally.
Checklist: