Skip to content
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

[RN Mobile] - Add helper to wait for screen to load before continuing test #39377

Merged

Conversation

jostnes
Copy link
Contributor

@jostnes jostnes commented Mar 11, 2022

What?

This PR attempts to fix the flakiness we're seeing on the Android test by waiting for the expected screen to load before continuing the test

Multiple test runs were done on this draft PR and the results look to be promising

Why?

To reduce flakiness on Android builds tests. This PR is a follow up to the issue in this PR

How?

This PR removes the implicit waits that are being called during initialization and replaces them with a new waitForVisible() function that waits for the screen to load. The wait time is currently set to 25 seconds (adjustable if we decided this is not enough in the future). If the screen loads earlier, it will stop waiting and continue with the test. If the screen does not load within 25 seconds, an error will be thrown.

Currently being used on editor screen initialization and on getTextViewForHtmlViewContent() as we are seeing failures like this and this that seems to be happening intermittently on that particular function.

Testing Instructions

  1. Run the tests in React Native E2E Tests (Android) and React Native E2E Tests (iOS) and ensure that tests are passing

@fluiddot fluiddot added Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Mar 11, 2022
@fluiddot
Copy link
Contributor

@jostnes I finally added the change for returning the locator within the waitForVisible function in a9298ee. I tested it locally and worked fine, in any case, let me know if you have any feedback about it.

…ests

# Conflicts:
#	packages/react-native-editor/__device-tests__/helpers/utils.js
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jostnes Thank you very much for addressing this, I think this solution will reduce definitively the flakiness on the E2E tests 🎊 .

I re-run the CI job several times and except for one all of them have passed. The error in the failed one doesn't look related to this changes, so I wouldn't block this PR due to that.

Screenshot 2022-03-11 at 18 06 52

Error message of Attempt #2:

[elementByXPath("/hierarchy/android.widget.FrameLayout/android.widget.FrameLayout/android.widget.FrameLayout/android.widget.LinearLayout/android.widget.FrameLayout/android.widget.ListView/android.widget.TextView[9]")] Error response status: 7, , NoSuchElement - An element could not be located on the page using the given search parameters. Selenium error: An element could not be located on the page using the given search parameters.

I also tested the changes locally and worked as expected. At this point, I think it's safe to continue with this approach and merge it 🎊 .

@jostnes
Copy link
Contributor Author

jostnes commented Mar 14, 2022

Thanks for re-running the tests and applying the recommendation! I still don't have the access to merge this PR and rerun one of the failing checks. Can you help with that this time, please? Thanks again!

@fluiddot
Copy link
Contributor

Thanks for re-running the tests and applying the recommendation! I still don't have the access to merge this PR and rerun one of the failing checks. Can you help with that this time, please? Thanks again!

Sure thing, although the failing check (End-to-End Tests / Admin - 2 (pull_request)) keeps failing after retrying. Looks like it's a known issue, as it's also happening across other commits and PRs within Gutenberg, so it should be safe to merge it in its current state. @hypest I'm wondering if you could help us with the merging process, thanks 🙇.

@hypest
Copy link
Contributor

hypest commented Mar 14, 2022

As far as I understand the changes here are not time sensitive, and since there is an effort to fix the failing Admin tests in parallel, let's wait a bit. For context, we try to avoid merging PRs even when the failing jobs are not related to the changes, unless the PR in question is high priority for some reason. Let's circle back in a few hours or tomorrow. Thanks!

@jostnes
Copy link
Contributor Author

jostnes commented Mar 15, 2022

Looks like the issue on the Admin test is fixed 🎉 I've merged trunk which restarted the process and looks to be all green now. @fluiddot can you help with the merge, please? Thanks very much!

@hypest hypest merged commit cecd782 into WordPress:trunk Mar 15, 2022
@github-actions github-actions bot added this to the Gutenberg 12.9 milestone Mar 15, 2022
@jostnes jostnes deleted the rnmobile/add-waitforvisible-android-flaky-tests branch March 15, 2022 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - Automation Label used to initiate Mobile App PR Automation Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants