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 Show Classic block contents on load #25162 and Classic block: Optional chain on possibly null editor #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:03Z

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!

sirreal added [Block] Classic Affects the Classic Editor Block [Package] E2E Tests /packages/e2e-tests [Status] In Progress Tracking issues with work in progress labels Sep 9, 2020

sirreal assigned fullofcaffeine Sep 9, 2020

fullofcaffeine marked this pull request as ready for review September 9, 2020 22:32

fullofcaffeine requested review from ajitbohra, nerrad, ntwb and talldan as code owners September 9, 2020 22:32

ockham reviewed Sep 9, 2020

View reviewed changes

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

ockham reviewed Sep 10, 2020

View reviewed changes

fullofcaffeine and others added 8 commits September 10, 2020 17:32

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

Rename selector

32f0998

Use focus instead of click, inline

a96c8db

Add exception handling

748c799

ockham approved these changes Sep 10, 2020

View reviewed changes

ockham merged commit d7cda50 into WordPress:master Sep 10, 2020

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Sep 10, 2020

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

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

ockham mentioned this pull request Sep 14, 2020

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Loading

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

Loading

fullofcaffeine commented Sep 9, 2020 •

edited by ockham

Loading

ockham Sep 10, 2020

ockham Sep 10, 2020

sirreal Sep 10, 2020 •

edited

Loading

ockham Sep 10, 2020

fullofcaffeine Sep 10, 2020

fullofcaffeine Sep 10, 2020

fullofcaffeine Sep 10, 2020 •

edited

Loading

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

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 Loading

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 Loading

fullofcaffeine commented Sep 9, 2020 • edited by ockham Loading

ockham Sep 10, 2020

Choose a reason for hiding this comment

ockham Sep 10, 2020

Choose a reason for hiding this comment

sirreal Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

ockham Sep 10, 2020

Choose a reason for hiding this comment

fullofcaffeine Sep 10, 2020

Choose a reason for hiding this comment

fullofcaffeine Sep 10, 2020

Choose a reason for hiding this comment

fullofcaffeine Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

ockham commented Sep 10, 2020

fullofcaffeine commented Sep 10, 2020

ockham commented Sep 10, 2020

ockham commented Sep 10, 2020

ockham left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 10, 2020

fullofcaffeine commented Sep 9, 2020 •

edited by ockham

Loading

fullofcaffeine commented Sep 9, 2020 •

edited

Loading

fullofcaffeine commented Sep 9, 2020 •

edited by ockham

Loading

sirreal Sep 10, 2020 •

edited

Loading

fullofcaffeine Sep 10, 2020 •

edited

Loading