mozharness support for Android web-platform-tests

RESOLVED FIXED in Firefox 64

Status

enhancement
P1
normal
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: gbrown, Assigned: gbrown)

Tracking

(Blocks 1 bug)

Version 3
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(5 attachments, 3 obsolete attachments)

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.
Priority: -- → P1
See Also: → 1323620
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.
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
Looks like missing certutil, perhaps?
Blocks: 1490969
Depends on: 1495863
As you suggested in comment 2 - works great for me.
Attachment #9013823 - Flags: review?(james)
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
Attachment #9013847 - Flags: review?(bob)
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+
Attachment #9013847 - Flags: review?(bob) → review+
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)
Attachment #9013823 - Flags: review?(james) → review+
(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
Keywords: leave-open
Pushed by gbrown@mozilla.com:
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
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?
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13346 for changes under testing/web-platform/tests
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...
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)
Upstream PR merged
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?
Flags: needinfo?(wkocher)
I think that's expected, since wpt itself isn't really supported on fennec yet.
Flags: needinfo?(wkocher)
Depends on: 1496773
Depends on: 1497566
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!!
Attachment #9015646 - Flags: review?(bob)
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).
Attachment #9015663 - Flags: review?(dburns)
Attachment #9015663 - Flags: review?(bob)
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
(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
Wow!
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?
Attachment #9015646 - Flags: review?(bob) → review+
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?
(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.
(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.
Attachment #9015646 - Attachment is obsolete: true
Attachment #9016111 - Flags: review?(bob)
Attachment #9016111 - Attachment is obsolete: true
Attachment #9016111 - Flags: review?(bob)
Attachment #9016113 - Flags: review?(bob)
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.
Attachment #9016113 - Attachment is obsolete: true
Attachment #9016113 - Flags: review?(bob)
Attachment #9016122 - Flags: review?(bob)
Attachment #9015663 - Flags: review?(dburns) → review+
Comment on attachment 9016122 [details] [diff] [review]
3. move emulator management into AndroidMixin

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

r+ \0/
Attachment #9016122 - Flags: review?(bob) → review+
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+
Attachment #9015663 - Flags: review?(bob) → review+
Pushed by gbrown@mozilla.com:
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
Minor change to tc config to make this easy to enable.
Attachment #9016473 - Flags: review+
Keywords: leave-open
Remaining work is:
 - running in TestRunnerActivity (bug 1496773)
 - greening up tests (no point in looking at that until running in TestRunnerActivity)
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/961ceb4f759b
Add taskcluster config for android wpt, disabled for now; r=me,a=test-only
https://hg.mozilla.org/mozilla-central/rev/961ceb4f759b
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Blocks: 1425322
Blocks: 1510536
You need to log in before you can comment on or make changes to this bug.