Skip to content

Conversation

@mokagio
Copy link
Contributor

@mokagio mokagio commented Jul 19, 2023

We are seeing UI tests in the editor in the branches that updated the Gutenberg integration to use a new version of React Native, a new Javascript engine, and a new distribution mode. Those are a lot of core changes, so it's not that surprising some assumptions no longer hold, in particular when there is React Native involved.

Here's an example of a failure, which can be seen here or here

image

I spent some time trying to make the standard approach work, including changing the element query to run on the textViews[...].firstMatch directly, but it didn't work. I'm not particularly happy with the approach I ended up with, but it's the only one I could make to work.

You can see the tests in this build, from which I cherry-picked, pass, while the ones in the previous runs failed.

I'm opening this PR against trunk to:

  1. Decouple the test changes from the other Gutenberg upgrade work
  2. See whether the implementation is sound by checking if it works in a context where typeTexts was shown to work, too

Regression Notes

  1. Potential unintended areas of impact – N.A.
  2. What I did to test those areas of impact (or what existing automated tests I relied on) – N.A.
  3. What automated tests I added (or what prevented me from doing so) – N.A.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes. N.A.
  • I have considered adding accessibility improvements for my changes. N.A.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. N.A.

UI changes testing checklist: Not a UI PR.

@mokagio mokagio added this to the Someday milestone Jul 19, 2023
@mokagio mokagio added the Testing Unit and UI Tests and Tooling label Jul 19, 2023
@mokagio
Copy link
Contributor Author

mokagio commented Jul 19, 2023

@tiagomar @jostnes you have been doing lots of great work in the UI tests recently. This PR is still in draft because I haven't seen the tests pass and I'll clock off before it finishes, but I'd still love your input.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 19, 2023

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr21134-59cb230
Version22.8
Bundle IDcom.jetpack.alpha
Commit59cb230
App Center Buildjetpack-installable-builds #5456
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 19, 2023

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr21134-59cb230
Version22.8
Bundle IDorg.wordpress.alpha
Commit59cb230
App Center BuildWPiOS - One-Offs #6428
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@mokagio mokagio marked this pull request as ready for review July 20, 2023 03:09
@mokagio mokagio requested a review from a team as a code owner July 20, 2023 03:09
@jostnes
Copy link
Contributor

jostnes commented Jul 20, 2023

👋 @mokagio sorry for the delay

From a quick look, my first suggestion would be to use textViews too, since that didn't work, wondering if you have tried that but without .firstMatch? i.e. app.textViews["Post title. Empty"] as that should only match one element in the element tree so don't think it needs .firstMatch

Another one is probably to try using app.otherElements["post-title"] because only one of the post title's otherElements has a unique identifier

(both 👆 work locally but have not been tested in CI so not sure how helpful they'll be)

@fluiddot
Copy link
Contributor

@jostnes Following this comment I shared in the other PR, I tried the approaches you shared but none seemed to work when running the tests locally in a simulator. You mentioned that worked on your end, but I assume it worked because this PR is not yet using the new React Native version 0.71.11. Once we merge #20956, it's likely that won't work.

@jostnes
Copy link
Contributor

jostnes commented Jul 21, 2023

@jostnes Following this comment I shared in the other PR, I tried the approaches you shared but none seemed to work when running the tests locally in a simulator. You mentioned that worked on your end, but I assume it worked because this PR is not yet using the new React Native version 0.71.11. Once we merge #20956, it's likely that won't work.

@fluiddot is #20956 the PR with the new RN version? i can take a look if there are other alternatives while using that PR and will let y'all know if there are any alternatives. in the meantime, using @mokagio's would be the best bet for now

@fluiddot
Copy link
Contributor

@fluiddot is #20956 the PR with the new RN version?

Yes, that PR uses the new RN versions but also incorporates the workaround Gio provided (#21143).

I can take a look if there are other alternatives while using that PR and will let y'all know if there are any alternatives. in the meantime, using @mokagio's would be the best bet for now

Great, thanks! I couldn't narrow down the culprit of the issue, but it's likely that the new version of RN is rendering text elements differently than before 🤔 .

@jostnes
Copy link
Contributor

jostnes commented Jul 21, 2023

i found the reason it stopped working after the RN update. before the update the element tree looks like this:

                                                Other, 0x7f93a8810550, {{0.0, 74.0}, {820.0, 175.0}}, label: 'Post title. Empty Start writing… Vertical scroll bar, 1 page Horizontal scroll bar, 1 page Add paragraph block'
                                                  Other, 0x7f93a8810670, {{0.0, 74.0}, {820.0, 57.0}}, label: 'Post title. Empty'
                                                    Other, 0x7f93a8810790, {{120.0, 74.0}, {580.0, 57.0}}, label: 'Post title. Empty'
                                                      Other, 0x7f93a88108b0, {{120.0, 74.0}, {580.0, 57.0}}, label: 'Post title. Empty'
                                                        Other, 0x7f93a88109d0, {{120.0, 86.0}, {580.0, 45.0}}, identifier: 'post-title', label: 'Post title. Empty'
                                                          Other, 0x7f93a8810af0, {{136.0, 92.0}, {548.0, 33.0}}, label: 'Post title. Empty'
                                                            TextView, 0x7f93a8810c10, {{136.0, 92.0}, {548.0, 33.0}}, label: 'Post title. Empty', value: ​, Keyboard Focused
                                                              StaticText, 0x7f93a8810d30, {{136.0, 92.0}, {548.0, 33.0}}, label: 'Add title'

see that TextView has Keyboard Focused value and this is what typeText() command looks for (from the doc: The element or a descendant must have keyboard focus; otherwise, an error is raised.)

after the update, this is the element tree:

                                                Other, 0x7fac2bd21900, {{0.0, 74.0}, {820.0, 175.0}}, label: 'Post title. Empty Start writing… Vertical scroll bar, 1 page Horizontal scroll bar, 1 page Add paragraph block'
                                                  Other, 0x7fac2bd21a20, {{0.0, 74.0}, {820.0, 57.0}}, label: 'Post title. Empty'
                                                    Other, 0x7fac2bd21b40, {{120.0, 74.0}, {580.0, 57.0}}, label: 'Post title. Empty'
                                                      Other, 0x7fac2bd21c60, {{120.0, 74.0}, {580.0, 57.0}}, label: 'Post title. Empty'
                                                        Other, 0x7fac2bd21d80, {{120.0, 86.0}, {580.0, 45.0}}, identifier: 'post-title', label: 'Post title. Empty'
                                                          Other, 0x7fac2bd21ea0, {{136.0, 92.0}, {548.0, 33.0}}, label: 'Post title. Empty'
                                                            TextView, 0x7fac2bd21fc0, {{136.0, 92.0}, {548.0, 33.0}}, label: 'Post title. Empty', value: ​
                                                              StaticText, 0x7fac2bd220e0, {{136.0, 92.0}, {548.0, 33.0}}, label: 'Add title'

TextView no longer has Keyboard Focused. So using typeText() won't work for any elements with the 'Post title. Empty' label in this tree.

is this something that could be fixed from RN side?

another thing we could try is using the keyboard to send the individual keys of the text, which is also a bit hacky and might take a longer time. i think for now, the paste text solution is the best bet for this not to fail

@fluiddot
Copy link
Contributor

Nice findings @jostnes, thanks for investigating the issue 🙇 !

is this something that could be fixed from RN side?

Good question. I haven't found any open ticket in the React Native repository related to this. So, I'm not sure if this is a known issue. We'd need to investigate how the new version of RN is rendering TextView elements, and if there's a breakage, open a GitHub issue there.

another thing we could try is using the keyboard to send the individual keys of the text, which is also a bit hacky and might take a longer time. i think for now, the paste text solution is the best bet for this not to fail

Yeah, I agree. For now, the approach of pasting text should work. Maybe in the future, if we need to check a specific behavior in the editor that needs typing, we can explore an alternative.

@mokagio
Copy link
Contributor Author

mokagio commented Jul 24, 2023

Given the issue has been verified in different ways to be related to the React Native upgrade, and that this work is already part of the corresponding branch here, see 4530832, I'm going to close this PR.

@mokagio mokagio closed this Jul 24, 2023
@mokagio mokagio deleted the mokagio/update-ui-tests-for-gutenberg-xcframework branch July 24, 2023 11:33
@mokagio mokagio removed this from the Someday milestone Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Testing Unit and UI Tests and Tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants