Add regresion E2E test for the empty reusable block causing WSODs issue #26913

fullofcaffeine · 2020-11-12T03:14:06Z

Description

Adds a regression test in the client block E2E spec to avoid the regression of this issue: #26485.

How to test

Check out this branch, and apply the following diff (effectively reverting the fix in Fix block inserter WSOD when an empty reusable block exists #26484):

diff --git c/packages/block-editor/src/store/selectors.js w/packages/block-editor/src/store/selectors.js
index c2153ee33c..9432c252bb 100644
--- c/packages/block-editor/src/store/selectors.js
+++ w/packages/block-editor/src/store/selectors.js
@@ -1704,11 +1704,7 @@ export const __experimentalGetParsedReusableBlock = createSelector(
 
 		// Only reusableBlock.content.raw should be used here, `reusableBlock.content` is a
 		// workaround until #22127 is fixed.
-		return parse(
-			typeof reusableBlock.content.raw === 'string'
-				? reusableBlock.content.raw
-				: reusableBlock.content
-		);
+		return parse( reusableBlock.content.raw || reusableBlock.content );
 	},
 	( state ) => [ getReusableBlocks( state ) ]
 );

Prepare the env

Run npm run build and npm run wp-env-start;
Access your wp-env instance and make sure Gutenberg is active.

Run the E2E test

Run npm run test-e2e packages/e2e-tests/specs/editor/various/reusable-blocks.test.js;
The test will create a reusable block, then edit it and make it empty, then try to insert a block in a new post. Verify that the error happens and that the test fails;
Now remove the patch (git checkout -- packages/block-editor/src/store/selectors.js), and rebuild (npm run build);
Re-run the e2e test. It should pass this time.

Run the unit test

Run npm run test-unit-js packages/block-editor/src/store/test/selectors.js.
The test will create a reusable block, then edit it and make it empty, then try to insert a block in a new post. Verify that the error happens and that the test fails.
Now remove the patch (git checkout -- packages/block-editor/src/store/selectors.js), and rebuild (npm run build).
Re-run the unit 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.

gziolo

I left some minor comments. Thank you for sending PR with new tests added 👍

gziolo · 2020-11-13T05:28:42Z

packages/block-editor/src/store/test/selectors.js

+	it( "should not throw an exception if reusable block's content.raw is an empty string", () => {
+		expect( () =>
+			__experimentalGetParsedReusableBlock( state, 1 )
+		).not.toThrowError();


You can also update this check and test for the actual result. If code would throw it will fail the test as well.

Do you mean to re-purpose the test to test one of the actual scenarios/happy-path for the __experimentalGetParsedReusableBlock helper func? That means it wouldn't be a specific regression test anymore (as stated in the describe). That's not a big deal, but then we'd be testing the func, and not the bug regression (which would still be tested indirectly, as you mention, though). I'm fine with that but just want to make sure we're on the same page.

Yes, this is what I meant. not.toThrowError(); part would be replaced with the assertion checking the actual result and the description for the test updated.

gziolo · 2020-11-13T05:34:30Z

packages/e2e-test-utils/src/index.js

@@ -35,6 +35,7 @@ export {
 	insertBlockDirectoryBlock,
 	openGlobalBlockInserter,
 	closeGlobalBlockInserter,
+	toggleGlobalBlockInserter,


There are 2 existing helpers to control the block inserter. Would it be possible to use them instead of introducing another helper function?

👋🏻 Hi Greg! I tried openGlobalBlockInserter but it waits for the actual inserter el to be present on the page. In the case the error ever regresses, it would cause a timeout because the inserter is never shown. What we really need is to just click to show the inserter, and this helper does just that. What's the other helper?

Also, this is actually an existing helper, I just exported it.

I tried openGlobalBlockInserter but it waits for the actual inserter el to be present on the page. In the case the error ever regresses, it would cause a timeout because the inserter is never shown.

It looks like openGlobalBlockInserter is exactly what fits best here. toggleGlobalBlockInserter only verifies if the button can be clicked. The waiting part is crucial here to prevent regression. If it would timeout in the future, that's a proper way to signal that something went wrong.

toggleGlobalBlockInserter only verifies if the button can be clicked.

Hmm, it actually wraps a click call: https://github.com/WordPress/gutenberg/blob/master/packages/e2e-test-utils/src/inserter.js#L49. For the purposes of this regression test, that's all we need.

If we decide to use openGlobalBlockInserter, there's another potential problem as well (which I just tested): if it the bug regresses, it will fail with the timeout and not with the expect below it. It also means it will slow down the actual test that would finish faster if we weren't waiting for an inserter we know won't appear.

gziolo · 2020-11-13T05:41:05Z

packages/e2e-tests/specs/editor/various/reusable-blocks.test.js

+
+		await toggleGlobalBlockInserter();
+
+		expect( console ).not.toHaveErrored();


This check is scoped only to the e2e spec. It isn't possible to catch errors on the page this way. We have a global verification for the page:

gutenberg/packages/e2e-tests/config/setup-test-framework.js

Line 99 in 23a69e2

page.on( 'console', ( message ) => {

Hmmm, interesting. It seemed like it did catch the error when I tested. Or maybe it was just the error bubbling up. Maybe I missed something.

Do you mean then this assertion is not needed at all? Any hints on how I should rewrite that?

It would mean there is console.error call in the test code, not on the page. What error do you get when it isn't called?

I double-checked (by re-introducing the bug that caused the regression) and that expect does seem to work for the purposes of the test, see the output in the gist:

https://gist.github.com/fullofcaffeine/453e58fba78d902bc690e0b6994e7b31

It is true that it might be considered redundant because if I remove it, the test still fails because the error bubbles up (as described here) and it caught from within the console object:

https://gist.github.com/fullofcaffeine/ade1be595e7f1c5bd46ed5b652814934

That being said, I'd rather keep the expect there as it communicates what the test is not expecting.

So, it seems it's fine the way it is? Let me know if I missed something 🙇🏻

… sure it properly handles the scenario when the reusable block's content is an empty string

…f explicitly checking that it does not throw an exception

…SODs issue (#26913)" This reverts commit 1b2d663.

gziolo · 2020-11-14T15:58:52Z

It looks like the new e2e test fails on master. I’m opening a revert PR for now but we should definitely update this test and try merge again.

…SODs issue (#26913)" (#26979) This reverts commit 1b2d663.

fullofcaffeine · 2020-11-14T17:47:24Z

It looks like the new e2e test fails on master. I’m opening a revert PR for now but we should definitely update this test and try merge again.

Strange, this spec is green for me locally.

It seems to fail when trying to get hold of the block toolbar, maybe the approach to focus the block is flaky? It does seem to consistently work locally for me, though.

I'll create a new PR for the revert of the revert and look into this next week.

Thanks! 🙇🏻

fullofcaffeine · 2020-11-14T18:33:20Z

Revert of revert PR is here, ~~I'll investigate the flakiness and check what's up~~. The linked PR fixes the issue ✅

* Revert "Revert "Add regresion E2E test for the empty reusable block causing WSODs issue (#26913)" (#26979)" This reverts commit cca33e5. * Wait for the block to be present on the page and click it to five it focus instead of tabbing to it * Make the test more resilient by waiting for the Update button to become enabled

fullofcaffeine mentioned this pull request Nov 12, 2020

Add regression test for the GB 9.2 empty reusable block issue #26548

Closed

fullofcaffeine marked this pull request as ready for review November 13, 2020 00:51

fullofcaffeine requested review from ajitbohra, ellatrix, gziolo, nerrad, ntwb and talldan as code owners November 13, 2020 00:51

gziolo reviewed Nov 13, 2020

View reviewed changes

gziolo added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) labels Nov 13, 2020

fullofcaffeine and others added 5 commits November 13, 2020 20:40

Add regresion E2E test for the empty reusable block causing WSODs issue

0cf0237

Add unit regression test __experimentalGetParsedReusableBlock to make…

c054069

… sure it properly handles the scenario when the reusable block's content is an empty string

Minor cleanup

aea0867

Change the unit test to check for the function return value instead o…

e99ec76

…f explicitly checking that it does not throw an exception

Wait for the editor writing flow div before trying to click it

044c358

sirreal added the [Type] Code Quality Issues or PRs that relate to code quality label Nov 14, 2020

gziolo approved these changes Nov 14, 2020

View reviewed changes

gziolo merged commit 1b2d663 into WordPress:master Nov 14, 2020

github-actions bot added this to the Gutenberg 9.4 milestone Nov 14, 2020

gziolo added a commit that referenced this pull request Nov 14, 2020

Revert "Add regresion E2E test for the empty reusable block causing W…

c4f2b7c

…SODs issue (#26913)" This reverts commit 1b2d663.

gziolo mentioned this pull request Nov 14, 2020

Revert "Add regresion E2E test for the empty reusable block causing W… #26979

Merged

6 tasks

gziolo added a commit that referenced this pull request Nov 14, 2020

Revert "Add regresion E2E test for the empty reusable block causing W…

cca33e5

…SODs issue (#26913)" (#26979) This reverts commit 1b2d663.

fullofcaffeine mentioned this pull request Nov 14, 2020

Revert the reversion of https://github.com/WordPress/gutenberg/pull/26913 and fix flakiness #26982

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add regresion E2E test for the empty reusable block causing WSODs issue #26913

Add regresion E2E test for the empty reusable block causing WSODs issue #26913

fullofcaffeine commented Nov 12, 2020 •

edited

Loading

gziolo left a comment

gziolo Nov 13, 2020

fullofcaffeine Nov 13, 2020 •

edited

Loading

gziolo Nov 13, 2020

gziolo Nov 13, 2020

fullofcaffeine Nov 13, 2020 •

edited

Loading

fullofcaffeine Nov 13, 2020

gziolo Nov 13, 2020

fullofcaffeine Nov 13, 2020

gziolo Nov 13, 2020

fullofcaffeine Nov 13, 2020 •

edited

Loading

fullofcaffeine Nov 13, 2020 •

edited

Loading

gziolo Nov 13, 2020

fullofcaffeine Nov 13, 2020 •

edited

Loading

gziolo commented Nov 14, 2020

fullofcaffeine commented Nov 14, 2020

fullofcaffeine commented Nov 14, 2020 •

edited

Loading


		await toggleGlobalBlockInserter();

		expect( console ).not.toHaveErrored();

Add regresion E2E test for the empty reusable block causing WSODs issue #26913

Add regresion E2E test for the empty reusable block causing WSODs issue #26913

Conversation

fullofcaffeine commented Nov 12, 2020 • edited Loading

Description

How to test

Prepare the env

Run the E2E test

Run the unit test

Types of changes

Checklist:

gziolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fullofcaffeine Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fullofcaffeine Nov 13, 2020 • 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

fullofcaffeine Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

fullofcaffeine Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fullofcaffeine Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

gziolo commented Nov 14, 2020

fullofcaffeine commented Nov 14, 2020

fullofcaffeine commented Nov 14, 2020 • edited Loading

fullofcaffeine commented Nov 12, 2020 •

edited

Loading

fullofcaffeine Nov 13, 2020 •

edited

Loading

fullofcaffeine Nov 13, 2020 •

edited

Loading

fullofcaffeine Nov 13, 2020 •

edited

Loading

fullofcaffeine Nov 13, 2020 •

edited

Loading

fullofcaffeine Nov 13, 2020 •

edited

Loading

fullofcaffeine commented Nov 14, 2020 •

edited

Loading