google / mobly

E2E test framework for tests with complex environment requirements.
https://github.com/google/mobly
Apache License 2.0
623 stars 174 forks source link

Progressively increase delay between adb root retries and fix Typo #867

Closed antofara closed 1 year ago

antofara commented 1 year ago

Some low-end devices take more than 3 retries to reconnect after issuing an adb root command


This change is Reviewable

google-cla[bot] commented 1 year ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

xpconanfan commented 1 year ago

Why would the number of retries matter though? I'd imagine the amount of time we wait before we try is more relevant? If you have a way to verify, can you try the strategy where we wait increasingly longer with each retry? E.g. wait 1s after first failure, 3s after second failure.

On Tue, Jan 31, 2023, 00:36 antofara @.***> wrote:

@.**** commented on this pull request.

In mobly/controllers/android_device_lib/adb.py https://github.com/google/mobly/pull/867#discussion_r1091600207:

@@ -29,7 +29,7 @@

Number of attempts to execute "adb root", and seconds for interval time of

this commands.

-ADB_ROOT_RETRY_ATTMEPTS = 3 +ADB_ROOT_RETRY_ATTEMPTS = 5

I found empirically that for low end devices 2 retries is not enough, and on average they need 1-2 more. With 5 I noticed it succeeds 100% of the times. If it helps I can send you the results of a test where I simply keep rebooting the device and monitor the failures to reconnect after reboot.

— Reply to this email directly, view it on GitHub https://github.com/google/mobly/pull/867#discussion_r1091600207, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARDNZJ37CK4MEDVCQIDUMTWVDFKLANCNFSM6AAAAAAULUWCJQ . You are receiving this because you commented.Message ID: @.***>

antofara commented 1 year ago

Both approach are equivalent to support low-end devices, but increasing the wait time goes to the detriment of high end devices that have to wait more unnecessarily. While increasing the attempts lets faster devices resume the test faster. At the moment the delay before retrying is 10 secs. I can increase it to 15 and double it at the second iteration, totalling 45 seconds, that should be ok for the slowest of the devices. But the downside is that faster devices that normally take 16 seconds to reconnect would need to anyways wait 45 seconds.

On Tue, 31 Jan 2023, 10:04 Ang Li, @.***> wrote:

Why would the number of retries matter though? I'd imagine the amount of time we wait before we try is more relevant? If you have a way to verify, can you try the strategy where we wait increasingly longer with each retry? E.g. wait 1s after first failure, 3s after second failure.

On Tue, Jan 31, 2023, 00:36 antofara @.***> wrote:

@.**** commented on this pull request.

In mobly/controllers/android_device_lib/adb.py https://github.com/google/mobly/pull/867#discussion_r1091600207:

@@ -29,7 +29,7 @@

Number of attempts to execute "adb root", and seconds for interval

time of

this commands.

-ADB_ROOT_RETRY_ATTMEPTS = 3 +ADB_ROOT_RETRY_ATTEMPTS = 5

I found empirically that for low end devices 2 retries is not enough, and on average they need 1-2 more. With 5 I noticed it succeeds 100% of the times. If it helps I can send you the results of a test where I simply keep rebooting the device and monitor the failures to reconnect after reboot.

— Reply to this email directly, view it on GitHub https://github.com/google/mobly/pull/867#discussion_r1091600207, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AARDNZJ37CK4MEDVCQIDUMTWVDFKLANCNFSM6AAAAAAULUWCJQ

. You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/google/mobly/pull/867#issuecomment-1409995756, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALVEKTYN43XF453UHD4AYCLWVDISBANCNFSM6AAAAAAULUWCJQ . You are receiving this because you were assigned.Message ID: @.***>

antofara commented 1 year ago

With this strategy (3 attempts, 15 seconds - 30 seconds delay) it works with the low-end devices I'm working with. Let me know what's the preferred alternative, given the aforementioned trade off

xpconanfan commented 1 year ago

Thanks for giving it a try. Based on your info, it sounds like time is definitely the deciding factor here.

So I think increase wait time between the 2nd and 3rd retry is the better strategy. Otherwise, we are essentially using retries as time counters, and assuming retrying five times will exceed the slow device's prep time, which hides the true intention and makes the code implicit

xpconanfan commented 1 year ago

Pls fix the CLA warning