Update/e2e tests options

[draganescu]

Description

Following #13993 and part of #8319 this adds the option to run all e2e test commands with an extra interactive commands, which shows Chrome at a human speed, so you can visually inspect your test.

So now we can run:

npm run test-e2e interactive // all tests interactively
npm run test-e2e interactive FILE_NAME // one test file interactively
npm run test-e2e:watch interactive // all tests interactively and watch for changes
npm run test-e2e:watch interactive FILE_NAME // one test file interactively and watch for changes

How has this been tested?

Ran locally with the interactive command and appears to work as expected.

Note

@talldan suggested we'd have

npm run test-e2e:watch --interactive

but the only way to have that is if we'd also append more '--' as in

npm run test-e2e:watch -- --interactive

and I figured it'd be too many dashes, plus when passed to jest it would throw an unrecognised arg error so we'd have clean it before.

Just using the word seemed to work out of the box, maybe ;)

[draganescu]

@gziolo travis is failing again, would my change break anything other than this simple command?

[ntwb]

I've restarted the tests, failures appear unrelated to this PR

[talldan]

I had to restart the one test again—passing now!

[aduth]

but the only way to have that is if we'd also append more '--' as in

This is consistent though with other usage / recommendations though:

https://github.com/WordPress/gutenberg/blob/master/docs/contributors/testing-overview.md#tldr-broken-snapshots

gutenberg/package.json

Lines 188 to 189 in fe2136d

	"test-unit:update": "npm run test-unit -- --updateSnapshot",
	"test-unit:watch": "npm run test-unit -- --watch",

Personally, if it's an argument, it feels more appropriate to use a standard arguments syntax. It feels a bit awkward initially, but this is the standard way of passing arguments to the underlying command of any npm script.

Related: npm/npm#5518

plus when passed to jest it would throw an unrecognised arg error so we'd have clean it before

It's an interesting point, since we have options supported by our abstraction, and options to pass through to Jest. Arguably, we should be explicit with our interface in such a way that we only pass through arguments either formed in our script handler, or whitelisted to be handled by Jest. But this is already at odds with how we use the test scripts, where as seen above we pass through Jest's --updateSnapshot and --watch arguments.

I don't really have a good solution on mind at the moment.

[gziolo]

npm run test-e2e:watch -- --interactive

Yes, this is the way how it is handled with npm. At the same time you can always use:

./node_modules/.bin/wp-scripts test-e2e --interactive

and everything is going to work as expected.

[gziolo]

It doesn't work the way it was proposed:

> gutenberg@5.1.1 test-e2e /Users/gziolo/PhpstormProjects/gutenberg
> wp-scripts test-e2e --config packages/e2e-tests/jest.config.js "interactive"

No tests found, exiting with code 1
Run with `--passWithNoTests` to exit with code 0
In /Users/gziolo/PhpstormProjects/gutenberg/packages/e2e-tests
  250 files checked.
  testMatch: **/__tests__/**/*.js,**/?(*.)(spec|test).js,**/test/*.js - 62 matches
  testPathIgnorePatterns: /node_modules/ - 250 matches
  testRegex:  - 250 matches
Pattern: interactive - 0 matches

The issue is that interactive is passed as a regular CLI param and Jest processes it as it was file pattern.

The only way to make it work is what @aduth shared:

Arguably, we should be explicit with our interface in such a way that we only pass through arguments either formed in our script handler, or whitelisted to be handled by Jest. But this is already at odds with how we use the test scripts, where as seen above we pass through Jest's --updateSnapshot and --watch arguments.

This command integrates both Jest and Puppeteer so it becomes a bit complex in comparison to all other commands. One way of making it easier to whitelist would be to prefix all custom arguments with --puppeteer-.

[draganescu]

@gziolo I have implemented the argument as @aduth suggested with the extra dashes. Now the command can optionally be run as:

npm run test-e2e:watch -- --puppeteer-interactive --puppeteer-slowmo=10

Also, I left the speed at 80 as a default, and also, see above, added a new argument for changing the speed. To solve the problems of passing all new arguments to Jest I added a small new function that removes arguments with certain prefixes from the list of arguments that is passed to Jest.

[draganescu]

Another option would to not filter by prefix, but instead by command name. This way we would have shorter arguments for example:

npm run test-e2e:watch -- --interactive --slowmo=10

and in the cleanUpArgs function I could just take each one out. What do you think @gziolo @aduth ?

[gziolo]

Another option would to not filter by prefix, but instead by command name. This way we would have shorter arguments for example:

npm run test-e2e:watch -- --interactive --slowmo=10

and in the cleanUpArgs function I could just take each one out.

Yes, that would work, too. I don't feel strongly about any of this.

[draganescu]

@gziolo I have cleaned up the functions by moving the argument removal by prefix into the getCliArgs function of the process util. I've also rebased. We should be good to go :)

[gziolo]

Nice one, we still need to document these new features. I will open a follow-up with more documentation improvements.

[youknowriad]

Awesome work @draganescu