Skip to content

Add a 250ms buffer between withTimeout retries#2931

Open
Fishbowler wants to merge 1 commit intomainfrom
kinder_retries
Open

Add a 250ms buffer between withTimeout retries#2931
Fishbowler wants to merge 1 commit intomainfrom
kinder_retries

Conversation

@Fishbowler
Copy link
Contributor

Proposed changes

Right now, MaestroTimer.withTimeout is a dumb do/while loop. But it's used for things like assertVisible checks, which means it's really quite intensive on the device. Whilst this slows checks by a quarter of a second, it's likely to speed up a number of tests by not slowing the device or saturating the device link.

A later PR should make this (and other timeouts) configurable via config.yaml

Testing

Wikipedia flows look good locally. Excited to see what happens in CI...

Issues fixed

Copy link
Collaborator

@amanjeetsingh150 amanjeetsingh150 left a comment

Choose a reason for hiding this comment

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

I'm not fan of putting magic number especially sleeps in code. For example right now, I'm not even sure what problem this solves:

  • Reading PR description, I see "to reduce intensive check on device"? Why is it hurting us? What does it mean 🤔 ?
  • This could have a ripple effect on existing passing tests. Some tests may rely on certain timing assumptions, and introducing sleeps could cause them to become flaky or fail unexpectedly.
  • I agree the current logic isn’t ideal, and there are already places with sleeps in the codebase. That’s also what makes it harder to reason about and maintain over time. I’d love to understand the problem in more detail, ideally with an example, so we can explore alternatives to hardcoded sleeps.

@Fishbowler
Copy link
Contributor Author

This code can't rely on an effect for it's timeout, since it's the code that's being used by other commands to wait for an effect. This is what polls for assertVisible, etc. We can only retry.

Right now there's a do/while which will run at the top speed of the smallest bottleneck.

I'll see if I can draw out something more empirical.

@simon-gilmurray
Copy link
Contributor

Hey @amanjeetsingh150
This original discussion stemmed from a thread on the new addition of a sleep command - I spoke about numerous scenarios where I'd found a JS sleep beneficial in my Maestro tests.
One of these scenarios was when I started automating an RN app with Maestro - using extendedWaitUntil would cause the device to freeze when on the splash/loading screen. To get around this, I had to use a fixed sleep to bypass loading.

I believe Dan is searching for an improvement that would alleviate this scenario and help with some more intensive device querying - although as I mentioned on the thread, I'm not sure slowing everything down by 250ms is the best solution - but it is a tricky one to solve!

@Fishbowler
Copy link
Contributor Author

This will only add a pause after a failure, before the next attempt. I think on unaffected devices, the difference wouldn't be perceptible. On some hosts, this delay will be shorter than the bottleneck was, and yet could still see shortened test times.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants