1477807 - Distinguish ADBTimeoutErrors from other exceptions in Android remote tests

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Bob Clary [:bc] (inactive)]

There are several places in our test automation which do not distinguish between normal exceptions which may not indicate a problem with the device under test and ADBTimeoutErrors which typically are fatal to a test run. This leads to situations where the test framework continues to attempt to contact the device resulting in multiple ADBTimeoutErrors. For example,

<https://treeherder.mozilla.org/#/jobs?repo=try&revision=21f968df8951596158030b40c9e96060505baaf4&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=188983291>

<https://treeherder.mozilla.org/logviewer.html#?job_id=188983291&repo=try&lineNumber=2526>

which took 42 minutes to complete due to 4 ADBTimeoutErrors.

The following shows the effect of handling ADBTimeoutErrors separately where a single ADBTimeoutError results in a run time of 27 minutes.

<https://treeherder.mozilla.org/#/jobs?repo=try&revision=a476ade1be650715aa69861bec8eacc4fbe461d2&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=189292242>

<https://treeherder.mozilla.org/logviewer.html#?job_id=189292242&repo=try&lineNumber=2540>

A full run of android tests with this patch on try:

<https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef305f6ec28913e6ed2bf7d79b2e616461d49329&filter-tier=1&filter-tier=2&filter-tier=3>

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: adb-exceptions.patch

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: adb-exceptions.patch

My previous patch left out the part which prevented the clean up and device info from running when an ADBTimeoutError occured. The previous g5 06-10 try run did include both patches however so that does show the effect of the ADBTimeoutError. I've folded the two patches together and submitted the result to try again. This does not include the g5s so doesn't have an explicit ADBTimeoutError.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27827ca1d6b76c15fd954f903ca96dc4187f5094&filter-tier=1&filter-tier=2&filter-tier=3

Comment 3

[Geoff Brown [:gbrown]]

Comment on attachment 8994785 [details] [diff] [review]
adb-exceptions.patch

Review of attachment 8994785 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/runtests.py
@@ +65,5 @@
>  from leaks import ShutdownLeaks, LSANLeaks
>  from mochitest_options import (
>      MochitestArgumentParser, build_obj, get_default_valgrind_suppression_files
>  )
> +from mozdevice import ADBTimeoutError

I think runtests.py is otherwise independent of mozdevice or any other Android complications. Does this risk causing mozdevice import failures for folks running 'mach mochitest' locally in desktop-only configurations? Could we live without this change to runtests.py?

Comment 4

[Geoff Brown [:gbrown]]

I took the precaution of pushing to try again and hit consistent robocop failures:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4359d495a651ffd0b1eb455622a3e72bb1058b1

but now I see you already pushed to try in comment 2 and did not hit this problem. Not sure what to make of that...

Comment 5

[Geoff Brown [:gbrown]]

Nevermind -- the robocop failures are unrelated to your changes.

https://bugzilla.mozilla.org/show_bug.cgi?id=1451513#c19

Comment 6

[Bob Clary [:bc] (inactive)]

I've been trying to check if I have any problems running the desktop tests locally but recent nss changes I think have broken my ability to build. Bisecting that at the moment but I'll look into keeping the device related changes in the "remote" device oriented files.

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: followup.patch

Removes need for the import. See https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4252d8ee7addcfdb9609729c38d3203e9cebd8f&filter-tier=1&filter-tier=2&filter-tier=3&filter-searchStr=android-hw

I'll fold this into the previous patch if it is ok with you. I haven't been able to get a build locally but this shouldn't have any effect on non-android platforms.

(Un)fortunately, the new Pixel devices have been having issues but this allows us to test failure paths. The device issues should be resolved going forward I hope.

https://treeherder.mozilla.org/logviewer.html#?job_id=190316822&repo=try&lineNumber=2712 shows hitting the new code.

Other device issues:

https://treeherder.mozilla.org/logviewer.html#?job_id=190314772&repo=try&lineNumber=10

https://treeherder.mozilla.org/logviewer.html#?job_id=190314835&repo=try&lineNumber=1266

https://treeherder.mozilla.org/logviewer.html#?job_id=190316936&repo=try&lineNumber=3043

https://treeherder.mozilla.org/logviewer.html#?job_id=190316876&repo=try&lineNumber=10

https://treeherder.mozilla.org/logviewer.html#?job_id=190316873&repo=try&lineNumber=2019

Comment 8

[Bob Clary [:bc] (inactive)]

fwiw, I finally got a build (must be a compiler error) and mach seems to work fine on reftests and mochitests.

Comment 9

[Geoff Brown [:gbrown]]

Comment on attachment 8995380 [details] [diff] [review]
followup.patch

Review of attachment 8995380 [details] [diff] [review]:
-----------------------------------------------------------------

Clever!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69d756618c48
Distinguish ADBTimeoutErrors from other exceptions in Android remote tests, r=gbrown.

Comment 11

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/69d756618c48