Add regression E2E test for the classic block initialization issue #25169

fullofcaffeine · 2020-09-09T02:03:49Z

Description

Adds a regression test in the client block E2E spec to avoid the regression of this issue: #24696, as suggested in #25162 (comment).

How has this been tested?

Check out this branch, and apply the following diff (effectively reverting #25162 and #25163):

diff --git a/packages/block-library/src/classic/edit.js b/packages/block-library/src/classic/edit.js
index c8da3b1b6d..8195e51006 100644
--- a/packages/block-library/src/classic/edit.js
+++ b/packages/block-library/src/classic/edit.js
@@ -54,11 +54,7 @@ export default class ClassicEdit extends Component {
                if ( document.readyState === 'complete' ) {
                        this.initialize();
                } else {
-                       document.addEventListener( 'readystatechange', () => {
-                               if ( document.readyState === 'complete' ) {
-                                       this.initialize();
-                               }
-                       } );
+                       window.addEventListener( 'DOMContentLoaded', this.initialize );
                }
        }
 
@@ -74,7 +70,7 @@ export default class ClassicEdit extends Component {
                } = this.props;
 
                const editor = window.tinymce.get( `editor-${ clientId }` );
-               const currentContent = editor?.getContent();
+               const currentContent = editor.getContent();
 
                if (
                        prevProps.attributes.content !== content &&

Run npm build and npm run wp-env-start.
Make sure the Gutenberg plugin is active in your local test instance. That's localhost:8889 -- log into wp-admin to verify (credentials: admin/password)
Run npm run test-e2e -- packages/e2e-tests/specs/editor/blocks/classic.test.js. (To watch tests in interactive mode, append --puppeteer-interactive).
Verify that the Classic › Should not fail after save/reload test fails with TypeError: Cannot read property 'getContent' of null.
Now remove the patch (git checkout -- packages/block-library/src/classic/edit.js), and rebuild (npm run build).
Re-run the e2e test. It should pass this time.

Types of changes

New E2E test.

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.
I've updated all React Native files affected by any refactorings/renamings in this PR.

sirreal · 2020-09-09T08:32:25Z

This is failing for me now with:


 FAIL  packages/e2e-tests/specs/editor/blocks/classic.test.js (29.11s)
  Classic
    ✕ Should not fail after save/reload (16216ms)
    ○ skipped should be inserted
    ○ skipped should insert media, convert to blocks, and undo in one step

  ● Classic › Should not fail after save/reload

    TimeoutError: Element .edit-post-more-menu [aria-label="Options"] not found

      waiting for function failed: timeout 1300ms exceeded

ockham · 2020-09-09T17:35:17Z

Some notes:

The e2e test can be run locally via npm run wp-env start, followed by npm run test-e2e -- packages/e2e-tests/specs/editor/blocks/classic.test.js. To watch them in interactive mode, append --puppeteer-interactive to the latter command.

I can repro the issue Jon is seeing. Apparently, switching between Visual and Code Editor isn't working.

This test should fail (earlier) if we revert 65c9f74 (that's #25162), and run npm run build. However, it doesn't seem to 🤔

This is a screenshot I took while running npm run test-e2e -- packages/e2e-tests/specs/editor/blocks/classic.test.js --puppeteer-interactive:

It appears that the Gutenberg plugin isn't active during this e2e test, and that we're thus testing against the editor version that comes with Core, rather than the current GB codebase 🤔 I wonder if that's because we're running this test in isolation (and missing some 'global' setup function that enables the plugin), or whether this is a glitch with wp-env on Linux (which both @fullofcaffeine and I are using), or whether this currently affects all e2e tests 😕

ockham · 2020-09-09T18:02:18Z

Update, as Riad explained to me in Slack, the Gutenberg plugin has to be activated manually in the WP instance used for testing (at localhost:8889, credentials: admin/password).

ockham · 2020-09-09T19:17:41Z

We've found that we need to disable Puppeteer cache before reloading the editor in order to repro the issue. The following works:

diff --git a/packages/e2e-tests/specs/editor/blocks/classic.test.js b/packages/e2e-tests/specs/editor/blocks/classic.test.js
index ce450cd172..d2e9ee06df 100644
--- a/packages/e2e-tests/specs/editor/blocks/classic.test.js
+++ b/packages/e2e-tests/specs/editor/blocks/classic.test.js
@@ -131,6 +131,7 @@ describe( 'Classic', () => {
                await saveDraft();
 
                // reload
+               await page.setCacheEnabled( false );
                await page.reload();
 
                await clickClassic();

We need to investigate if that turns off caching for all subsequent tests, and if yes, how we can restore the previous caching state.

fullofcaffeine · 2020-09-09T22:29:34Z

As @ockham mentioned, by disabling the cache, the regression is reproducible. It's not if the cache is left enabled (default).

You can test it by reverting the commit 65c9f74 and 25e6ae, building GB again, and running this specific test.

I've chosen to go the simpler route of disabling, running the relevant steps to that reproduce the regression, then enabling the cache again. I assume it should work fine unless we have some kind of page object leakage in parallel tests, but I'm assuming that's not the case (I also don't know much about the Gutenberg testing infrastructure yet).

If anyone knows a better way to solve this or why browser caching causes the regression not to manifest in the E2E testing environment, please share your thoughts, we'd be keen to better understand why disabling the cache is needed in this case.

packages/e2e-tests/specs/editor/blocks/classic.test.js

fullofcaffeine · 2020-09-09T23:09:21Z

With the merge of #25163, if you want to reproduce this issue, you also need to revert 25e6ae3, in addition to 65c9f74.

ockham · 2020-09-10T10:56:04Z

packages/e2e-tests/specs/editor/blocks/classic.test.js

@@ -46,6 +47,7 @@ describe( 'Classic', () => {
 		// Click the image button.
 		await page.waitForSelector( 'div[aria-label^="Add Media"]' );
 		await page.click( 'div[aria-label^="Add Media"]' );
+


Suggested change

ockham · 2020-09-10T10:56:54Z

packages/e2e-tests/specs/editor/blocks/classic.test.js

+		// Might move to utils if this becomes useful enough for other tests
+		const runWithoutCache = async ( cb ) => {
+			await page.setCacheEnabled( false );
+			await cb();
+			await page.setCacheEnabled( true );
+		};


TBH I was considering just inlining this, since it's really just two (fairly intuitive) commands. Might not make much sense to encapsulate them.

We need to make sure that page.setCacheEnabled( true ) runs at the end even if the first bits throw or reject.

Ah, good point!

Ah yes, good point indeed!

TBH I was considering just inlining this

Yeah, fine with me, we don't really use it anywhere else so you have a point.

Besides the actual method to disable and enable the cache is self-descriptive, the additional helper function doesn't add much in terms of meaning.

ockham · 2020-09-10T13:27:29Z

I updated the PR desc with some step-by-step instructions.

fullofcaffeine · 2020-09-10T14:40:22Z

I updated the PR desc with some step-by-step instructions.

Thanks!

ockham · 2020-09-10T14:40:28Z

Static Analysis job is failing. Fix: #25223

ockham · 2020-09-10T15:32:20Z

#25223 has been merged. I'll rebase this one.

Issue: #24696

Considering commit 65c9f74 is reverted, the only actions needed to reproduce the issue are to: * Add the block to the editor, add some content; * Save; * Reload the editor; * Click it. It should error.

Co-authored-by: Bernie Reiter <[email protected]>

ockham

Thanks a lot for working on this, @fullofcaffeine! Let's merge this once all checks are green 👍

github-actions · 2020-09-10T16:30:11Z

Congratulations on your first merged pull request, @fullofcaffeine! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

fullofcaffeine force-pushed the fullofcaffeine:add/regression-e2e-test-for-classic-block-issue branch from 69f2e86 to ba37a23 Sep 9, 2020

sirreal added [Block] Classic [Package] E2E Tests [Status] In Progress labels Sep 9, 2020

sirreal assigned fullofcaffeine Sep 9, 2020

fullofcaffeine force-pushed the fullofcaffeine:add/regression-e2e-test-for-classic-block-issue branch from ba37a23 to b077a49 Sep 9, 2020

fullofcaffeine marked this pull request as ready for review Sep 9, 2020

fullofcaffeine requested review from ajitbohra, nerrad, ntwb and talldan as code owners Sep 9, 2020

ockham reviewed Sep 9, 2020

View changes

packages/e2e-tests/specs/editor/blocks/classic.test.js Outdated Show resolved Hide resolved

fullofcaffeine force-pushed the fullofcaffeine:add/regression-e2e-test-for-classic-block-issue branch from 6795b88 to 1bd8f38 Sep 9, 2020

ockham reviewed Sep 10, 2020

View changes

fullofcaffeine and others added 5 commits Sep 9, 2020

Add regression E2E test for the classic block initialization issue

1f4e551

Issue: #24696

Simplify classic block initialization issue regression test scenario

a1af5a6

Considering commit 65c9f74 is reverted, the only actions needed to reproduce the issue are to: * Add the block to the editor, add some content; * Save; * Reload the editor; * Click it. It should error.

Move non-relevant lines out of the `runWithoutCache`

4dbe863

Co-authored-by: Bernie Reiter <[email protected]>

Remove obsolete snapshot

56c55f4

Remove unused import

d300c6f

ockham added 3 commits Sep 10, 2020

Rename selector

32f0998

Use focus instead of click, inline

a96c8db

Add exception handling

Loading status checks…

748c799

ockham force-pushed the fullofcaffeine:add/regression-e2e-test-for-classic-block-issue branch from 6469459 to 748c799 Sep 10, 2020

ockham approved these changes Sep 10, 2020

View changes

github-actions bot added the First-time Contributor label Sep 10, 2020

github-actions bot added this to the Gutenberg 9.0 milestone Sep 10, 2020

fullofcaffeine deleted the fullofcaffeine:add/regression-e2e-test-for-classic-block-issue branch Sep 10, 2020

ockham mentioned this pull request Sep 14, 2020

Avoid relying on DOM events to measure the loading time #25288

Merged

WordPress / gutenberg

Add regression E2E test for the classic block initialization issue #25169

Add regression E2E test for the classic block initialization issue #25169

fullofcaffeine commented Sep 9, 2020 •

edited by ockham

sirreal commented Sep 9, 2020

ockham commented Sep 9, 2020

ockham commented Sep 9, 2020

ockham commented Sep 9, 2020

fullofcaffeine commented Sep 9, 2020 •

edited

fullofcaffeine commented Sep 9, 2020 •

edited by ockham

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ockham commented Sep 10, 2020

fullofcaffeine commented Sep 10, 2020

ockham commented Sep 10, 2020

ockham commented Sep 10, 2020

ockham left a comment

github-actions bot commented Sep 10, 2020

WordPress / gutenberg

Join GitHub today

GitHub is where the world builds software

Add regression E2E test for the classic block initialization issue #25169

Add regression E2E test for the classic block initialization issue #25169

Conversation

fullofcaffeine commented Sep 9, 2020 • edited by ockham

Description

How has this been tested?

Types of changes

Checklist:

sirreal commented Sep 9, 2020

ockham commented Sep 9, 2020

ockham commented Sep 9, 2020

ockham commented Sep 9, 2020

fullofcaffeine commented Sep 9, 2020 • edited

fullofcaffeine commented Sep 9, 2020 • edited by ockham

This comment has been minimized.

ockham Sep 10, 2020 Contributor

This comment has been minimized.

ockham Sep 10, 2020 Contributor

This comment has been minimized.

sirreal Sep 10, 2020 • edited Member

This comment has been minimized.

ockham Sep 10, 2020 Contributor

This comment has been minimized.

fullofcaffeine Sep 10, 2020 Author Member

This comment has been minimized.

fullofcaffeine Sep 10, 2020 Author Member

This comment has been minimized.

fullofcaffeine Sep 10, 2020 • edited Author Member

ockham commented Sep 10, 2020

fullofcaffeine commented Sep 10, 2020

ockham commented Sep 10, 2020

ockham commented Sep 10, 2020

ockham left a comment

github-actions bot commented Sep 10, 2020

Essential cookies

Always active

Analytics cookies

fullofcaffeine commented Sep 9, 2020 •

edited by ockham

fullofcaffeine commented Sep 9, 2020 •

edited

fullofcaffeine commented Sep 9, 2020 •

edited by ockham

ockham Sep 10, 2020
Contributor

ockham Sep 10, 2020
Contributor

sirreal Sep 10, 2020 •

edited

Member

ockham Sep 10, 2020
Contributor

fullofcaffeine Sep 10, 2020
Author Member

fullofcaffeine Sep 10, 2020
Author Member

fullofcaffeine Sep 10, 2020 •

edited

Author Member