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

[14494] - Improve e2e testing docs and add cli args #14673

Closed
wants to merge 4 commits into from
Closed

[14494] - Improve e2e testing docs and add cli args #14673

wants to merge 4 commits into from

Conversation

nicolad
Copy link
Member

@nicolad nicolad commented Mar 28, 2019

Description

Fixes to: #14494
I added local arg to npm run test-e2e command.

How has this been tested?

It was tested locally.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@@ -50,4 +50,10 @@ if ( hasCliArg( '--puppeteer-interactive' ) ) {
process.env.PUPPETEER_SLOWMO = getCliArg( '--puppeteer-slowmo' ) || 80;
}

if ( hasCliArg( '--puppeteer-local' ) ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Hello @gziolo :)
I am not sure of 2 things here:

  1. whether local is the best arg name
  2. if checking arg with --puppeteer- as in example above is easy to reason about

Copy link
Member

Choose a reason for hiding this comment

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

Actually, the defaults are:

process.env.WP_BASE_URL = 'http://localhost:8889';
process.env.WP_USERNAME = 'admin';
process.env.WP_PASSWORD = 'password';

See: https://github.com/WordPress/gutenberg/blob/master/packages/e2e-test-utils/src/shared/config.js#L9

So there is no need to set them explicitly. We need the opposite, to have a way to override all those 3 values:

if ( hasCliArg( '--wordpress-host' ) ) {
    process.env.WP_BASE_URL = getCliArg( '--wordpress-host' );
}
if ( hasCliArg( '--wordpress-username' ) ) {
    process.env. WP_USERNAME = getCliArg( '--wordpress-username' );
}
if ( hasCliArg( '--wordpress-password' ) ) {
    process.env. WP_PASSWORD = getCliArg( '--wordpress-password' );
}

or even better something which uses look up table and loops over it.

@@ -215,6 +215,10 @@ _Example:_
This is how you execute those scripts using the presented setup:

* `npm run test:e2e` - runs all unit tests.
* `npm run test-e2e interactive` - runs all unit tests interactively.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it look as follows? /cc @draganescu

* `npm run test-e2e -- --puppeteer-interactive` - runs all unit tests interactively.
* `npm run test-e2e FILE_NAME -- --puppeteer-interactive ` - runs one test file interactively.
* `npm run test-e2e:watch -- --puppeteer-interactive` - runs all tests interactively and watch for changes.

In watch mode, you rather don't pass the name of the file as by default it runs only for modified files.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made this change based on this PR's description: #14129

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but I didn't update the PR's description with the changes in the comments. @gziolo is right, in the end it was developed as:

* `npm run test-e2e -- --puppeteer-interactive` - runs all unit tests interactively.
* `npm run test-e2e FILE_NAME -- --puppeteer-interactive ` - runs one test file interactively.
* `npm run test-e2e:watch -- --puppeteer-interactive` - runs all tests interactively and watch for changes.

@@ -50,4 +50,10 @@ if ( hasCliArg( '--puppeteer-interactive' ) ) {
process.env.PUPPETEER_SLOWMO = getCliArg( '--puppeteer-slowmo' ) || 80;
}

if ( hasCliArg( '--puppeteer-local' ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Actually, the defaults are:

process.env.WP_BASE_URL = 'http://localhost:8889';
process.env.WP_USERNAME = 'admin';
process.env.WP_PASSWORD = 'password';

See: https://github.com/WordPress/gutenberg/blob/master/packages/e2e-test-utils/src/shared/config.js#L9

So there is no need to set them explicitly. We need the opposite, to have a way to override all those 3 values:

if ( hasCliArg( '--wordpress-host' ) ) {
    process.env.WP_BASE_URL = getCliArg( '--wordpress-host' );
}
if ( hasCliArg( '--wordpress-username' ) ) {
    process.env. WP_USERNAME = getCliArg( '--wordpress-username' );
}
if ( hasCliArg( '--wordpress-password' ) ) {
    process.env. WP_PASSWORD = getCliArg( '--wordpress-password' );
}

or even better something which uses look up table and loops over it.

@gziolo gziolo added [Type] Developer Documentation Documentation for developers [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Mar 29, 2019
@gziolo gziolo requested a review from draganescu March 29, 2019 11:17
@nicolad
Copy link
Member Author

nicolad commented Mar 29, 2019

I messed up deleting the fork, created another PR based on current feedback: #14717

@nicolad nicolad closed this Mar 29, 2019
@nicolad nicolad added the [Status] Not Implemented Issue/PR we will (likely) not implement. label Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Not Implemented Issue/PR we will (likely) not implement. [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants