[RNMobile] - Add optional wait in getBlockAtPosition#40078
Merged
Conversation
|
Size Change: +300 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
fluiddot
reviewed
Apr 7, 2022
Contributor
fluiddot
left a comment
There was a problem hiding this comment.
I verified that the E2E tests succeeded in wordpress-mobile/gutenberg-mobile#4734, so the changes look great to me 🎊 .
I added some comments within the source code, there's nothing critical so let me know if you'll be able to address them. Otherwise, I'll be more than happy to approve the PR.
packages/react-native-editor/__device-tests__/gutenberg-editor-paragraph.test.js
Show resolved
Hide resolved
|
|
||
| await editorPage.setHtmlContent( | ||
| [ | ||
| testData.unknownElementBlock, |
Contributor
There was a problem hiding this comment.
unknownElementBlock might sound too generic since this is related to a Paragraph block, what do you think about renaming it to unknownElementParagraphBlock?
Comment on lines
+454
to
+457
| const blockActionsMenuButton = await waitForVisible( | ||
| this.driver, | ||
| blockActionsMenuButtonLocator | ||
| ); |
Contributor
There was a problem hiding this comment.
I noticed that the following code block handles the case where the Block Actions Menu button is not visible. The buttons is displayed at the bottom of the block, so it tries to scroll down the block list until it's visible. Not sure why this behavior is only applied on Android since it might also happen on iOS 🤔 . In any case, I'm wondering if we could use the waitForVisible helper there too, wdyt?
gutenberg/packages/react-native-editor/__device-tests__/pages/editor-page.js
Lines 459 to 470 in eda2d66
Contributor
Author
There was a problem hiding this comment.
Oh right, I can try that, this one I will try in the next PR instead of here as this is something that the paragraph tests use a lot and testing could take a while. Good point about being Android only, not sure why but I'll try to remove it to see if it will work for both platforms in the next PR as well.
jostnes
commented
Apr 7, 2022
packages/react-native-editor/__device-tests__/gutenberg-editor-paragraph.test.js
Show resolved
Hide resolved
Comment on lines
+454
to
+457
| const blockActionsMenuButton = await waitForVisible( | ||
| this.driver, | ||
| blockActionsMenuButtonLocator | ||
| ); |
Contributor
Author
There was a problem hiding this comment.
Oh right, I can try that, this one I will try in the next PR instead of here as this is something that the paragraph tests use a lot and testing could take a while. Good point about being Android only, not sure why but I'll try to remove it to see if it will work for both platforms in the next PR as well.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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?
I have been testing paragraph tests in this draft PR. However, the changes became too big so I am breaking it down to a few smaller PRs. This PR is the first part of a few PRs in an attempt to fix the flakiness on tests that are using the paragraph blocks.
Why?
The paragraph tests are currently flaky and don't work when running locally. When it fails in the CI, it is harder to debug it locally to know if it's an issue with the test or something else. This change is the first part to make it work consistently both when testing locally and in CI.
How?
gutenberg-editor-paragraph.test.jstotest-data.jsfile to be consistent with other test filesgetBlockAtPosition()to use the new helperwaitForVisible(). It is optional because from testing found that adding this here works for some tests but not all. In order to not have to update all tests at the same time, adding this condition so we can try it out test by test. Once we have checked all the tests, this condition can be removed/updated.Testing Instructions
Check that test still work in the CI.
Screenshots or screencast