Try to fix flaky synced pattern test #55406
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
|
Size Change: 0 B Total Size: 1.66 MB ℹ️ View Unchanged
|
ellatrix
added
the
[Type] Automated Testing
Mamaduka
approved these changes
Oct 17, 2023
andrewserong
approved these changes
Oct 18, 2023
There was a problem hiding this comment.
This sounds like a reasonable fix for the flaky test to me, but I'm wondering if the flaky test reveals there's a focus loss after creating a pattern when there mightn't be expected to be one?
From a quick look in #54804 it sounds like from calling setEditingPattern after pattern creation it might be expected that focus is on the newly added pattern, whereas when I test out the steps of this e2e test locally, it seems that there could be a focus loss after inserting the pattern?
Here's a quick screengrab:
2023-10-18.14.21.41.mp4
I'll just CC @glendaviesnz and @aaronrobertshaw just in case there's anything unexpected there.
In terms of this test, which is about writing flow, the change here LGTM as the test is meant to cover the Up arrow behaviour further down int his test, and this PR unblocks stabilising that.
There was a problem hiding this comment.
I'll just CC @glendaviesnz and @aaronrobertshaw just in case there's anything unexpected there.
Thanks for the ping 👍
From a quick look in #54804 it sounds like from calling setEditingPattern after pattern creation it might be expected that focus is on the newly added pattern
That expectation is probably confirmed via the existing test relying on the toBeFocused assertion. @kevin940726 might have a better understanding of what the expected behaviour should be.
In terms of this test, I'm on board with the proposed change if it unblocks things and fixes the flakey test.
Yes, there's most likely a preexisting bug. I didn't look into it further since my priority was to get this test stabilised and this test doesn't have to rely on he pattern receiving focus after insertion. Thanks for anyone looking into this bug. |
|
I have a fix for the focus issue opened in #55473! |
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?
Fixes #55284.
It seems to be the first focus check that is the problem, maybe focus after converting to a synced pattern is not guaranteed. Let's just wait for the block to be loaded, I think
toBeVisibleshould do that.Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast