Conversation
cb08cce to
db104b1
Compare
db104b1 to
5e48c49
Compare
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! 😄 |
jkmassel
left a comment
There was a problem hiding this comment.
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 ![]()
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) apiiOS uses the
typemethod for entering text. The reason Andorid has not been using thetypemethod is because (only on Android) thetypemethod deletes all of the text in theEditTextbefore 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 totypeinto 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
typeAPI, 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 thetypeapi "clears" the list bullet, which seems to corrupt the block.One nice side benefit of using the
typeapi 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
typeapi 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
RELEASE-NOTES.txtif necessary.