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.

Sign up for GitHub

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

E2E tests: Introduce insertBlock() util #7013

Merged
merged 1 commit into from
May 30, 2018
Merged

E2E tests: Introduce insertBlock() util #7013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions test/e2e/specs/adding-blocks.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Internal dependencies
*/
import '../support/bootstrap';
import { newPost, newDesktopBrowserPage } from '../support/utils';
import { newPost, newDesktopBrowserPage, insertBlock } from '../support/utils';

describe( 'adding blocks', () => {
beforeAll( async () => {
Expand Down Expand Up @@ -49,12 +49,7 @@ describe( 'adding blocks', () => {
await page.keyboard.type( 'Quote block' );

// Using the regular inserter
await page.click( '.edit-post-header [aria-label="Add block"]' );
await page.waitForSelector( '.editor-inserter__menu' );
await page.keyboard.type( 'code' );
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Enter' );
await insertBlock( 'code' );
await page.keyboard.type( 'Code block' );

// Unselect blocks to avoid conflicts with the inbetween inserter
Expand Down
14 changes: 3 additions & 11 deletions test/e2e/specs/multi-block-selection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Internal dependencies
*/
import '../support/bootstrap';
import { newPost, newDesktopBrowserPage, pressWithModifier } from '../support/utils';
import { newPost, newDesktopBrowserPage, pressWithModifier, insertBlock } from '../support/utils';

describe( 'Multi-block selection', () => {
beforeAll( async () => {
Expand All @@ -19,16 +19,8 @@ describe( 'Multi-block selection', () => {
// Creating test blocks
await page.click( '.editor-default-block-appender' );
await page.keyboard.type( 'First Paragraph' );
await page.click( '.edit-post-header [aria-label="Add block"]' );
await page.keyboard.type( 'Image' );
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Enter' );
await page.click( '.edit-post-header [aria-label="Add block"]' );
await page.keyboard.type( 'Quote' );
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Enter' );
await insertBlock( 'Image' );
await insertBlock( 'Quote' );
await page.keyboard.type( 'Quote Block' );

const blocks = [ firstBlockSelector, secondBlockSelector, thirdBlockSelector ];
Expand Down
8 changes: 2 additions & 6 deletions test/e2e/specs/splitting-merging.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Internal dependencies
*/
import '../support/bootstrap';
import { newPost, newDesktopBrowserPage } from '../support/utils';
import { newPost, newDesktopBrowserPage, insertBlock } from '../support/utils';

describe( 'splitting and merging blocks', () => {
beforeAll( async () => {
Expand All @@ -12,11 +12,7 @@ describe( 'splitting and merging blocks', () => {

it( 'Should split and merge paragraph blocks using Enter and Backspace', async () => {
//Use regular inserter to add paragraph block and text
await page.click( '.edit-post-header [aria-label="Add block"]' );
await page.keyboard.type( 'paragraph' );
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Enter' );
await insertBlock( 'paragraph' );
await page.keyboard.type( 'FirstSecond' );

//Move caret between 'First' and 'Second' and press Enter to split paragraph blocks
Expand Down
17 changes: 17 additions & 0 deletions test/e2e/support/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,23 @@ export async function getHTMLFromCodeEditor() {
return textEditorContent;
}

/**
* Opens the inserter, searches for the given term, then selects the first
* result that appears.
*
* @param {string} searchTerm The text to search the inserter for.
*/
export async function insertBlock( searchTerm ) {
await page.click( '.edit-post-header [aria-label="Add block"]' );
// Waiting here is necessary because sometimes the inserter takes more time to
Copy link
Member

Choose a reason for hiding this comment

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

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. 🙈

Copy link
Member Author

@noisysocks noisysocks Jun 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

My understanding1 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:

screen shot 2018-06-01 at 10 58 28

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.

1 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.

Copy link
Member Author

@noisysocks noisysocks Jun 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@noisysocks noisysocks Jun 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

[...] 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.

// render than Puppeteer takes to complete the 'click' action
await page.waitForSelector( '.editor-inserter__menu' );
await page.keyboard.type( searchTerm );
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Enter' );
}

/**
* Performs a key press with modifier (Shift, Control, Meta, Mod), where "Mod"
* is normalized to platform-specific modifier (Meta in MacOS, else Control).
Expand Down