Migrate flaky PostPublishButton e2e tests to Playwright #52285
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
requested review from
kevin940726,
ntwb,
nerrad and
ajitbohra
as code owners
|
Size Change: 0 B Total Size: 1.7 MB ℹ️ View Unchanged
|
|
Maybe we should remove this e2e test altogether. Testing intermediate states like "should be disabled when", which depend on how fast the network request resolves, are flaky. These disabled states can be covered in unit tests for the component. @kevin940726, @jsnajdr, what do you think? |
|
The "should be disabled when metabox is being saved" test can indeed be removed, in #44971 I'm also removing it, because it can't be reimplemented reliably with the new saving process logic. The "should be disabled when post is being saved" test is not affected by #44971, it just turns out it's flaky, because the save can be finished before the async query even starts looking for the CSS selector. Yes, the test can be removed, it's not particularly critical. But if you wanted to save it, one way is to make use of the new let resolve;
page.evaluate( () => {
addFilter( 'editor.__unstableSavePost', () => new Promise( r => { resolve = r; } ) );
} );
await page.click( saveButton );
expect( await page.$( disabled ) );
resolve();In practice there are issues like that you can't really just assign |
|
@oandregal, as you introduce the original e2e test for publish button, what do you think about its removal? The implementation details, like the button's disabled state, are already covered by unit tests. |
|
@Mamaduka thanks for reaching out. It must have been years since I last looked into this. I haven't dug deeply into how to make e2e tests reliable, so I trust your call. I find it a bit sad that we need to remove it, given some of it (saving metaboxes) covers not obvious paths people might not test manually. As a general approach, I'd favor e2e over unit tests. Unit tests verify the component behaves correctly for a set of given input. e2e tests verify the same thing and that the inputs still behave the way components expect them to behave: they cover much ground. |
|
@oandregal, I'm also in favor of e2e tests. I'll check if I can change the tests in a way that feels less like implementation details testing but still covers initial cases. If that's doable. |
I think testing metabox saving is still possible and desirable. The problem was solely with a test that was testing a very ephemeral thing: that the Save button is disabled while the save operation is in progress. It's difficult for an e2e test to precisely catch the moment when the button is disabled. All the operations are async, both the save itself and also the interaction of Puppeteer/Playwright with the browser. You need to be able to "pause" the save operation somehow, programatically, and it's exactly this "pausing" where the problematic test had to reach out into Gutenberg internals. |
|
Just want to throw out some ideas that we technically can throttle the network in Playwright so that the save operation doesn't fulfill immediately. With route interception, we can set a programmatically controlled delay and manually fulfill it once the saving assertion has passed. However, I think this is the kind of test that is best suited for integration testing. Unfortunately, we don't have that architecture in place to support that yet. |
Mamaduka
force-pushed
the
migrate/post-publish-button-tests
branch
from
a796061 to
68a68cc
Compare
Mamaduka
added
the
[Type] Automated Testing
|
I've updated the tests. Now they're using route interception and deferring technique — props to @kevin940726 for suggestions and code examples. The meta-box test now actually tests saving flow when the meta-box plugin is enabled instead of emulating the action. The e2e tests passed on the first try; I've restarted them just to be sure. Update: I re-run the e2e test four times, no failure for the migrated tests. |
Mamaduka
force-pushed
the
migrate/post-publish-button-tests
branch
from
68a68cc to
73d5d88
Compare
Mamaduka
requested review from
WunderBart and
oandregal
and removed request for
ntwb,
ajitbohra and
nerrad
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Part of #38851.
Closes #35641.
Migrates
PostPublishButtone2e tests to Playwright.Testing Instructions