1495430 - mozharness support for Android web-platform-tests

Status: RESOLVED FIXED

(Testing :: General, enhancement, P1)

Description

[Geoff Brown [:gbrown]]

web-platform-tests run from their own web_platform_tests.py mozharness script currently, which has no notion of Android device setup, etc. We can leverage recent work on AndroidMixin to make web_platform_tests.py Android-aware.

Comment 1

[Geoff Brown [:gbrown]]

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3revision=d453458f34cf76238702b87de8427b707467eeb3

Comment 2

[James Graham [:jgraham]]

This is exciting! I think the bug in your try push may be fixed by s/"ca_certificate_path": kwargs["ssl_env"].ca_cert_path()/ "ca_certificate_path": config.ssl_config["ca_cert_path"]/ in fennec.py.

Comment 3

[Geoff Brown [:gbrown]]

Thanks :jgraham! That gets us just a little bit further. I'll keep debugging...

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=576e0327b9c77c9f0f42569d576aeecb32b5f631

Comment 4

[James Graham [:jgraham]]

Looks like missing certutil, perhaps?

Comment 5

[Geoff Brown [:gbrown]]

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=61250717c699ba723e26a11931d1e12105deec64

Next challenge is getting the adb path to the harness...

Comment 6

[Geoff Brown [:gbrown]]

Attachment: 1. fix ca_certificate_path in fennec runner

As you suggested in comment 2 - works great for me.

Comment 7

[Geoff Brown [:gbrown]]

At least some tests ran here!

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&selectedJob=202937192&revision=d5995d346858a3bad3c7ae34e0a681b04d544e75

Needs lots of cleanup, etc.

Comment 8

[Geoff Brown [:gbrown]]

Attachment: 2. move some more code into AndroidMixin

I noticed we can consolidate _query_package_name() between emulator/hardware and also use download_hostutils() from AndroidMixin. (wpt will need to do the same.)

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=af834e8dae50c5274923d283dd609ef7d1c03afd

Comment 9

[Bob Clary [:bc] (inactive)]

Comment on attachment 9013847 [details] [diff] [review]
2. move some more code into AndroidMixin

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

lgtm. Moar Consolidations! r+

Comment 10

[Geoff Brown [:gbrown]]

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=f7fbfc2ff09c86edf491401921c2af77d9de4bdf

I notice that the harness reports logcat, but it seems to repeat lines sometimes:

[task 2018-10-02T23:27:20.661Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.240 I/Gecko   ( 8090): Attempting load of libEGL.so
[task 2018-10-02T23:27:20.666Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.240 I/Gecko   ( 8090): Attempting load of libEGL.so
[task 2018-10-02T23:27:20.667Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.220 I/Gecko   ( 8090): -*- nsDNSServiceDiscovery.js : nsDNSServiceDiscovery
[task 2018-10-02T23:27:20.673Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.220 I/Gecko   ( 8090): -*- nsDNSServiceDiscovery.js : nsDNSServiceDiscovery
[task 2018-10-02T23:27:20.673Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.240 I/Gecko   ( 8090): Attempting load of libEGL.so
[task 2018-10-02T23:27:20.676Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.240 I/Gecko   ( 8090): Attempting load of libEGL.so
[task 2018-10-02T23:27:20.676Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.220 I/Gecko   ( 8090): -*- nsDNSServiceDiscovery.js : nsDNSServiceDiscovery
[task 2018-10-02T23:27:20.680Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.220 I/Gecko   ( 8090): -*- nsDNSServiceDiscovery.js : nsDNSServiceDiscovery
[task 2018-10-02T23:27:20.681Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.240 I/Gecko   ( 8090): Attempting load of libEGL.so
[task 2018-10-02T23:27:20.683Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.240 I/Gecko   ( 8090): Attempting load of libEGL.so
[task 2018-10-02T23:27:20.684Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.240 I/Gecko   ( 8090): Attempting load of libEGL.so
[task 2018-10-02T23:27:20.685Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.220 I/Gecko   ( 8090): -*- nsDNSServiceDiscovery.js : nsDNSServiceDiscovery
[task 2018-10-02T23:27:20.686Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.220 I/Gecko   ( 8090): -*- nsDNSServiceDiscovery.js : nsDNSServiceDiscovery
[task 2018-10-02T23:27:20.687Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.220 I/Gecko   ( 8090): -*- nsDNSServiceDiscovery.js : nsDNSServiceDiscovery
[task 2018-10-02T23:27:20.687Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.240 I/Gecko   ( 8090): Attempting load of libEGL.so
[task 2018-10-02T23:27:20.687Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.240 I/Gecko   ( 8090): Attempting load of libEGL.so
[task 2018-10-02T23:27:20.691Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.220 I/Gecko   ( 8090): -*- nsDNSServiceDiscovery.js : nsDNSServiceDiscovery
[task 2018-10-02T23:27:20.692Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.220 I/Gecko   ( 8090): -*- nsDNSServiceDiscovery.js : nsDNSServiceDiscovery
[task 2018-10-02T23:27:20.692Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.240 I/Gecko   ( 8090): Attempting load of libEGL.so
[task 2018-10-02T23:27:20.692Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.240 I/Gecko   ( 8090): Attempting load of libEGL.so
[task 2018-10-02T23:27:20.696Z] 23:27:20     INFO - STDOUT: 10-03 00:27:11.240 I/Gecko   ( 8090): Attempting load of libEGL.so

(notice the repeating logcat timestamps)

Comment 11

[Geoff Brown [:gbrown]]

(In reply to Geoff Brown [:gbrown] from comment #10)
> I notice that the harness reports logcat, but it seems to repeat lines

I tried suppressing logcat with --no-capture-stdio, but that just changed the formatting, causing "output is not a valid structured log message":

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&selectedJob=203005066&revision=de800a25b6faa4de68646a6ebc50240274862192

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4b8f37b625d
Fix ca_certificate_path in wpt fennec runner; r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/b41f84a1a1e2
Move a little more code into AndroidMixin; r=bc

Comment 13

[James Graham [:jgraham]]

Yeah, don't pass --no-capture-stdio; the wpt mozharness script is set up to fail if we write random stuff to stdout. That said I have literally no idea what's going on with logcat; if you can reproduce the error locally it may be easier to debug. I suspect that two different things are setting up loggers for some reason?

Comment 14

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13346 for changes under testing/web-platform/tests

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a4b8f37b625d
https://hg.mozilla.org/mozilla-central/rev/b41f84a1a1e2

Comment 16

[Geoff Brown [:gbrown]]

I didn't get very far today, except that I found disabling logcat seemed to improve performance and test results considerably (I put back --no-capture-stdio and removed the logcat code):

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=72b5bc777be58f0220388201795cce58a142a5ae

Need to test that more...

Comment 17

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/13346
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/PvEoLyyoT1ynC8S4VG9onw)

Comment 18

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 19

[Geoff Brown [:gbrown]]

I tried running wpt-reftests for the first time today and noticed they fail immediately:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&selectedJob=203511685&revision=1d9cd35290256ad57fa963f463beb2904d67fc23

  Unsupported test type reftest for product fennec

:kwierso: Do you know if that is expected?

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I think that's expected, since wpt itself isn't really supported on fennec yet.

Comment 21

[Geoff Brown [:gbrown]]

Attachment: 3. move emulator management into AndroidMixin

I was able to simplify android_hardware_unittest.py a little more too.

In addition to enabling emulator management from other scripts, like wpt, this sets us up for the possibility of re-uniting android_emulator_unittest.py and android_hardware_unittest.py...or even perhaps consolidating it all into desktop_unittest.py!!

Comment 22

[Geoff Brown [:gbrown]]

Attachment: 4. use AndroidMixin in web_platform_tests.py

Doesn't break desktop wpt:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=9077d22056f87365f35e62532d71ac35fa7b19d7

Enables Android wpt (with taskcluster changes -- not yet proposed):

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=2551825129f2032b612a96993054f3832ddc7bf6

(Android wpt still runs with many errors -- should be able to clear these up with a manifest update).

Comment 23

[Geoff Brown [:gbrown]]

I should also demonstrate that existing android-em and android-hw tests are not broken:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=9c9ee1d2e78504146ae87cf3c104f07ba4eb7f4a

Comment 24

[Geoff Brown [:gbrown]]

(In reply to Geoff Brown [:gbrown] from comment #22)
> Enables Android wpt (with taskcluster changes -- not yet proposed):
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&tier=1%2C2%2C3&revision=2551825129f2032b612a96993054f3832ddc7bf
> 6

Sorry, that failed because it was running in the geckoview TestRunnerActivity, not yet supported.

Here is a try run against Firefox for Android:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=93321df918409c7686c02b8446bf08d068b91df5

Comment 25

[James Graham [:jgraham]]

Wow!

Comment 26

[Bob Clary [:bc] (inactive)]

Comment on attachment 9015646 [details] [diff] [review]
3. move emulator management into AndroidMixin

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

r+. Looks good with comments you can ignore if you like.

::: testing/mozharness/mozharness/mozilla/testing/android.py
@@ +74,5 @@
> +            c = self.config
> +            installer_url = c.get('installer_url', None)
> +            return self.device_serial is not None or self.is_emulator or \
> +                (installer_url is not None and installer_url.endswith(".apk"))
> +        except AttributeError:

I am a little concerned that we might be hiding important information by not logging this somehow... Not a blocker though.

@@ +82,5 @@
> +    def is_emulator(self):
> +        try:
> +            c = self.config
> +            return True if c.get('emulator_manifest') else False
> +        except AttributeError:

ditto

@@ +400,5 @@
> +    def kill_processes(self, process_name):
> +        self.info("Killing every process called %s" % process_name)
> +        out = subprocess.check_output(['ps', '-A'])
> +        for line in out.splitlines():
> +            if process_name in line:

I know this is the same as before but I was wondering if we should handle the case where process_name is a substring of another process the way we egao did in Bug 1190701 ?

@@ +464,5 @@
> +                                       output_dir=dirs['abs_work_dir'],
> +                                       cache=cache):
> +                    self.fatal("Unable to download emulator via tooltool!")
> +        else:
> +            self.warning("Cannot get emulator: configure emulator_url or emulator_manifest")

Should we make this fatal?

Comment 27

[Bob Clary [:bc] (inactive)]

Comment on attachment 9015663 [details] [diff] [review]
4. use AndroidMixin in web_platform_tests.py

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

I get failures to detect the device when trying to run locally via ./mach wpt with and without --device-serial...

 1:25.05 WARNING Failure during init Traceback (most recent call last):
  File "/home/bclary/mozilla/builds/inbound-taskcluster/mozilla/testing/web-platform/tests/tools/wptrunner/wptrunner/testrunner.py", line 199, in init
    self.browser.start(group_metadata=group_metadata, **self.browser_settings)
  File "/home/bclary/mozilla/builds/inbound-taskcluster/mozilla/testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/fennec.py", line 217, in start
    self.runner.device.connect()
  File "/home/bclary/mozilla/builds/inbound-taskcluster/mozilla/testing/mozbase/mozrunner/mozrunner/devices/emulator.py", line 157, in connect
    super(BaseEmulator, self).connect()
  File "/home/bclary/mozilla/builds/inbound-taskcluster/mozilla/testing/mozbase/mozrunner/mozrunner/devices/base.py", line 138, in connect
    raise IOError("No devices connected. Ensure the device is on and "
IOError: No devices connected. Ensure the device is on and remote debugging via adb is enabled in the settings.

Is that something we want to support in this patch?

Comment 28

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #26)
> ::: testing/mozharness/mozharness/mozilla/testing/android.py
> @@ +74,5 @@
> > +            c = self.config
> > +            installer_url = c.get('installer_url', None)
> > +            return self.device_serial is not None or self.is_emulator or \
> > +                (installer_url is not None and installer_url.endswith(".apk"))
> > +        except AttributeError:
> 
> I am a little concerned that we might be hiding important information by not
> logging this somehow... Not a blocker though.

All of the @property's in AndroidMixin have a similar issue. Recall that they are all called at init time by mozharness, before self.config is available. I think we want to fail silently at that time. I hope we don't fail silently later -- not sure what else to do.

> @@ +400,5 @@
> > +    def kill_processes(self, process_name):
> > +        self.info("Killing every process called %s" % process_name)
> > +        out = subprocess.check_output(['ps', '-A'])
> > +        for line in out.splitlines():
> > +            if process_name in line:
> 
> I know this is the same as before but I was wondering if we should handle
> the case where process_name is a substring of another process the way we
> egao did in Bug 1190701 ?

Yes - updated.

> @@ +464,5 @@
> > +                                       output_dir=dirs['abs_work_dir'],
> > +                                       cache=cache):
> > +                    self.fatal("Unable to download emulator via tooltool!")
> > +        else:
> > +            self.warning("Cannot get emulator: configure emulator_url or emulator_manifest")
> 
> Should we make this fatal?

Yes - updated.

Comment 29

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #27)
> Is that something we want to support in this patch?

I want to make sure I don't break 'mach wpt' with my patches, but I understand that's not the case here. Let's handle mach issues in bug 1496627.

Comment 30

[Geoff Brown [:gbrown]]

Attachment: 3. move emulator management into AndroidMixin

Comment 31

[Geoff Brown [:gbrown]]

Attachment: 3. move emulator management into AndroidMixin

Comment 32

[Geoff Brown [:gbrown]]

Attachment: 3. move emulator management into AndroidMixin

As discussed on irc, this restores the original kill_processes() implementation, since we feel that further reliance on ps column output is fragile. One day we might revisit with pkill instead, but let's go with this for now.

Comment 33

[Bob Clary [:bc] (inactive)]

Comment on attachment 9016122 [details] [diff] [review]
3. move emulator management into AndroidMixin

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

r+ \0/

Comment 34

[Bob Clary [:bc] (inactive)]

Comment on attachment 9015663 [details] [diff] [review]
4. use AndroidMixin in web_platform_tests.py

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

looks ok to me. r+

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/912784581287
Move most mozharness android emulator support into AndroidMixin; r=bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/5095c0fdd000
Use AndroidMixin in web_platform_tests.py mozharness script; r=bc,automatedtester

Comment 36

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/912784581287
https://hg.mozilla.org/mozilla-central/rev/5095c0fdd000

Comment 37

[Geoff Brown [:gbrown]]

Attachment: 5. add taskcluster config for android wpt, disabled

Minor change to tc config to make this easy to enable.

Comment 38

[Geoff Brown [:gbrown]]

Remaining work is:
 - running in TestRunnerActivity (bug 1496773)
 - greening up tests (no point in looking at that until running in TestRunnerActivity)

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/961ceb4f759b
Add taskcluster config for android wpt, disabled for now; r=me,a=test-only

Comment 40

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/961ceb4f759b