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

Android e2e test flakiness fix #1813

Merged
merged 1 commit into from
Jan 27, 2020
Merged

Android e2e test flakiness fix #1813

merged 1 commit into from
Jan 27, 2020

Conversation

mchowning
Copy link
Contributor

@mchowning mchowning commented Jan 24, 2020

Partially addresses #1662

Summary

This PR addresses some of the Android e2e flakiness. In particular, it should resolve the issue where only a part of the desired text would be input on the device, resulting in an expectation failure where the actual text was only the beginning of the expected text.

In short, this PR avoids using ADB to insert text when possible because that was found to not work consistently. For reasons discussed below, we still needed to use ADB to insert text when we wanted to append text, but there are only a few cases where that is necessary (list blocks), and the text being appended there is very short. If that ends up being a problem, we could always just disable the list tests on Android until a better solution is found.

What this PR does NOT do

"Requested environment is unavailable error"

This PR does not address the "requested environment is unavailable" error. I originally believed this error was entirely due to the saucelabs queue getting backed up. I still think that is probably the issue some of the time, but I have since observed the environment unavailable error occur when the saucelabs queue was empty (and the error occurred before saucelabs received a request to run tests, i.e., there was no record of the test on saucelabs).

Rotation test issues

In addition, since upgrading our RN to version 61.5, I have seen intermittent errors locally the rotation tests. I was able to reproduce this error locally even with the changes in this PR, so I don't think this PR will fix that (although I cannot seem to reproduce that error now). In addition, @hypest reported observing these same errors on CI before the RN version upgrade, so it sounds like this issue may be an unrelated to the upgrade.

Background

Using ADB to insert text

In testing, I observed that even locally sometimes only the beginning of the text passed to ADB would get entered. I initially investigated trying to get ADB to work more consistently, but I was not able to find a way to avoid this.

Using Appium's type (aka sendKeys) api

iOS uses the type method for entering text. The reason Andorid has not been using the type method is because (only on Android) the type method deletes all of the text in the EditText before entering the new text. That alone wouldn't be that big of a problem but, in our app, it also removes the block entirely. This means that anytime we tried to type into a block, the block would be removed before the test tried to add the text to the block, resulting in the test throwing an error on account of the missing block. This is why we were using ADB.

It turns out, however, that as long as the block contains some text, then the block is not removed. Therefore, in this PR, I am using ADB to insert a single space into a block before using the type API, which removes that space (but not the entire block) and then inserts the desired text. If we want to append text however, we still must use ADB. Fortunately, at this point the list block tests are the only tests that require appending text. This is because with list blocks the type api "clears" the list bullet, which seems to corrupt the block.

One nice side benefit of using the type api more, is that it seems to enter text a fair bit quicker than ADB, so it speeds up the e2e tests a bit.

A Possible Alternative Approach

One possible alternative approach would be to have Android only use the type api and simply disable the list tests on Android since those are the only tests that require the ability to append text. I'm trying to avoid this solution because it both (1) reduces Android's test coverage and (2) prevents us from writing any new tests that need "append" type behavior. If, however, we find there are issues with the approach taken with this PR, it may be worth considering this as an alternative.

To Test

Confirm that e2e tests pass on both platforms.

At the time I moved this from being a draft PR to being Ready for Review, the Android e2e tests for the complete set of changes in this PR has passed all 10 times I ran it on CI (the old failures of this PR were when I was still working on the fix) and many more times locally. Interestingly, there was one failure with these changes on CI, but this was on iOS and I suspect it was unrelated to this PR.

PR Submission Checklist

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@mchowning mchowning force-pushed the android_e2e_typing_fix branch from cb08cce to db104b1 Compare January 24, 2020 21:30
@mchowning mchowning force-pushed the android_e2e_typing_fix branch from db104b1 to 5e48c49 Compare January 27, 2020 11:57
@mchowning mchowning marked this pull request as ready for review January 27, 2020 13:46
@mchowning mchowning changed the title TESTING - Android e2e test flakiness fix Android e2e test flakiness fix Jan 27, 2020
@mchowning mchowning added the Testing Anything related to automated tests label Jan 27, 2020
@mchowning mchowning added this to the 1.22 milestone Jan 27, 2020
@mchowning mchowning requested a review from marecar3 January 27, 2020 13:47
@hypest
Copy link
Contributor

hypest commented Jan 27, 2020

In addition, @hypest reported observing these same errors on CI before the RN version upgrade

Just to be clear, what I've been seeing even before the upgrade to 0.61.5 is that the rotation test fails way too often, and the error is always that the text read back is only a part of the start of the original string sent.

@mchowning
Copy link
Contributor Author

Just to be clear, what I've been seeing even before the upgrade to 0.61.5 is that the rotation test fails way too often, and the error is always that the text read back is only a part of the start of the original string sent.

Ah, thanks for clarifying @hypest. In that case, this PR should fix that as well! 😄

Copy link
Contributor

@jkmassel jkmassel left a comment

Choose a reason for hiding this comment

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

This approach makes a lot of sense, and the "add a space that gets deleted" approach is perfect.

I confirmed that there is a long history of passed tests in CircleCI, so I'm good to say :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing Anything related to automated tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants