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

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

Merged
merged 5 commits into from
Nov 14, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0cf0237
Add regresion E2E test for the empty reusable block causing WSODs issue
fullofcaffeine Nov 12, 2020
c054069
Add unit regression test __experimentalGetParsedReusableBlock to make…
fullofcaffeine Nov 13, 2020
aea0867
Minor cleanup
fullofcaffeine Nov 13, 2020
e99ec76
Change the unit test to check for the function return value instead o…
fullofcaffeine Nov 14, 2020
044c358
Wait for the editor writing flow div before trying to click it
fullofcaffeine Nov 14, 2020
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
21 changes: 21 additions & 0 deletions packages/block-editor/src/store/test/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ const {
__experimentalGetLastBlockAttributeChanges,
getLowestCommonAncestorWithSelectedBlock,
__experimentalGetActiveBlockIdByBlockNames: getActiveBlockIdByBlockNames,
__experimentalGetParsedReusableBlock,
} = selectors;

describe( 'selectors', () => {
Expand Down Expand Up @@ -3034,3 +3035,23 @@ describe( 'selectors', () => {
} );
} );
} );

describe( '__experimentalGetParsedReusableBlock', () => {
const state = {
settings: {
__experimentalReusableBlocks: [
{
id: 1,
content: { raw: '' },
},
],
},
};

// Regression test for https://github.com/WordPress/gutenberg/issues/26485. See https://github.com/WordPress/gutenberg/issues/26548.
it( "Should return an empty array if reusable block's content.raw is an empty string", () => {
expect( __experimentalGetParsedReusableBlock( state, 1 ) ).toEqual(
[]
);
} );
} );
4 changes: 4 additions & 0 deletions packages/e2e-test-utils/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,10 @@ running the test is not already the admin user).
Switches the current user to whichever user we should be
running the tests as (if we're not already that user).

<a name="toggleGlobalBlockInserter" href="#toggleGlobalBlockInserter">#</a> **toggleGlobalBlockInserter**

Toggles the global inserter.

<a name="toggleMoreMenu" href="#toggleMoreMenu">#</a> **toggleMoreMenu**

Toggles the More Menu.
Expand Down
1 change: 1 addition & 0 deletions packages/e2e-test-utils/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export {
insertBlockDirectoryBlock,
openGlobalBlockInserter,
closeGlobalBlockInserter,
toggleGlobalBlockInserter,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@fullofcaffeine fullofcaffeine Nov 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

👋🏻 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

} from './inserter';
export { installPlugin } from './install-plugin';
export { installTheme } from './install-theme';
Expand Down
6 changes: 4 additions & 2 deletions packages/e2e-test-utils/src/inserter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,10 @@ async function isGlobalInserterOpen() {
);
} );
}

async function toggleGlobalBlockInserter() {
/**
* Toggles the global inserter.
*/
export async function toggleGlobalBlockInserter() {
await page.click(
'.edit-post-header [aria-label="Add block"], .edit-site-header [aria-label="Add block"]'
);
Expand Down
42 changes: 42 additions & 0 deletions packages/e2e-tests/specs/editor/various/reusable-blocks.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import {
searchForReusableBlock,
getEditedPostContent,
trashAllPosts,
visitAdminPage,
toggleGlobalBlockInserter,
} from '@wordpress/e2e-test-utils';

function waitForAndAcceptDialog() {
Expand Down Expand Up @@ -330,4 +332,44 @@ describe( 'Reusable blocks', () => {
// Check that we have two paragraph blocks on the page
expect( await getEditedPostContent() ).toMatchSnapshot();
} );

it( 'will not break the editor if empty', async () => {
await insertReusableBlock( 'Awesome block' );

await visitAdminPage( 'edit.php', [ 'post_type=wp_block' ] );

const [ editButton ] = await page.$x(
`//a[contains(@aria-label, 'Awesome block')]`
);
await editButton.click();

await page.waitForNavigation();

// Give focus to the editor
await page.waitForSelector( '.block-editor-writing-flow' );
await page.click( '.block-editor-writing-flow' );

// Move focus to the paragraph block
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Tab' );

// Delete the block, leaving the reusable block empty
await clickBlockToolbarButton( 'More options' );
const deleteButton = await page.waitForXPath(
'//button/span[text()="Remove block"]'
);
deleteButton.click();

// Save the reusable block
await page.click( '.editor-post-publish-button__button' );
await page.waitForXPath(
'//*[contains(@class, "components-snackbar")]/*[text()="Reusable Block updated."]'
);

await createNewPost();

await toggleGlobalBlockInserter();

expect( console ).not.toHaveErrored();
Copy link
Member

Choose a reason for hiding this comment

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

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 ) => {

Copy link
Member Author

@fullofcaffeine fullofcaffeine Nov 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@fullofcaffeine fullofcaffeine Nov 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@fullofcaffeine fullofcaffeine Nov 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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 🙇🏻

} );
} );