Fix flaky template revert e2e tests #50851
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
added
[Type] Bug
|
Size Change: 0 B Total Size: 1.4 MB ℹ️ View Unchanged
|
|
Flaky tests detected in 07067dc. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5048734922
|
|
I also had a brief look at this yesterday as I noticed this one is prolific. I had the same conclusion that it was closing the already open sidebar.
They should be reset, I'm not sure how regularly though? Code is here: |
talldan
approved these changes
May 23, 2023
IIRC, that happens before all tests. Maybe we should add something after each test? Not sure about the performance impact. |
|
I think there's a regression with resetting the preferences before all tests. I could only reproduce the problem when running the template part and template revert tests together. The template part tests open the document settings, and the preferences were persistent across the test files. Here's another example #50519. You can try running these tests before the c464a3f commit. npm run test:e2e:playwright -- test/e2e/specs/site-editor/template-part.spec.js test/e2e/specs/site-editor/template-revert.spec.jsIs it possible to disable the preference persistence layer via PHP? Then we can easily rely on the |
|
When we say before all tests, do we mean before each test file, or before the entire suite being run? I was under the impression it should be the former.
My reason for not doing that is that I think this is making the tests less like the real world. If issues are introduced with the persistence layer, the e2e tests won't catch them. I think it should be enough to make sure they're reset before each file. |
Yes, this should handle most of the cases, and using utility methods can handle the rest of them :) |
It still fails for me, even when I put the following in the template revert await page.evaluate( () => {
window.localStorage.clear();
} );
await requestUtils.resetPreferences();That should completely clear preferences. That it doesn't makes me wonder if the commands are actually succeeding. Also, it makes my wp-env docker environment fail when I run these, so testing it is very slow. 😄 edit: Oh wait, just noticed it's a different test that fails. Same issue though. It's a bit weird. |
|
The network request is successful in my tests. I would recommend using the new UI mode for debugging if you're not already using it :) Screenshot
|
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?
Resolves #50783.
Resolves #47897.
PR fixes "Template revert" e2e test regressions after #50369.
Why?
The user preferences are persisted in DB and shared across the tests. If previous tests opened the document settings sidebar, the template revert tests accidentally closed it, causing a locator failure that depended on the sidebar's open state.
How?
Updates the
revertTemplatehelper to use theeditor.openDocumentSettingsSidebar()utility method instead of a direct locator. The former has extra checks to avoid accidentally closing the document settings sidebar.Testing Instructions
To quickly reproduce the issue on the
trunk:editor.openDocumentSettingsSidebar()at the end of thetest.beforeEachblock.npm run test:e2e:playwright -- /test/e2e/specs/site-editor/template-revert.spec.js.