Fix Gutenberg e2e tests failures. #3324
Conversation
budzanowski
added
skip-changelog
|
Size Change: 0 B Total Size: 1.12 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for sorting this out @budzanowski !
This approach seems reasonable. I tested by re-running the tests locally on trunk (reproduced the bug) and then again on this branch, and confirmed this fixes the problem. 🎉
Note – I manually set up gutenberg plugin and latest e2e utils as appropriate, using the commands in the travis config:
npm run wp-env run tests-cli "wp plugin install gutenberg --activate"npm install @wordpress/e2e-test-utils@latest
I'm approving the PR, but I wonder if it's better to revisit the tests that were failing. Here are a couple of ideas:
- Perhaps we can tweak how we implement
can only be inserted oncetests so they are more robust across WP versions. E.g. perhaps we can search the inserter for the block and confirm that it's hidden or disabled. Ideally we can do this using utils from WordPress core. - Or perhaps these tests are not actually that useful - maybe they aren't the most important thing to test.
Up to you if you want to try any of this, not a blocker - feel free to go with your existing solution.
(By the way, the inline comments on the code are mostly me figuring out the failure mode, and understanding the proposed solution.)
| * | ||
| * @param {string} searchTerm The text to search the inserter for. | ||
| */ | ||
| export async function insertBlockDontWaitForInsertClose( searchTerm ) { |
There was a problem hiding this comment.
This name seems a little long, what's the main purpose of this utility func?
I think we can probably keep the name as insertBlock - i.e. the previous closing behaviour was an unintentional side effect.
Aha, after re-reading the PR description, I see that we were relying on the previous behaviour.
Since the second add operation is not possible the block selector will not be closed and the focus will not be shifted.
I'm a bit confused - here's a summary of what I understand, let me know if I've got something wrong.
- Our tests often start with
visitBlockPage()which takes care of creating a page containing the block. - For some blocks, we only allow one instance per page.
- To test this behaviour, we do something like this:
await insertBlock( block.name );
await closeInserter();
expect( await getAllBlocks() ).toHaveLength( 1 );I'm assuming this code had an issue – what was the specific issue / error? Maybe our test code assumed something it shouldn't.
Is it possible to switch to using closeGlobalBlockInserter instead of our custom func - would that help? Or perhaps we can tweak the approach here, do we need to close the inserter?
await insertBlock( block.name );
await closeGlobalBlockInserter();
expect( await getAllBlocks() ).toHaveLength( 1 );There was a problem hiding this comment.
Update: I ran this locally (with trunk) to better understand the issue. Here's the all products test fail:
FAIL tests/e2e/specs/backend/all-products.test.js (33.901s)
● All Products Block › can only be inserted once
: Timeout - Async callback was not invoked within the 30000ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 30000ms timeout specified by jest.setTimeout.Error:
So the issue is that something's timing out in the test, maybe the insertBlock().
There was a problem hiding this comment.
Exactly the failure was inside the insertBlock() that was waiting for the inserter to close https://github.com/WordPress/gutenberg/blob/master/packages/e2e-test-utils/src/inserter.js#L135 that is why we have our own function that does not wait for this behavior.
| export const closeInserter = async () => { | ||
| await page.click( '.edit-post-header [aria-label="Add block"]' ); | ||
| }; |
There was a problem hiding this comment.
Nice to see this common code factored out into the library 👍
Does this always close the inserter, or does it assume that the inserter is open? For example, if the inserter is not open, does it open it? Put another way, if we call twice in a row, what happens?
If so, might be more robust to add a check for whether the inserter is open/visible at the start of the function.
There was a problem hiding this comment.
Actually, it looks like we can use e2e-test-utils for this:
If these are not available in older environments that we use for our tests, we can implement a function with the same name to patch it to the current API 😄
|
@haszari |
There was a recent change in how the blocks insert mechanism works. It now works as a popover: WordPress/gutenberg#24429
This means that inserting a block should close the modal and put the focus on the newly added block. Since the automatic close action is part of the new behavior the
insertBlockpuppeteer command was also updated. It now expects that the blocks selection panel becomes closed and focus is shifted:https://github.com/WordPress/gutenberg/blob/master/packages/e2e-test-utils/src/inserter.js#L135
We have three instances of a test where we test that the block can be added just once on the page. Since the second add operation is not possible the block selector will not be closed and the focus will not be shifted. This means that we can't use
insertBlockfor our tests. A new utility function was created that tries to add the block but does not wait for the closing.Fixes #2993
How to test the changes in this Pull Request: