1426586 - Autophone - execute Autophone Device tests via Taskcluster

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, enhancement)

Description

[Bob Clary [:bc] (inactive)]

Autophone executes performance tests and unittests on real Android devices. It exists in https://github.com/mozilla/autophone and as such is completely outside the normal testing infrastructure.

By converting Autophone to use the Taskcluster infrastructure to execute Gecko tests we can obtain parity with other test frameworks and promote the maintainability and scalability of Android device testing.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: wip android-hw.patch

Add support for android hardware using taskcluster workers.

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: wip add-android-hw-common-tests.patch

Add android-hw-common-tests to taskcluster/ci/test/test-sets.yml

Comment 3

[Bob Clary [:bc] (inactive)]

Attachment: wip gecko-t-ap-unit-workertypes.patch

Add worker types proj-autophone/gecko-t-ap-unit-gs3, proj-autophone/gecko-t-ap-unit-pix for android-hw-common-tests

Comment 4

[Bob Clary [:bc] (inactive)]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0204bf1e6f3720d2f4edd68e5843e15aa40e8de

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: wip android-hw.patch

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: wip debug-artifacts.patch

Will probably remove this altogether but it is helpful atm.

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: wip add-android-hw-common-tests.patch

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: wip gecko-t-ap-unit-workertypes-gs3.patch

Comment 9

[Bob Clary [:bc] (inactive)]

Attachment: wip gecko-t-ap-unit-workertypes-pix.patch

Comment 10

[Bob Clary [:bc] (inactive)]

Attachment: wip gecko-t-ap-unit-android-hw-common-tests.patch

Comment 11

[Bob Clary [:bc] (inactive)]

Attachment: wip adb-chmod-sdcard-root.patch

I'll ask for review of this in Bug 1440714.

Comment 12

[Bob Clary [:bc] (inactive)]

Attachment: wip adb-test-root-root.patch

I'll ask for review of this in Bug 1440714.

Comment 13

[Bob Clary [:bc] (inactive)]

Attachment: wip remotexpcshelltests.patch

I'll ask for review of this in Bug 1440714.

Comment 14

[Bob Clary [:bc] (inactive)]

Attachment: wip jittests.patch

I'll ask for review of this in Bug 1440714.

Comment 15

[Bob Clary [:bc] (inactive)]

Attachment: wip arm64-build-test-platforms.patch

Comment 16

[Bob Clary [:bc] (inactive)]

Attachment: wip gecko-t-ap-unit-android-hw-common-tests-arm64.patch

Comment 17

[Bob Clary [:bc] (inactive)]

A try run with the above patches using a local gs3 and bitbar pixels:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=44a59658315a960146d3d5060cb8913872ab9af3

Comment 18

[Bob Clary [:bc] (inactive)]

Attachment: android_hardware_unittest.patch

Basically a modified clone of the android emulator mozharness config and script.

Geoff: I think we have an opportunity later to consolidate the emulator and hardware scripts since they are so very similar but I think it will be best if we keep them separate until we are in a good place.

Comment 19

[Bob Clary [:bc] (inactive)]

Attachment: mozharness-test-job-transform.patch

Jonas: This add support for the 'script-engine' to the job transforms. It is (as noted in the comments) a clone of the code for 'native-engine'.

Comment 20

[Bob Clary [:bc] (inactive)]

Attachment: android-em.patch

Geoff: This renames the android test platforms to be android-em so that we can distinguish between emulators and hardware.

Comment 21

[Bob Clary [:bc] (inactive)]

Attachment: test-platform-assignments.patch

Geoff, Joel: This adds the android hardware to the test platforms. Initially, I'm only enabling this on try until we have this running smoothly there.

Comment 22

[Bob Clary [:bc] (inactive)]

Attachment: add-android-hw-common-tests.patch

Geoff, Joel: This defines the android-hw-common-tests which lists the tests being enabled for android hardware.

Comment 23

[Bob Clary [:bc] (inactive)]

Attachment: gecko-t-ap-unit-workertypes.patch

Geoff, Joel: This patch assigns the android-hw-common-tests to the various supported hardware and identifies the worker types and engine used in the taskcluster-workers.

Comment 24

[Bob Clary [:bc] (inactive)]

Attachment: gecko-t-ap-unit-android-hw-common-tests.patch

Jonas, Geoff, Joel: This explicitly sets the worker types for the hardware. I'm not totally certain that using null for the default test platform is right, but it seems to work.

Comment 25

[Bob Clary [:bc] (inactive)]

try runs with only slight differences with current patch versions:
<https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&filter-tier=1&filter-tier=2&filter-tier=3&fromchange=1c3d1c2613ee346cb2dcae5ce83b9db06dec1420>

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8982220 [details] [diff] [review]
test-platform-assignments.patch

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

some small nits- happy to consider them in a followup or carry the r+ forward

::: taskcluster/ci/test/misc.yml
@@ +85,5 @@
>      chunks:
>          by-test-platform:
>              # android-em-4.3-arm7-api-16/debug -- not run
>              android-em-4.3-arm7-api-16/opt: 4
> +            android-hw.*: 4

could we do |android.*: 4|

@@ +188,5 @@
>      allow-software-gl-layers: false
>      run-on-projects:
>          by-test-platform:
> +            # Do not run on android hardware
> +            android-hw.*: []

if we want test-verify to run on hardware, test-verify-gpu should run; but it will need a little bit of tweaking.  we only have gpu to split out specific tests that are known to require gpu

::: taskcluster/ci/test/mochitest.yml
@@ +46,5 @@
>      instance-size:
>          by-test-platform:
>              linux64-jsdcov/opt: xlarge
>              android-em.*: xlarge
> +            android-hw.*: large

do we need instance-size here?  isn't it ignored?

::: taskcluster/ci/test/reftest.yml
@@ +46,5 @@
>      chunks:
>          by-test-platform:
>              android-em-4.3-arm7-api-16/debug: 10
>              android-em-7.0-x86/opt: 1
> +            android.*: 4

I would make the 1 or 2;  opt could run in ~20 minutes and debug ~45 minutes with 1 chunk.

@@ +77,5 @@
>          by-test-platform:
>              android-em-4.3-arm7-api-16/debug: 100
>              android-em-7.0-x86/opt: 3
>              android-em.*: 40
> +            android-hw.*: 8

this is good if we have adjust the 7200 second timeout down to 3600;  debug is <45 minutes; opt (g5 and p2) < 25 minutes.

@@ +118,5 @@
>          by-test-platform:
>              android-em-4.3-arm7-api-16/debug: 56
>              android-em-7.0-x86/opt: 5
>              android-em.*: 28
> +            android-hw.*: 8

I would argue for 4 chunks

@@ +131,5 @@
>              linux32/debug: both
>              default: true
>      max-run-time:
>          by-test-platform:
> +            android.*: 7200

current runtime with 8 chunks is <20 minutes/chunk; if we go with 4 chunks can we use 3600;  if we keep 8, can we do 1800?

::: taskcluster/ci/test/test-platforms.yml
@@ +263,5 @@
>  
>  ##
>  # Android platforms (matching /android-em.*/)
> +#
> +# android-em test platforms execute on android emulators.

do we need to add android-hw here?

Comment 27

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8982223 [details] [diff] [review]
add-android-hw-common-tests.patch

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

::: taskcluster/ci/test/test-sets.yml
@@ +372,5 @@
> +    - mochitest-webgl
> +    - reftest
> +    - robocop
> +    - test-verify
> +    - xpcshell

I want to be careful that we do not allow this for motog5 as that is designed just for performance tests.

we will probably run many of these tests on aarch64, but not regular p2.  The reason why is emulators will get us the regular arm coverage.

I would like to rethink this a bit more.

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8982224 [details] [diff] [review]
gecko-t-ap-unit-workertypes.patch

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

::: taskcluster/ci/test/test-platforms.yml
@@ +311,5 @@
> +    build-platform: android-api-16/opt
> +    test-sets:
> +        - android-hw-common-tests
> +
> +android-hw-p2-8-0-android-aarch64/opt:

do we need a debug here?

::: taskcluster/taskgraph/util/workertypes.py
@@ +49,5 @@
>      "scriptworker-prov-v1/shipit-dev": ('shipit', None),
>      "scriptworker-prov-v1/treescript-v1": ('treescript', None),
>      'releng-hardware/gecko-t-osx-1010': ('generic-worker', 'macosx'),
> +    'proj-autophone/gecko-t-ap-unit-m5': ('script-engine', 'linux'),
> +    'proj-autophone/gecko-t-ap-unit-p2': ('script-engine', 'linux'),

proj-autophone?  is that hardcoded into taskcluster?  I think it is a fine name, just curious.

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8982225 [details] [diff] [review]
gecko-t-ap-unit-android-hw-common-tests.patch

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

could we do this with a transform instead of repeated code over and over again?  The changes look fine, but I really don't like the cut/paste style of we have a cleaner solution.  Possibly there is a good reason for not using a transform.

Comment 30

[Bob Clary [:bc] (inactive)]

(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #26)
> Comment on attachment 8982220 [details] [diff] [review]
> test-platform-assignments.patch
> 
> Review of attachment 8982220 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> some small nits- happy to consider them in a followup or carry the r+ forward
> 
> ::: taskcluster/ci/test/misc.yml
> @@ +85,5 @@
> >      chunks:
> >          by-test-platform:
> >              # android-em-4.3-arm7-api-16/debug -- not run
> >              android-em-4.3-arm7-api-16/opt: 4
> > +            android-hw.*: 4
> 
> could we do |android.*: 4|

It depends if we/gbrown want to treat all of the android's the same. We have android-em-(4.3-arm|4.2-x86|7.0-x86)

> 
> @@ +188,5 @@
> >      allow-software-gl-layers: false
> >      run-on-projects:
> >          by-test-platform:
> > +            # Do not run on android hardware
> > +            android-hw.*: []
> 
> if we want test-verify to run on hardware, test-verify-gpu should run; but
> it will need a little bit of tweaking.  we only have gpu to split out
> specific tests that are known to require gpu
> 

It wasn't (isn't) clear to me but I wanted to make sure I wasn't requesting a virtual machine with gpu somewhere unintentionally. We can try it.

> ::: taskcluster/ci/test/mochitest.yml
> @@ +46,5 @@
> >      instance-size:
> >          by-test-platform:
> >              linux64-jsdcov/opt: xlarge
> >              android-em.*: xlarge
> > +            android-hw.*: large
> 
> do we need instance-size here?  isn't it ignored?

It is ignored due to the later patch that explicitly sets the workertype but it is required. Again I picked large to keep from accidentally asking for an xlarge. The schema doesn't allows us to say null or none or something else as far as I know.

> 
> ::: taskcluster/ci/test/reftest.yml
> @@ +46,5 @@
> >      chunks:
> >          by-test-platform:
> >              android-em-4.3-arm7-api-16/debug: 10
> >              android-em-7.0-x86/opt: 1
> > +            android.*: 4
> 
> I would make the 1 or 2;  opt could run in ~20 minutes and debug ~45 minutes
> with 1 chunk.

I had

android-em.*: 4
android-hw.*: 4

I don't think we want the emulator to use 1 or 2.
We'd probably want to distinguish the m5 since it appears to be much slower.
I could split out android-em-4, android-em-7, android-hw-p2, android-hw-m5 and reduce the chunks for the p2.

> 
> @@ +77,5 @@
> >          by-test-platform:
> >              android-em-4.3-arm7-api-16/debug: 100
> >              android-em-7.0-x86/opt: 3
> >              android-em.*: 40
> > +            android-hw.*: 8
> 
> this is good if we have adjust the 7200 second timeout down to 3600;  debug
> is <45 minutes; opt (g5 and p2) < 25 minutes.
> 
> @@ +118,5 @@
> >          by-test-platform:
> >              android-em-4.3-arm7-api-16/debug: 56
> >              android-em-7.0-x86/opt: 5
> >              android-em.*: 28
> > +            android-hw.*: 8
> 
> I would argue for 4 chunks

Ok.

> 
> @@ +131,5 @@
> >              linux32/debug: both
> >              default: true
> >      max-run-time:
> >          by-test-platform:
> > +            android.*: 7200
> 
> current runtime with 8 chunks is <20 minutes/chunk; if we go with 4 chunks
> can we use 3600;  if we keep 8, can we do 1800?

Again I combined the android-hw with the previous android-em. I can split out android-hw and reduce it.

> 
> ::: taskcluster/ci/test/test-platforms.yml
> @@ +263,5 @@
> >  
> >  ##
> >  # Android platforms (matching /android-em.*/)
> > +#
> > +# android-em test platforms execute on android emulators.
> 
> do we need to add android-hw here?

In follow up patch.


(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #27)
> Comment on attachment 8982223 [details] [diff] [review]
> add-android-hw-common-tests.patch
> 
> Review of attachment 8982223 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: taskcluster/ci/test/test-sets.yml
> @@ +372,5 @@
> > +    - mochitest-webgl
> > +    - reftest
> > +    - robocop
> > +    - test-verify
> > +    - xpcshell
> 
> I want to be careful that we do not allow this for motog5 as that is
> designed just for performance tests.
> 
> we will probably run many of these tests on aarch64, but not regular p2. 
> The reason why is emulators will get us the regular arm coverage.
> 
> I would like to rethink this a bit more.

I was planning on making the assignments via the workertype via gecko-t-ap-unit-android-hw-common-tests.patch. That would give us fine control over what is run without having lots of test sets. I can adapt to the preferred style whatever we decide that is.


(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #28)
> Comment on attachment 8982224 [details] [diff] [review]
> gecko-t-ap-unit-workertypes.patch
> 
> Review of attachment 8982224 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: taskcluster/ci/test/test-platforms.yml
> @@ +311,5 @@
> > +    build-platform: android-api-16/opt
> > +    test-sets:
> > +        - android-hw-common-tests
> > +
> > +android-hw-p2-8-0-android-aarch64/opt:
> 
> do we need a debug here?
> 

There isn't one (yet). The way I did it originally I didn't make the distinction and got messages in the taskgraph generation about there not being a android-hw-p2-8-0-android-aarch64/debug and the task being dropped so I was very explicit. I can look at it more.

> ::: taskcluster/taskgraph/util/workertypes.py
> @@ +49,5 @@
> >      "scriptworker-prov-v1/shipit-dev": ('shipit', None),
> >      "scriptworker-prov-v1/treescript-v1": ('treescript', None),
> >      'releng-hardware/gecko-t-osx-1010': ('generic-worker', 'macosx'),
> > +    'proj-autophone/gecko-t-ap-unit-m5': ('script-engine', 'linux'),
> > +    'proj-autophone/gecko-t-ap-unit-p2': ('script-engine', 'linux'),
> 
> proj-autophone?  is that hardcoded into taskcluster?  I think it is a fine
> name, just curious.

Well, not in taskcluster itself but in the taskcluster client configurations. We picked it early on and have just kept running with it.

Comment 31

[Bob Clary [:bc] (inactive)]

(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #29)
> Comment on attachment 8982225 [details] [diff] [review]
> gecko-t-ap-unit-android-hw-common-tests.patch
> 
> Review of attachment 8982225 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> could we do this with a transform instead of repeated code over and over
> again?  The changes look fine, but I really don't like the cut/paste style
> of we have a cleaner solution.  Possibly there is a good reason for not
> using a transform.

We might be able to do so, but we need to make sure we have an explicit worker type so that we take the first branch in
https://searchfox.org/mozilla-central/source/taskcluster/taskgraph/transforms/tests.py#918

Comment 32

[Bob Clary [:bc] (inactive)]

PS. This also gives us very visible fine control over which worker types are used to run the tests. I like that. We could easily modify these so that there are different worker types defined for the tests rather than the identical ones I am starting with. For example, p2 aarch64 but not arm7 or p2 but not m5 etc.

I'm not sure but it seems at least until we know what we will be running it would be confusing to do this as a transform. But good idea in general.

Your call.

Comment 33

[Geoff Brown [:gbrown]]

Comment on attachment 8982220 [details] [diff] [review]
test-platform-assignments.patch

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

::: taskcluster/ci/test/misc.yml
@@ +85,5 @@
>      chunks:
>          by-test-platform:
>              # android-em-4.3-arm7-api-16/debug -- not run
>              android-em-4.3-arm7-api-16/opt: 4
> +            android-hw.*: 4

I think |android.*: 4| would be okay. Maybe remove the "not run" comment at the same time.

::: taskcluster/ci/test/mochitest.yml
@@ +402,5 @@
>      loopback-video: true
>      max-run-time:
>          by-test-platform:
>              windows.*: 5400
> +            android.*: 7200

The 7200 seconds (2 hours) for emulator is pretty exceptional. If you can run faster on hardware, you should reduce this, ideally to the 3600 default.

Comment 34

[Geoff Brown [:gbrown]]

Comment on attachment 8982217 [details] [diff] [review]
android_hardware_unittest.patch

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

Let's be sure to follow up on --total-chunks and --deviceSerial for geckoview-junit.

I wouldn't mind seeing android_common.py leveraged before check-in, but we can re-visit that later, perhaps when we consolidate emulator and hardware scripts.

::: testing/mozharness/configs/android/android_hw.py
@@ +32,5 @@
> +    "find_links": [
> +        "https://pypi.pub.build.mozilla.org/pub",
> +    ],
> +    "pip_index": False,
> +    "suite_definitions": {

Oh no! I had really hoped you would re-use android_common.py. I notice the extra log- arguments and --deviceSerial. Probably those could be used by the emulator as well....

@@ +53,5 @@
> +                "--log-tbpl-level=%(log_tbpl_level)s",
> +                "--extra-profile-file=fonts",
> +                "--extra-profile-file=hyphenation",
> +                "--screenshot-on-fail",
> +                "--total-chunks=20",

Why --total-chunks? That should come from the taskcluster yml.

@@ +75,5 @@
> +                "--log-raw-level=%(log_raw_level)s",
> +                "--log-errorsummary=%(error_summary_file)s",
> +                "--log-tbpl-level=%(log_tbpl_level)s",
> +                "--screenshot-on-fail",
> +                "--total-chunks=10",

Ditto.

@@ +340,5 @@
> +            "options": [
> +                "--certificate-path=%(certs_path)s",
> +                "--remote-webserver=%(remote_webserver)s",
> +                "--symbols-path=%(symbols_path)s",
> +                "--utility-path=%(utility_path)s",

Why no --deviceSerial?

::: testing/mozharness/scripts/android_hardware_unittest.py
@@ +310,5 @@
> +    def _restart_adbd(self):
> +        self._run_with_timeout(30, [self.adb_path, 'kill-server'])
> +        self._run_with_timeout(30, [self.adb_path, 'root'])
> +
> +    def _screenshot(self, prefix):

Screenshots are useful to show the state of the emulator, but probably not useful for bitbar?

Comment 35

[Geoff Brown [:gbrown]]

Comment on attachment 8982225 [details] [diff] [review]
gecko-t-ap-unit-android-hw-common-tests.patch

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

I don't have a strong opinion on transform vs yml configuration.

Comment 36

[Geoff Brown [:gbrown]]

Comment on attachment 8982223 [details] [diff] [review]
add-android-hw-common-tests.patch

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

That's an impressive list of tests!

When I read that, it does suggest to me "all of these tests will be run on all android-hw devices". If that's not the case, I'd be more inclined to have multiple android-hw test sets defined here. But I'm not sure I understand all the consideration...maybe bc and jmaher should work this out.

Comment 37

[Geoff Brown [:gbrown]]

Comment on attachment 8982217 [details] [diff] [review]
android_hardware_unittest.patch

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

::: testing/mozharness/scripts/android_hardware_unittest.py
@@ +543,5 @@
> +        logcat_filename = 'logcat-%s.log' % self.device_serial
> +        logcat_path = os.path.join(self.abs_dirs['abs_blob_upload_dir'],
> +                                   logcat_filename)
> +        self.logcat_file = open(logcat_path, 'w')
> +        logcat_cmd = [self.adb_path, '-s', self.device_serial, '-v',

oops! Missing 'logcat' here, so your logs show:

11:27:27     INFO - adb -s ZY322LDS6T -v threadtime Trace:S StrictMode:S ExchangeService:S
11:27:27     INFO - Running timeout 30 adb -s ZY322LDS6T shell ps
adb: usage: unknown command -v

...and you are missing the logcat artifact!

Comment 38

[Bob Clary [:bc] (inactive)]

(In reply to Geoff Brown [:gbrown] from comment #33)
> Comment on attachment 8982220 [details] [diff] [review]
> test-platform-assignments.patch
> 
> Review of attachment 8982220 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: taskcluster/ci/test/misc.yml
> @@ +85,5 @@
> >      chunks:
> >          by-test-platform:
> >              # android-em-4.3-arm7-api-16/debug -- not run
> >              android-em-4.3-arm7-api-16/opt: 4
> > +            android-hw.*: 4
> 
> I think |android.*: 4| would be okay. Maybe remove the "not run" comment at
> the same time.
> 

Ok.

> ::: taskcluster/ci/test/mochitest.yml
> @@ +402,5 @@
> >      loopback-video: true
> >      max-run-time:
> >          by-test-platform:
> >              windows.*: 5400
> > +            android.*: 7200
> 
> The 7200 seconds (2 hours) for emulator is pretty exceptional. If you can
> run faster on hardware, you should reduce this, ideally to the 3600 default.

Ok.

(In reply to Geoff Brown [:gbrown] from comment #34)
> Comment on attachment 8982217 [details] [diff] [review]
> android_hardware_unittest.patch
> 
> Review of attachment 8982217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let's be sure to follow up on --total-chunks and --deviceSerial for
> geckoview-junit.
> 

Done.

> I wouldn't mind seeing android_common.py leveraged before check-in, but we
> can re-visit that later, perhaps when we consolidate emulator and hardware
> scripts.
> 

Sounds good.

> ::: testing/mozharness/configs/android/android_hw.py
> @@ +32,5 @@
> > +    "find_links": [
> > +        "https://pypi.pub.build.mozilla.org/pub",
> > +    ],
> > +    "pip_index": False,
> > +    "suite_definitions": {
> 
> Oh no! I had really hoped you would re-use android_common.py. I notice the
> extra log- arguments and --deviceSerial. Probably those could be used by the
> emulator as well....
> 

Ok.

> @@ +53,5 @@
> > +                "--log-tbpl-level=%(log_tbpl_level)s",
> > +                "--extra-profile-file=fonts",
> > +                "--extra-profile-file=hyphenation",
> > +                "--screenshot-on-fail",
> > +                "--total-chunks=20",
> 
> Why --total-chunks? That should come from the taskcluster yml.

Copy/paste from an old version of android_common.py that was removed in Bug 1431433.
I'll remove them all.

> 
> @@ +75,5 @@
> > +                "--log-raw-level=%(log_raw_level)s",
> > +                "--log-errorsummary=%(error_summary_file)s",
> > +                "--log-tbpl-level=%(log_tbpl_level)s",
> > +                "--screenshot-on-fail",
> > +                "--total-chunks=10",
> 
> Ditto.
> 
> @@ +340,5 @@
> > +            "options": [
> > +                "--certificate-path=%(certs_path)s",
> > +                "--remote-webserver=%(remote_webserver)s",
> > +                "--symbols-path=%(symbols_path)s",
> > +                "--utility-path=%(utility_path)s",
> 
> Why no --deviceSerial?
> 

Brain fart? Fixed.

> ::: testing/mozharness/scripts/android_hardware_unittest.py
> @@ +310,5 @@
> > +    def _restart_adbd(self):
> > +        self._run_with_timeout(30, [self.adb_path, 'kill-server'])
> > +        self._run_with_timeout(30, [self.adb_path, 'root'])
> > +
> > +    def _screenshot(self, prefix):
> 
> Screenshots are useful to show the state of the emulator, but probably not
> useful for bitbar?

They can do screenshots for us. I don't see why they wouldn't be useful there as well.

(In reply to Geoff Brown [:gbrown] from comment #36)
> Comment on attachment 8982223 [details] [diff] [review]
> add-android-hw-common-tests.patch
> 
> Review of attachment 8982223 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That's an impressive list of tests!
> 
> When I read that, it does suggest to me "all of these tests will be run on
> all android-hw devices". If that's not the case, I'd be more inclined to
> have multiple android-hw test sets defined here. But I'm not sure I
> understand all the consideration...maybe bc and jmaher should work this out.

Ok. Let's figure this out when we have a better idea of what will be actually running in production.

Thanks!

Comment 39

[Bob Clary [:bc] (inactive)]

(In reply to Geoff Brown [:gbrown] from comment #37)
> Comment on attachment 8982217 [details] [diff] [review]
> android_hardware_unittest.patch
> 
> Review of attachment 8982217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/mozharness/scripts/android_hardware_unittest.py
> @@ +543,5 @@
> > +        logcat_filename = 'logcat-%s.log' % self.device_serial
> > +        logcat_path = os.path.join(self.abs_dirs['abs_blob_upload_dir'],
> > +                                   logcat_filename)
> > +        self.logcat_file = open(logcat_path, 'w')
> > +        logcat_cmd = [self.adb_path, '-s', self.device_serial, '-v',
> 
> oops! Missing 'logcat' here, so your logs show:
> 
> 11:27:27     INFO - adb -s ZY322LDS6T -v threadtime Trace:S StrictMode:S
> ExchangeService:S
> 11:27:27     INFO - Running timeout 30 adb -s ZY322LDS6T shell ps
> adb: usage: unknown command -v
> 
> ...and you are missing the logcat artifact!

doh!

Comment 40

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8982218 [details] [diff] [review]
mozharness-test-job-transform.patch

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

The idea with script-engine is the same as scriptworker, you define a scrip that takes a custom payload.

If you want to use it like native-engine (ie. it has the same payload format), then don't make a new wrapper for it.

The question you have to ask is:
  - What is the payload format
  - How do you write a wrapper for that:
     - is it pass-through without any transforms, or,
     - is it generalized
     (probably not worth the effort)

Hit me up on IRC Wednesday, and we jump on vidyo on chat about this :)

Comment 41

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8982225 [details] [diff] [review]
gecko-t-ap-unit-android-hw-common-tests.patch

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

I have no real insights on this.

Comment 42

[Bob Clary [:bc] (inactive)]

Attachment: 01 android-em.patch

New order for the patches.

This patch is an un bit rotted version of the original. carrying forwards gbrown's r+.

Comment 43

[Bob Clary [:bc] (inactive)]

Attachment: 02 android_hardware_unittest.patch

updated version carrying forward gbrown's r+.

Comment 44

[Bob Clary [:bc] (inactive)]

Attachment: 03 add-android-hw-common-tests.patch

Removed the extra tests and used a different test set name: android-hw-aarch64-unittests. This includes mochitest-media and jit tests on Pixel 2 aarch64.

Comment 45

[Bob Clary [:bc] (inactive)]

Attachment: 04 mozharness-test-job-transform.patch

As we discussed this changes the implementation name to script-engine-autophone.

Comment 46

[Bob Clary [:bc] (inactive)]

Attachment: test-platform-assignments.patch

Reduced to only the mochitest-media and jit tests.

Comment 47

[Bob Clary [:bc] (inactive)]

try run: Fuzzy query=^test-android-hw-p2 jittest | mochitest-media

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

try run: try: -b do -p android-x86,android-api-16,android-aarch64 -u all -t none

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

Only the first two patches android-em.patch, android_hardware_unittest.patch can be landed prior to turning on the tests. The others must land together when the tests are enabled.

Comment 48

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8986358 [details] [diff] [review]
test-platform-assignments.patch

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

nice and simple

Comment 49

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8986356 [details] [diff] [review]
03 add-android-hw-common-tests.patch

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

some small questions, happy to r+ after answers are given in bug or irc

::: taskcluster/ci/test/test-platforms.yml
@@ +309,5 @@
> +    build-platform: android-aarch64/opt
> +    test-sets:
> +        - android-hw-aarch64-unittests
> +
> +android-hw-p2-8-0-android-aarch64/debug:

do we have debug aarch64 builds?

::: taskcluster/ci/test/test-sets.yml
@@ +369,5 @@
>      - mochitest-plain-headless
> +
> +android-hw-aarch64-unittests:
> +    - jittest
> +    - mochitest-media

do we want mochitest-media on aarch64?  I thought this was for 32 bit regular geckoview?

Comment 50

[Bob Clary [:bc] (inactive)]

(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #49)
> Comment on attachment 8986356 [details] [diff] [review]
> 03 add-android-hw-common-tests.patch
> 
> Review of attachment 8986356 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> some small questions, happy to r+ after answers are given in bug or irc
> 
> ::: taskcluster/ci/test/test-platforms.yml
> @@ +309,5 @@
> > +    build-platform: android-aarch64/opt
> > +    test-sets:
> > +        - android-hw-aarch64-unittests
> > +
> > +android-hw-p2-8-0-android-aarch64/debug:
> 
> do we have debug aarch64 builds?
> 

Not yet but there was no reason not to have them according to snorp so I filed Bug 1467868.

> ::: taskcluster/ci/test/test-sets.yml
> @@ +369,5 @@
> >      - mochitest-plain-headless
> > +
> > +android-hw-aarch64-unittests:
> > +    - jittest
> > +    - mochitest-media
> 
> do we want mochitest-media on aarch64?  I thought this was for 32 bit
> regular geckoview?

It seemed to me they really didn't care about 32bit and that 64bit was the future. I'll let cpeterson and snorp make the call. According to my estimates we can't support both unless we have more devices.

Comment 51

[Chris Peterson [:cpeterson]]

(In reply to Bob Clary [:bc:] from comment #50)
> > do we want mochitest-media on aarch64?  I thought this was for 32 bit
> > regular geckoview?
> 
> It seemed to me they really didn't care about 32bit and that 64bit was the
> future. I'll let cpeterson and snorp make the call. According to my
> estimates we can't support both unless we have more devices.

If we only have resources to test 32bit xor 64bit, then I think we should test 32bit. Like you said, 64bit-only is the future for Android, but we currently only ship 32bit builds of Fennec, Focus/Klar, and Firefox Reality. We don't know when we will ship 64bit builds.

Comment 52

[Bob Clary [:bc] (inactive)]

Chris: Then should I be testing Fennec and not geckoview-androidTest.apk (org.mozilla.geckoview.test.TestRunnerActivity) ?

Comment 53

[Chris Peterson [:cpeterson]]

(In reply to Bob Clary [:bc:] from comment #52)
> Chris: Then should I be testing Fennec and not geckoview-androidTest.apk
> (org.mozilla.geckoview.test.TestRunnerActivity) ?

When do we plan to retire our existing Android test devices? As long as we are still testing Fennec on those devices, I think we can focus on testing geckoview-androidTest.apk on Bitbar.

Comment 54

[Bob Clary [:bc] (inactive)]

I had thought the OG Autophone would be retired when we have perf tests running under Raptor. I had thought a couple of months but jmaher says more likely late Q3 or Q4.

The other option is not to run the jit tests until we have more devices since they take much longer than the mda tests.

I just realized that my time estimates are based only on aarch64 opt builds since that is all we have available at the moment. But once aarch64 debug builds become available, the current patches will automatically begin to run the jit tests on both opt and debug which will blow our time budget with the current number of devices. I could turn off aarch64 debug jit tests so we would keep under budget or just turn off the jit tests entirely for now.

Comment 55

[Joel Maher ( :jmaher ) (UTC -8)]

to add more context, we are running some perf tests on autophone, but not mochitest-media or jittests (jittests are new for autophone).

once we have mochitest-media/jittest running we will focus on perf:
raptor-speedometer (and if we are lucky compare against chrome)
raptor-webgl benchmark (unity?)
tbd-input latency
tbd-battery measurements

then porting over other perf scenarios like startup and pageload.

Comment 56

[Bob Clary [:bc] (inactive)]

Attachment: 03 add-android-hw-common-tests.patch

mda on arm7 opt,debug; jit arm7 opt

Comment 57

[Bob Clary [:bc] (inactive)]

Attachment: 05 test-platform-assignments.patch

removed geckoview-androidTest.apk from mda.

Comment 58

[Bob Clary [:bc] (inactive)]

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

Comment 59

[Chris Peterson [:cpeterson]]

(In reply to Bob Clary [:bc:] from comment #54)
> I had thought the OG Autophone would be retired when we have perf tests
> running under Raptor. I had thought a couple of months but jmaher says more
> likely late Q3 or Q4.

One concern: sphilp mentioned in email that the Raptor tests depend on a WebExtension and, at this time, GeckoView doesn't support WebExtensions (or any legacy Fennec add-ons). We might start look into WebExtension support in Q3 or Q4.


> The other option is not to run the jit tests until we have more devices
> since they take much longer than the mda tests.
> 
> I just realized that my time estimates are based only on aarch64 opt builds
> since that is all we have available at the moment. But once aarch64 debug
> builds become available, the current patches will automatically begin to run
> the jit tests on both opt and debug which will blow our time budget with the
> current number of devices. I could turn off aarch64 debug jit tests so we
> would keep under budget or just turn off the jit tests entirely for now.

I'd rather we run some jit tests than none, but anything aarch64 is a lower priority than 32bit ARM.

Bob, do you need any more information from me about GeckoView's test priorities for Bitbar?

Comment 60

[Bob Clary [:bc] (inactive)]

comment 56: jit arm7 opt
comment 58: coverage.

If that is good enough jit coverage, no. I 'm good.

Comment 61

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8986616 [details] [diff] [review]
03 add-android-hw-common-tests.patch

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

one remaining question.

::: taskcluster/ci/test/test-sets.yml
@@ +368,5 @@
>  mochitest-headless:
>      - mochitest-plain-headless
> +
> +android-hw-arm7-opt-unittests:
> +    - jittest

how do we ensure that jittest runs on aarch64 and not on 32 bit?

Comment 62

[Bob Clary [:bc] (inactive)]

As I believed I was instructed, I removed all aarch64 tests and configured it to run only on 32bit.

Please provide an exact list of tests and architectures you want to be tested.

Comment 63

[Joel Maher ( :jmaher ) (UTC -8)]

I thought we were doing:
* 32 bit fennec mochitest-media
* 64 bit jittest geckoview

did we have jittest before on autophone?  If so, then we should run 32 bit on fennec, if not- this is a new request specifically for arm64 and we should look into the future for 64 bit jittest and geckoview.

We have been shipping Fennec for many years and we should do what we can to run what we had in the past for coverage.  If we are standing up new coverage, then lets be forward looking.

Comment 64

[Bob Clary [:bc] (inactive)]

We didn't have jittest on Autophone.
jittest is a compiled test and doesn't use fennec or geckoview.

Please approve:

32bit opt/debug mochitest-media on fennec.
64bit opt only jittests.

Comment 65

[Joel Maher ( :jmaher ) (UTC -8)]

this sounds good based on requests and discussion in this bug to date.

Comment 66

[Bob Clary [:bc] (inactive)]

Attachment: add-android-hw-common-tests.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0781dba7cc391c8d8fd862158483ee5d1563b0b7&filter-tier=1&filter-tier=2&filter-tier=3&group_state=expanded

Comment 67

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8986735 [details] [diff] [review]
add-android-hw-common-tests.patch

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

looks good, thanks for making this happen!

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/baace08171da
Rename android emulator test platforms to android-em, r=gbrown
https://hg.mozilla.org/integration/mozilla-inbound/rev/13c99a5c12ff
Add mozharness script and config for android hardware tests, r=gbrown
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f5be9c3698e
Add android-hw unittests to taskcluster/ci/test/{test-platforms,test-sets}.yml, r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ddc31e62cce
Add job transforms for android hardware tests, r=jonasfj
https://hg.mozilla.org/integration/mozilla-inbound/rev/176d8c0698f3
Add android hardware test platforms for android-hw, r=gbrown,jmaher

Comment 69

[Bob Clary [:bc] (inactive)]

The mda tests are running on android-hw-p2-8-0-arm7-api-16 but I am missing the jit tests on android-hw-p2-8-0-android-aarch64 for some reason.

Comment 70

[Bob Clary [:bc] (inactive)]

I think the jit tests are missing just because the patches (so far) haven't affected the skip-unless-schedules jit. We'll see as we get more pushes.

Comment 71

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/baace08171da
https://hg.mozilla.org/mozilla-central/rev/13c99a5c12ff
https://hg.mozilla.org/mozilla-central/rev/7f5be9c3698e
https://hg.mozilla.org/mozilla-central/rev/8ddc31e62cce
https://hg.mozilla.org/mozilla-central/rev/176d8c0698f3