E2E tests: Introduce insertBlock() util

[noisysocks]

Addresses #6995 (comment) and fixes #6956.

See #6995 for some necessary background on all this.

[gziolo]

Nice refactoring 👍

[danielbachhuber]

💯 Thanks @noisysocks

[aduth]

sometimes the inserter takes more time to render than Puppeteer takes to complete the 'click' action

When and why does this occur? Sounds like a legitimate bug that we're pretending doesn't exist with the workaround here. 🙈

[noisysocks]

My understanding¹ is that the "bug" is that we're treating click() as if it weren't a very low-level API.

Here's what click() does:

1. Gets the centre point of the given element's bounding box
2. Moves the mouse to that position by sending Input.dispatchMouseEvent( { x, y, type: 'mouseMoved' } ) via the DevTools protocol
3. Sends Input.dispatchMouseEvent( { type: 'mousePressed' } ) via the DevTools protocol
4. If options.delay is specified, waits for that amount of time
5. Sends Input.dispatchMouseEvent( { type: 'mouseReleased' } ) via the DevTools protocol

The promise returned by click() is resolved when step 5 is completed. Each step takes a few milliseconds while Puppeteer waits for the DevTools protocol to send back an ACK message.

In other words, click() returns a promise that resolves after Chrome processes ~5 WebSocket messages. This may be slow or fast, but its speed depends on how busy Chrome's DevTools protocol server is and not on what our web application is doing.

We're treating click() as if it were jQuery.click()—a method that can be blocked when the web application is painting a frame or executing a script. But this is not true.

We ordinarily don't see any problems to do with using this low-level API because, normally, our UI responds to a click in less time than Chrome takes to process ~5 messages. But the inserter is fairly complex.

I measured this just now on my local machine and it took about 65 ms to render the inserter and have its search field focused:

I then measured that click() takes anywhere from 40 to 80 ms by inserting the following into the E2E test:

const start = Date.now();
await page.click( '.edit-post-header [aria-label="Add block"]' );
const end = Date.now();
console.log( end - start );

These times are very similar to each other, and so I'm convinced that this is why we were seeing intermittent test failures.

---

With all that in mind, some possible solutions:

• Tell Puppeteer to wait for the inserter selector to appear after sending the click() action and before continuing with the test. This is what I opted for.
• Pass in a delay option when calling click, e.g. page.click( '.edit-post-header [aria-label="Add block"]', { delay: 30 } ). I wasn't comfortable with this solution since it could still lead to an intermittent failure if the application is slow enough for whatever reason. It could also make our tests take longer to run.
• Speed up the inserter e.g. by deferring the rendering of its menu items until after the main components and search field has rendered. This would be nice since it would also make the UI feel more responsive for folks with slow CPUs, but I didn't opt for this option since it requires the most effort.

---

¹ Please note that Gutenberg is my first experience in using Puppeteer and this is all based on the hour or so that I spent digging into the source code and API docs. I could not find documentation which explains exactly how all of this works. If we want to learn more, my next step would be asking around in the DevTools protocol mailing list.

[noisysocks]

Aside: @pento just noted that we likely end up with a list block because the search field appears while code is being typed. Note that if you search de then the first result is the list block.

[aduth]

Wow, thanks for the thorough investigation and explanation @noisysocks ! Definitely helps shed light on what's going on here, and explains one of the most common issues that's been encountered with Puppeteer, which is race conditions between interactions and their expected effects. It's alluded to briefly in their documentation. Their suggestion is to define the expected waitFor* with the interaction in a Promise.all.

With your explanation, my hunch is that we'd still potentially have issues if the inserter, for whatever reason, started being rendered very quickly, since there might be the possibility that it's already rendered by the time we call waitForSelector in the tests? Thus it would hang forever because waitForSelector does not resolve itself if the selector predicate is already matched on the page.

Would something like this be an improvement then?

await Promise.all( [
	page.waitForSelector( '.editor-inserter__menu' ),
	page.click( '.edit-post-header [aria-label="Add block"]' ),
] );

To my general motivation for leaving a comment: I just want to make sure that the UI is not being rendered after some delay, where a test which otherwise would fail because it moves too fast is subject to the same breakage for a particularly fast user (or a user on a slow computer). That doesn't seem to be what's happening here. Though it's unfortunate we have to author tests in such a way where we're "waiting" for selectors merely due to the mechanics of the DevTools protocol messaging.

I measured this just now on my local machine and it took about 65 ms to render the inserter and have its search field focused: [...] Speed up the inserter e.g. by deferring the rendering of its menu items until after the main components and search field has rendered. This would be nice since it would also make the UI feel more responsive for folks with slow CPUs, but I didn't opt for this option since it requires the most effort.

I'd say we don't need to pursue this as a solution for testing in particular, and I don't think it'd be very bulletproof anyways, but that 65ms to render the Inserter is, in general, unacceptable, and should be improved anyways.

[noisysocks]

With your explanation, my hunch is that we'd still potentially have issues if the inserter, for whatever reason, started being rendered very quickly, since there might be the possibility that it's already rendered by the time we call waitForSelector in the tests?

This shouldn't be an issue because, according to the docs, waitForSelector will resolve immediately if the selector already exists.

The Promise.all example that they have in the docs is for the very specific case where a click() results in page navigation. In this case, it's important to call waitForNavigation() before the navigation occurs.

Though it's unfortunate we have to author tests in such a way where we're "waiting" for selectors merely due to the mechanics of the DevTools protocol messaging.

I actually did some more experimenting after writing my comment above because I wasn't satisfied with the seemingly scary proposition that we have to call waitFor* after every click() action.

It turns out that it's not necessary to do that because Chrome usually buffers events that occur while the UI is blocked due to a slow script. That is, it's normally safe to call click() or type() immediately after rendering a slow component because Chrome will wait for the frame to render before doing the clicking or typing. You can see this for yourself by repeatedly clicking on the inserter toggle: no matter how fast you do it, an even amount of clicks will result in it being closed.

The reason that we have to waitForSelector in this particular case is because type() relies on having the search <input> selected and Chrome doesn't focus the input until the next frame. You can see that behaviour in the screenshot I posted above: note that there's a single frame where the inserter has rendered but the input has no blue focus ring around it.

To confirm this I created this weird test HTML page. 'Click me' burns CPU cycles for 500 ms and then renders a div. The div contains another button and an auto-focused text field.

Note that:

• If you click 'Click me' and then immediately click where 'Now click me' will appear, everything works fine. Chrome buffers the click because the frame was blocked
• If you click 'Click me' and then start typing some stuff, the text field will only receive the last part of what you type. If you record this in the Performance tab, you can see that the text area isn’t focused until the frame after the div appears

tl;dr: Use waitForSelector() when you are relying on something that takes more than one frame, e.g. a network request, an autofocus, a setTimeout(..., 0)

but that 65ms to render the Inserter is, in general, unacceptable, and should be improved anyways.

I wonder if async rendering will help with this when it's enabled in React.

[aduth]

[...] I wasn't satisfied with the seemingly scary proposition that we have to call waitFor* after every click() action.

tl;dr: Use waitForSelector() when you are relying on something that takes more than one frame, e.g. a network request, an autofocus, a setTimeout(..., 0)

This is super helpful information, as I was starting to find myself sliding into the assumption to use waitForSelector on basically every interaction, as I had seen enough unexpected cases where it was needed to find it most reliable.