Try to rewrite e2e tests using jest-puppeter preset #8301

gziolo · 2018-07-30T15:42:27Z

Description

Closes #8619.

Improves #6956. See known issues to find out what is still necessary to make it bulletproof.

Tries to fix intermittent e2e test failures. In particular an error with preview test:

Starting to see a new one.

FAIL test/e2e/specs/preview.test.js
  ● Preview › Should open a preview window for a new post
    Protocol error (Runtime.callFunctionOn): Cannot find context with specified id undefined
      
      at Promise (node_modules/puppeteer/lib/Connection.js:198:56)
      at CDPSession.send (node_modules/puppeteer/lib/Connection.js:197:12)
      at ExecutionContext.evaluateHandle (node_modules/puppeteer/lib/ExecutionContext.js:71:75)
      at ExecutionContext.evaluate (node_modules/puppeteer/lib/ExecutionContext.js:46:31)
      at Frame.evaluate (node_modules/puppeteer/lib/FrameManager.js:299:20)

Example: #7841 https://travis-ci.org/WordPress/gutenberg/jobs/401939944

This is try to use jest-puppeteer preset recommended in Jest docs:

https://jestjs.io/docs/en/puppeteer#use-puppeteer-preset

The biggest difference is that we no longer use isolated browser instance per test suite but share one browser to speed up tests execution.

It also enables some new methods to expect API. See:
https://github.com/smooth-code/jest-puppeteer/tree/master/packages/expect-puppeteer#api

How has this been tested?

npm run test-e2e

Knows issues

We should open a follow up tasks for two issues.

Preview opens new tab after publish

Steps to reproduce:

Type something in the title field.
Click the preview button.
Go back to the editor.
Publish post.
Click the preview button.
Go back to the editor.
Type something in the title field.
Click the preview button.
It shouldn't open a new tab. It should reuse the existing one.

Too many undo levels

It happens only in the headless mode when running e2e tests. Sometimes it creates two undo levels for the first paragraph. When you modify test and add snapshots after each undo action you might see the following:

exports[`undo Should undo to expected level intervals 5`] = `
“<!-- wp:paragraph -->
<p>This</p>
<!-- /wp:paragraph -->”
`;

exports[`undo Should undo to expected level intervals 6`] = `
“<!-- wp:paragraph -->
<p>Thi</p>
<!-- /wp:paragraph -->”
`;

This isn't expected behavior. It uncovers some edge case happening inside the logic which creates undo levels.

gziolo · 2018-07-30T16:03:41Z

test/e2e/specs/preview.test.js

 		expect( previewPage.url() ).toBe( expectedPreviewURL );

 		// Return to editor to change title.
 		await editorPage.bringToFront();
 		await editorPage.type( '.editor-post-title__input', ' And more.' );

 		// Published preview should reuse same popup frame.
-		previewPage = await getOpenedPreviewPage();
+		// TODO: Fix code to reuse the same frame!
+		previewPage = await getOpenedPreviewPage( editorPage );


This is the only place where we don't reuse the same frame. Very surprising given given that there is code comment which states it 😃

gziolo · 2018-07-31T08:53:11Z

Travis run for e2e tests is back to 7 minutes!

This is what I see locally:

Test Suites: 23 passed, 23 total
Tests:       3 skipped, 66 passed, 69 total
Snapshots:   15 passed, 15 total
Time:        93.87s, estimated 95s
Ran all test suites

youknowriad · 2018-07-31T10:57:54Z

test/e2e/specs/nux.test.js

-
-	afterEach( async () => {
-		// Clear localStorage tips so they aren't persisted for the next test.
-		await clearLocalStorage();


Is this not needed anymore?

We clear localStorage after every test suite. In addition, when opening the editor and enableTips flag is passed we clean all disabled tips and reload the page.

youknowriad · 2018-07-31T10:58:12Z

test/e2e/specs/multi-block-selection.test.js

@@ -17,7 +15,7 @@ describe( 'Multi-block selection', () => {
 		const multiSelectedCssClass = 'is-multi-selected';

 		// Creating test blocks
-		await page.click( '.editor-default-block-appender' );
+		await page.click( '.editor-default-block-appender__content' );


Just curious why this change?

For consistency, we probably should wrap it with a utility method. Initially, I suspected that it might be the reason why undo test fails sometimes.

This updated class is the one which has the events bound, so it's more "correct".

youknowriad · 2018-07-31T10:59:37Z

test/e2e/specs/reusable-blocks.test.js

 	newPost,
 	pressWithModifier,
 	searchForBlock,
 } from '../support/utils';

 function waitForAndAcceptDialog() {
 	return new Promise( ( resolve ) => {
-		page.once( 'dialog', async ( dialog ) => {
-			await dialog.accept();


Why are we removing this?

We have global handler so you don't need to accept it twice.

Can we remove the entire waitForAndAcceptDialog function, then?

We use this Promise, so not quite sure 🤷‍♂️

youknowriad · 2018-07-31T11:00:37Z

test/e2e/specs/sidebar-behaviour.test.js

@@ -66,8 +61,7 @@ describe( 'Publishing', () => {

 	it( 'Should reopen sidebar the sidebar when resizing from mobile to desktop if the sidebar was closed automatically', async () => {
 		await setViewport( 'large' );
-		await newPost();
-		await setViewport( 'small' );
+		await newPost( { viewport: 'small' } );


I think it's weird to have an option to set the viewport size as part of the newPost helper, I prefer a separate helper for that.

I can revert those changes. Previously, we were setting large viewport using newDesktopBrowserPage helper.

Reverted 👍

youknowriad · 2018-07-31T11:02:53Z

test/e2e/support/setup-test-framework.js

-		headless: PUPPETEER_HEADLESS !== 'false',
-		slowMo: parseInt( PUPPETEER_SLOWMO, 10 ) || 0,
-	} );
+	enablePageDialogAccept();


I don't like that we're accepting dialogs globally, I prefer the explicitness we had before, because sometimes we want to test these dialogs

We need for the majority of test suites as far as I discovered today. There is disablePageDialogAccept method which would be useful in case when you want to handle it yourself. I'm happy to refactor, but then we would have to put it for every test which types something.

Ok 👍 It's fine

youknowriad · 2018-07-31T11:09:28Z

I'd appreciate if we extend our testing docs with steps to:

Run a unique test with a custom environment
Run a custom environment
Watch a unique test if possible?

Just trying to think of alternative workflows (other than CI) needed to debug and build tests.

gziolo · 2018-07-31T11:54:54Z

I brought back explicit calls for setViewport with 78825c0 as suggested by @youknowriad.

I also added a change which ensures npm run test-e2e is executed with the latest codebase with aecf753 as suggested by @notnownikki.

gziolo · 2018-07-31T11:59:26Z

@youknowriad I opened a follow-up issue for your comment about documentation - #8319.

gziolo · 2018-07-31T12:43:09Z

Added another small tweak which improves the trigger which wraps up after tests cleanup: 384cf5e.

notnownikki · 2018-07-31T20:23:29Z

Pushed up some code in 5ce6245 that makes the preview test pass consistently everywhere I try it.

Looked at this, and there's a problem with using the targetcreated event, because sometimes the listeners don't get triggered quickly enough and the test carries on without getting the target, and we get a crash. It's not just us getting caught by this, but popups are considered an edge case so it's not high priority to fix. See berstend/puppeteer-extra#6

Because we can't reliably get the created target, I've introduced a waitForTab which waits for the number of tabs to change, so we know we've got the preview tab open.

I also changed the test to only reuse the preview page in tests that we explicitly check for reuse behaviour. There are also assertions that we have the expected number of tabs open at all times.

The test passes consistently for me now, both with the browser and in headless mode (both of which resulted in different race conditions in the original test - it used to be possible for the test to fail because it thought the preview page had reloaded with updated content, but it hadn't yet even though it was in the process of doing so).

noisysocks

Left a few minor comments. Would love to get this in very soon—it's hard to review PRs when they're all 🔴! 😅

noisysocks · 2018-08-01T00:54:09Z

test/e2e/specs/preview.test.js

-		}
+	function sleep( time ) {
+		return new Promise( ( resolve ) => setTimeout( resolve, time ) );
+	}


Puppeteer already provides this function in the form of page.waitFor.

Also, I think we discussed this previously and the conclusion is to prefer waitForSelector over waitFor/sleep, since it's more reliable.

You can't wait for a selector on a page that doesn't exist yet though, you have to have the page instance, and that's what this is for.

Ok, that's fine, can we add an inline comment with this explanation?

Sure, in fact I've already changed this code locally and used the editor page to call waitFor, with an inline comment :)

noisysocks · 2018-08-01T00:55:44Z

test/e2e/specs/preview.test.js

 		expect( previewPage.url() ).toBe( expectedPreviewURL );

 		// Return to editor to change title.
 		await editorPage.bringToFront();
 		await editorPage.type( '.editor-post-title__input', ' And more.' );

 		// Published preview should reuse same popup frame.
-		previewPage = await getOpenedPreviewPage();
+		// TODO: Fix an existing bug which opens a new tab.


Can we create an issue to track this work?

#8330 - thanks :)
Issue is filed: #8318.

noisysocks · 2018-08-01T00:57:08Z

test/e2e/specs/reusable-blocks.test.js

 	newPost,
 	pressWithModifier,
 	searchForBlock,
 } from '../support/utils';

 function waitForAndAcceptDialog() {
 	return new Promise( ( resolve ) => {
-		page.once( 'dialog', async ( dialog ) => {
-			await dialog.accept();


Can we remove the entire waitForAndAcceptDialog function, then?

noisysocks · 2018-08-01T01:05:05Z

Heads up that #8330 contains a fix for the preview E2E test as well.

gziolo · 2018-08-08T10:40:54Z

It passed 2 times after recent tweaks. We should merge as soon as someone confirms I didn't break anything else 😅

notnownikki · 2018-08-08T10:50:44Z

🚢

Works well on my poor little laptop even when it's under load! This is a huge improvement. Thank you!

gziolo · 2018-08-08T11:05:37Z

Thank you all for commits, reviews, and testing. Let's iterate on this as it is still not perfect 🚀

gziolo force-pushed the fix/e2e-test-preview branch from 0683a23 to 249807f Compare July 30, 2018 15:52

gziolo self-assigned this Jul 30, 2018

gziolo added [Status] In Progress Tracking issues with work in progress [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Jul 30, 2018

gziolo commented Jul 30, 2018

View reviewed changes

gziolo force-pushed the fix/e2e-test-preview branch from 2e26a08 to 8d46c2f Compare July 30, 2018 16:27

gziolo removed the [Status] In Progress Tracking issues with work in progress label Jul 31, 2018

gziolo requested review from aduth, tofumatt, youknowriad, pento, a team and jorgefilipecosta July 31, 2018 10:10

gziolo force-pushed the fix/e2e-test-preview branch from a95eae1 to 69126e1 Compare July 31, 2018 10:18

This was referenced Jul 31, 2018

Undo / Redo inconsistencies when using mouse or keyboard #5685

Closed

Clicking Preview button opens a new tab after publish #8318

Closed

youknowriad reviewed Jul 31, 2018

View reviewed changes

gziolo mentioned this pull request Jul 31, 2018

Documentation: Extend our testing docs with details about e2e commands #8319

Closed

noisysocks reviewed Aug 1, 2018

View reviewed changes

gziolo and others added 19 commits August 8, 2018 11:50

Testing: Improve preview e2e test

53febfe

Tests: Use explicit setViewport calls

52307f1

Build: Ensure npm run build is always executed before e2e tests

3c3bd29

Tests: Change the trigger wchich finishes after test cleanup

e6f7a7b

Stop using targetcreated, see berstend/puppeteer-extra#6

6d8ade1

lint fix

442096a

Use waitFor instead of custom sleep

0f26dae

Fix conflicts with master

f2dd588

Lint fixes

3bd092a

Update comment on race condition workaround

9e050a8

Tests: Improve lifecycle methods for e2e setup

2cf655b

Send more mousemove events, wait for selector before clicking

63c49b9

chore: Improve variable name

33c9e99

chore: Add colon to "See" comment

aee99e5

chore: Tweak comments in undo test

107ba2e

chore: Move puppeteer config to e2e folder

0e7977c

chore: Introduce new e2e helper for clicking block appender

d8c1f97

Tests: Improve stability of e2e tests

d29fe22

tests: Make e2e tests work with the lastest master changes

6687d0a

gziolo force-pushed the fix/e2e-test-preview branch from 6e7f7de to 6687d0a Compare August 8, 2018 10:14

tests: Add puppeteer config back

299088e

gziolo merged commit 1b9af37 into master Aug 8, 2018

gziolo deleted the fix/e2e-test-preview branch August 8, 2018 11:04

gziolo added this to the 3.5 milestone Aug 8, 2018

gziolo added the [Type] Enhancement A suggestion for improvement. label Aug 8, 2018

gziolo mentioned this pull request Aug 8, 2018

tests: e2e test for the writing flow started to fail #8712

Closed

aduth mentioned this pull request Aug 13, 2018

Intermittently, multiple preview tabs are opened in the preview e2e test #8367

Closed

Try to rewrite e2e tests using jest-puppeter preset #8301

Try to rewrite e2e tests using jest-puppeter preset #8301

Conversation

gziolo commented Jul 30, 2018 • edited Loading

Description

How has this been tested?

Knows issues

Preview opens new tab after publish

Too many undo levels

Choose a reason for hiding this comment

gziolo commented Jul 31, 2018

Choose a reason for hiding this comment

gziolo Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Jul 31, 2018

gziolo commented Jul 31, 2018

gziolo commented Jul 31, 2018

gziolo commented Jul 31, 2018

notnownikki commented Jul 31, 2018

noisysocks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisysocks commented Aug 1, 2018

gziolo commented Aug 8, 2018

notnownikki commented Aug 8, 2018

gziolo commented Aug 8, 2018

gziolo commented Jul 30, 2018 •

edited

Loading

gziolo Jul 31, 2018 •

edited

Loading