Skip to content

Add retry to waitforscreen#15

Merged
jostnes merged 6 commits intotrunkfrom
add-retry-to-waitforscreen
Aug 3, 2023
Merged

Add retry to waitforscreen#15
jostnes merged 6 commits intotrunkfrom
add-retry-to-waitforscreen

Conversation

@jostnes
Copy link
Copy Markdown
Contributor

@jostnes jostnes commented Aug 1, 2023

Description

Some tests that have 100% reliability on iPhone are flaky on iPad. The top two are:

  1. testWPcomLoginLogout
  2. testCreateScheduledPost

Found that on iPad, certain flows open up a modal on top of a screen instead of redirecting to a new screen. When this happens, sometimes, the test hangs when trying to find the right element on the screen. I suspect that when the test reaches the part of the code to verify for elements, the modal has not rendered correctly, so the test cannot find what it's looking for. This PR adds a retry to waitForScreen so if, at that point, the screen has not been rendered, it can try again (max 3 retries).

Note after testing: For testCreateScheduledPost, found that the reason that this is happening is that the notice disappears before the test has the chance to tap it, this issue will be tackled separately and not fixed by the changes in this PR

One more note: A surprising but pleasant bonus, I discovered that the issue where the first test testInsightsStatsLoadProperly would fail consistently on the first run on the simulator, is resolved in this PR. Please see all the test runs after wordpress-mobile/WordPress-iOS@dc9586c commit in the testing PR

Testing

Using wordpress-mobile/WordPress-iOS#21213 to test the changes, tests ran multiple times and checked that this failure does not happen on iPad for testWPcomLoginLogout test:

Failed to determine hittability of "appSettings" Cell: Activation point invalid and no suggested hit points based on element frame

@jostnes jostnes marked this pull request as ready for review August 2, 2023 03:17
@jostnes jostnes added the enhancement New feature or request label Aug 2, 2023
@jostnes jostnes self-assigned this Aug 2, 2023
Copy link
Copy Markdown

@tiagomar tiagomar left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @jostnes!

The changes look good, can't wait to see it in the wild. :shipit:

Comment on lines +114 to +115
// Wait 1 second before retrying
sleep(1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the sleep necessary when we already wait for waitTimeout?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the sleep is necessary, I'd suggest making it configurable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

in this case, i think it's necessary for that short wait before retrying again and should be a really low number. i'll make it configurable

Comment on lines +112 to +119
} catch {
if currentRetryCount < maxRetries {
// Wait 1 second before retrying
sleep(1)
continue
} else {
throw WaitForScreenError.timedOut
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is the correct syntax, but we should be able to forward the error thrown from the do try part of the code, without having to replicate it.

Suggested change
} catch {
if currentRetryCount < maxRetries {
// Wait 1 second before retrying
sleep(1)
continue
} else {
throw WaitForScreenError.timedOut
}
} catch error {
if currentRetryCount < maxRetries {
// Wait 1 second before retrying
sleep(1)
continue
} else {
throw error
}

@mokagio
Copy link
Copy Markdown
Contributor

mokagio commented Aug 3, 2023

I've been thinking about how to test this in the UI tests for the library, but I can't come up with anything that would be reliable... 🤔

The best I can think of is a screen with a timer. I wonder if that would be robust or flaky. It'd be interesting if it was flaky because it would give us a way to make the library stronger. At the same time, it'd be annoying to have to deal with flaky tests here, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants