-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Fix flaky tests (homepage-settings.spec.js duplicate Sample Page / Sample page row match, publish-panel.spec.js focus assertion before panel close completed) #77893
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.
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
Open
danluu
wants to merge
1
commit into
WordPress:trunk
Choose a base branch
from
danluu:try/test-flakes-pr
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+31
−32
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an unlikely fix. The
.toBeFocused()check on the next should wait until the button is focused, i.e., should be fine with finding an unfocused button several times before giving up. The failure report looks like this:Note that when the button failed to be focused, it still has
aria-expanded=trueattribute. Waiting foraria-expanded=falseshould fail the same way as waiting for focus. The9 × locatorbit means that Playwright tried 9 times before giving up.It looks almost like the test fails to click the "Cancel" button at all. It can happen when it is disabled, i.e., the
isSavingNonPostEntityChangesselector value is true. This happens when some other entity is saving in the background. Strange.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is an odd, flaky one. Trace logs say that the button is enabled, but the click still fails.
Update: Our a11y way of disabling buttons uses
accessibleWhenDisabled, which rendersaria-disabled="true"and interceptsclick/mousedownwithstopPropagation()+preventDefault(), soonClicknever fires. This is why the logs were misleading.@jsnajdr, suspicion was right. I'm going to create PR for this. See #78082.
Trace logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the Playwright trace stored as an action artifact, I see that the test has trouble clicking "Cancel" because the
editor-post-publish-panel__slide-in-animationanimation is running. ThePostPublishPanelis sliding in and the "Cancel" button is moving. That's why the click sometimes fails.It's very strange because this animation is gated with
@media not (prefers-reduced-motion)and Playwright is configured withreducedMotion: 'reduce'. The animation shouldn't run. And yet, as anyone can check themselves by running the test withnpm run test:e2e:debugin UI mode, the animation is running. And if you add abackground-color: redstyle to make is visually obvious whetherprefers-reduced-motionis respected, then the panel is red! It's also interesting that adding a check in code:it will log
true. And yet the visual output based on CSS will behave likefalse.I think this is a Playwright bug, I can reproduce it even with a minimal independent example. I'll look into this further today.