Add support for mochitest-chrome to Android

RESOLVED FIXED

Status

Testing
Mochitest Chrome
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: MattN, Assigned: gbrown)

Tracking

(Blocks: 1 bug, {leave-open})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [LOE:M])

Attachments

(5 attachments, 2 obsolete attachments)

+++ This bug was initially created as a clone of Bug #797164 for B2G +++

There are many mochitest-chrome tests that are independent of UI but need extra privileges to reach into privileged objects as part of the initialization or checking results. SpecialPowers can make simple cases work in mochitest-plain but more complicated setups or ones using Promise.jsm or Tasks require a lot of changes to make them work with it. It may be useful to get test coverage on Android if we think that there are conditions where the test could fail on Android while passing on desktop (where they currently run).

Comment 1

3 years ago
Note that we don't need to run the entire suite at first - we mostly care about the tests in bug 1021060. If other mochitests fail, we can disable them on Android and convert them later.
one hiccup here is we disabled the mochitest .jar creation in bug 939719 since that was causing issues and we had not used it for a few years.

I am not sure if we need it as technologies have changed over the years, but that shouldn't be too hard to turn on again if needed- we could use it for remote testing only.
(Assignee)

Updated

3 years ago
Assignee: nobody → gbrown
(Assignee)

Comment 3

3 years ago
I pushed a try job that enabled --chrome and tried to run the toolkit/components/formautofill/test/chrome tests on linux, android, and b2g. The android and b2g tests hung and timed out.

I tried running the same locally. On desktop, the tests run and pass. On Android, redirect.html is loaded on the device but does not redirect. The test hangs and times out. I see no errors in the test log or the logcat.

With additional logging on Android, I can verify that the chromeEvent message handler is installed, but never invoked. I don't know why. http://hg.mozilla.org/mozilla-central/annotate/f2d7d694aae5/testing/mochitest/browser-test.js#l60
(Assignee)

Comment 4

3 years ago
If I manually load chrome://mochikit/content/harness.xul while runtestsremote.py is active, we are redirected to www.{}.com. If I go back from that page, I get back to harness.xul and the chrome test runner page displays with:
"""
Currently executing: chrome://mochitests/content/chrome/toolkit/components/formautofill/test/chrome/test_infrastructure.html

File not found: Firefox can't find the the file at /home/gbrown/objdirs/droid/_tests/testing/mochitest/chrome/toolkit/components/formautofill/test/chrome/test_infrastructure.html
""" 
So there are some path issues to sort out here, as well as the minor mystery of why the test harness is not loading harness.xul. And I wonder where "www.{}.com" comes from. On the positive side, we can access mochikit from a chrome: url, and xul seems okay.
I bet chrome://mochitests/content/chrome maps to /home/gbrown/objdirs/droid/_tests/testing/mochitest/chrome in the chrome.manifest file.  Previously we packaged all the tests up and downloaded them to the device in the profile or a tmp directory, then ran the tests from a .jar.  In fact I just removed that code a week or two ago since it hadn't been used in 4+ years.
(Assignee)

Comment 6

3 years ago
Thanks Joel. Actually it looks like the mapping happens in tests.manifest, created at http://hg.mozilla.org/mozilla-central/annotate/305d24174ace/testing/mochitest/runtests.py#l785. If I push test files to device and manipulate the path in tests.manifest to match, a manual load of harness.xul finds the first test file and attempts to run it.
(Assignee)

Comment 7

3 years ago
(In reply to Geoff Brown [:gbrown] from comment #3)
> With additional logging on Android, I can verify that the chromeEvent
> message handler is installed, but never invoked. I don't know why.
> http://hg.mozilla.org/mozilla-central/annotate/f2d7d694aae5/testing/
> mochitest/browser-test.js#l60

I understand what is happening now. 

redirect.html is loaded and sends "contentEvent" in redirect.html's onLoad handler, http://hg.mozilla.org/mozilla-central/annotate/aa72ddfe9f93/testing/mochitest/redirect.html#l10. 

The listener for contentEvent is installed in browser-test.js, but only after MozAfterPaint is received: http://hg.mozilla.org/mozilla-central/annotate/084441e904d1/testing/mochitest/browser-test.js#l34, 
http://hg.mozilla.org/mozilla-central/annotate/084441e904d1/testing/mochitest/browser-test.js#l72.

On desktop, the timing happens to work out. On Android, the listener is added about 500 ms after contentEvent is sent, so the listener is never invoked. If I remove the wait for MozAfterPaint (add the contentEvent listener at browser-test.js load time) contentEvent is received on both desktop and Android, in my local tests.
(Assignee)

Comment 8

3 years ago
(In reply to Geoff Brown [:gbrown] from comment #7)
> If I remove the wait for MozAfterPaint (add the contentEvent
> listener at browser-test.js load time) contentEvent is received on both
> desktop and Android, in my local tests.

This solution appears to hold up on try as well: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a6571bdfbea5
(Assignee)

Comment 9

3 years ago
Created attachment 8525567 [details] [diff] [review]
(1) delay redirect until after MozAfterPaint

This resolves the problem I described in Comment 7. It delays the redirect a bit, until after MozAfterPaint. In local tests, it works fine on Android, consistently loading the harness and running tests (which still fail, for other reasons -- more patches on the way). 

I prefer this approach to installing the listener earlier (onLoad): I don't know of a reason why the listener needs to wait for MozAfterPaint, but I assume the code is there for some reason, and I like the idea of not trying to start testing until initialization is (more) complete.

This change seems to cause no harm to existing tests/platforms: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e5ee06211c30
Attachment #8525567 - Flags: review?(jmaher)
(Assignee)

Updated

3 years ago
Keywords: leave-open
Comment on attachment 8525567 [details] [diff] [review]
(1) delay redirect until after MozAfterPaint

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

nice and simple.
Attachment #8525567 - Flags: review?(jmaher) → review+
(Assignee)

Comment 11

3 years ago
(1) https://hg.mozilla.org/integration/mozilla-inbound/rev/6d8adc89991c
https://hg.mozilla.org/mozilla-central/rev/6d8adc89991c
(Assignee)

Comment 13

3 years ago
There is a problem with logging, because of http://hg.mozilla.org/mozilla-central/annotate/b8240bb9ae4f/testing/mochitest/runtests.py#l2102:

  d = dict((k, v) for k, v in options.__dict__.iteritems() if not k.startswith('log'))

This excludes the logFile parameter from testConfig.js, so the chrome tests do not log to the logFile file on the device. The python harness tries to pull that file, never finds or displays any test results, and soon hits the no-output timeout, ending the test job.

The 'log' exception was discussed at https://bugzilla.mozilla.org/show_bug.cgi?id=1056329#c3.
(Assignee)

Comment 14

3 years ago
If I manually push test files to the device and hack around the problem in Comment 13, I can run mochitest-chrome locally against an Android device via adb or sut. (Less than 5% of tests fail -- some minor manifest updates will be necessary.)
(Assignee)

Updated

3 years ago
Blocks: 797164
See Also: bug 797164
(Assignee)

Comment 15

3 years ago
Created attachment 8528016 [details] [diff] [review]
(2) Do not filter log* parameters from testConfig.js

As I noted earlier, when I try to run chrome tests for Android, logging fails because the logFile parameter is not passed in testConfig.js. 

Bug 1056329 introduced the code to filter out log* parameters from those passed in testConfig.js. In comment 3, :akachkach said "I added this because otherwise chrome tests crash: as I mentioned in the description of the bug: it will try to serialize file objects to JSON, and fail." I don't understand this -- in all my tests, log* parameters are strings.

This patch backs out that one line from bug 1056329, which fixes my logging problems for Android mochitest-chrome. It does not appear to break anything on try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5ad3b59baedf
Attachment #8528016 - Flags: review?(cmanchester)
Attachment #8528016 - Flags: feedback?(ahmed.kachkach)
(In reply to Geoff Brown [:gbrown] from comment #15)
> Created attachment 8528016 [details] [diff] [review]
> (2) Do not filter log* parameters from testConfig.js
> 
> As I noted earlier, when I try to run chrome tests for Android, logging
> fails because the logFile parameter is not passed in testConfig.js. 
> 
> Bug 1056329 introduced the code to filter out log* parameters from those
> passed in testConfig.js. In comment 3, :akachkach said "I added this because
> otherwise chrome tests crash: as I mentioned in the description of the bug:
> it will try to serialize file objects to JSON, and fail." I don't understand
> this -- in all my tests, log* parameters are strings.
> 
> This patch backs out that one line from bug 1056329, which fixes my logging
> problems for Android mochitest-chrome. It does not appear to break anything
> on try:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5ad3b59baedf

I think I understand what Ahmed is getting at... if you pass '--log-raw logf.json' or '--log-mach -' to the test process when running locally, does everything work as expected?
(Assignee)

Comment 17

3 years ago
(In reply to Chris Manchester [:chmanchester] from comment #16)
> I think I understand what Ahmed is getting at... if you pass '--log-raw
> logf.json' or '--log-mach -' to the test process when running locally, does
> everything work as expected?

Yes -- those work just fine.
(In reply to Geoff Brown [:gbrown] from comment #17)
> (In reply to Chris Manchester [:chmanchester] from comment #16)
> > I think I understand what Ahmed is getting at... if you pass '--log-raw
> > logf.json' or '--log-mach -' to the test process when running locally, does
> > everything work as expected?
> 
> Yes -- those work just fine.

Maybe this is just an issue with mach. When I run |mach mochitest-chrome --log-mach -| I get a TypeError consistent with what Ahmed said. Should work with:

dict((k, v) for k, v in options.__dict__.items() if
     (not k.startswith('log_') or
      not any([k.endswith(fmt) for fmt in commandline.log_formatters.keys()])))
(Assignee)

Comment 19

3 years ago
Created attachment 8528097 [details] [diff] [review]
(2) do not filter logFile parameter from testConfig.js

Thanks Chris. I see the TypeError now, with that mach command (curiously, my patch works fine if I invoke runtests.py with the same arguments).

Your suggestion works fine -- here it is!
Attachment #8528016 - Attachment is obsolete: true
Attachment #8528016 - Flags: review?(cmanchester)
Attachment #8528016 - Flags: feedback?(ahmed.kachkach)
Attachment #8528097 - Flags: review?(cmanchester)
Eh, I knew we'd need that back at one point ;)
So the problem was that you could have an object in your log options (a formatter object I believe), it wasn't all strings.

Chris's solution should work fine! (but if we ever have other objects getting in the command line args in the future, that could be problematic)
(Assignee)

Comment 21

3 years ago
Created attachment 8528440 [details] [diff] [review]
(3) push chrome test files to device

This patch allows runtestsremote.py to over-ride the chrome test directory written to tests.manifest -- it allows remote tests to specify a different physical location for chrome test files. runtestsremote.py then defines that directory as <test-root>/chrome (/mnt/sdcard/tests/chrome), which it creates, populates from the local chrome directory, and later deletes as part of normal clean-up.

I am disappointed by the number of intermittent failures in this try run, but I think I was just unlucky and this still demonstrates no harm to existing test jobs: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=742344b720d5

In https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e14d49b06fa9, I force mochitest-1 to impersonate mochitest-chrome and demonstrate chrome tests running on Android:

08:11:41     INFO -  pushing /builds/slave/test/build/tests/mochitest/chrome to /mnt/sdcard/tests/chrome on device...
...
08:11:41     INFO -  runtests.py | Running tests: start.
08:11:41     INFO -  
08:11:41     INFO -  INFO | automation.py | Application pid: 1439
08:11:41     INFO -  Running tests 1-59/947
08:11:41     INFO -  0 INFO SimpleTest START
08:11:41     INFO -  1 INFO TEST-START | chrome://mochitests/content/chrome/caps/tests/mochitest/test_bug995943.xul
08:11:41     INFO -  2 INFO Invoking go for window with id: 14
08:11:41     INFO -  3 INFO TEST-PASS | chrome://mochitests/content/chrome/caps/tests/mochitest/test_bug995943.xul | Should only call go once! 

There are test failures and time-outs, but the majority of tests pass.
Attachment #8528440 - Flags: review?(jmaher)
Comment on attachment 8528440 [details] [diff] [review]
(3) push chrome test files to device

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

overall this looks good!  I am glad this is working out pretty well!
Attachment #8528440 - Flags: review?(jmaher) → review+
Comment on attachment 8528097 [details] [diff] [review]
(2) do not filter logFile parameter from testConfig.js

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

::: testing/mochitest/runtests.py
@@ +2100,5 @@
>        options.hideResultsTable = True
>  
> +    d = dict((k, v) for k, v in options.__dict__.items() if
> +     (not k.startswith('log_') or
> +      not any([k.endswith(fmt) for fmt in commandline.log_formatters.keys()])))

Nit: I think the second and third lines should be indented further.
Attachment #8528097 - Flags: review?(cmanchester) → review+
(Assignee)

Comment 24

3 years ago
With nit addressed --

(2) https://hg.mozilla.org/integration/mozilla-inbound/rev/bde6ca1bff09
(3) https://hg.mozilla.org/integration/mozilla-inbound/rev/272443dba538
(Assignee)

Comment 25

3 years ago
I don't understand how, but this seemed to cause infinite retries on Android 4.0 robocop, so I backed it out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8663be2ce7cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c081c7aa000
(Assignee)

Comment 26

3 years ago
This pushes the chrome files for every mochitest job - mochitest-plain, robocop, anything. That may be enough extra time to hit WATCHDOG KILLING.... At any rate, the chrome files should only be pushed for chrome jobs.
(Assignee)

Comment 27

2 years ago
Simply avoiding the push for non-chrome jobs fixes the robocop retry problem:

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

(2) https://hg.mozilla.org/integration/mozilla-inbound/rev/e8f0e9303339
(3) https://hg.mozilla.org/integration/mozilla-inbound/rev/0e0d89a0d414
https://hg.mozilla.org/mozilla-central/rev/e8f0e9303339
https://hg.mozilla.org/mozilla-central/rev/0e0d89a0d414
Hey Geoff, what's the status of this? I don't see it on TreeHerder and there is leave-open here.
Status: NEW → ASSIGNED
Flags: needinfo?(gbrown)
(Assignee)

Comment 30

2 years ago
Sorry for the confusion. I have been meaning to get back to this but haven't been finding time.

The landed patches allow mochitest-chrome tests to be run on android directly from runtestsremote.py, but there remain serious limitations:
 - no make target or mach command for mochitest-chrome on android -- bug 1120727, also long-neglected;
 - tests have not been "greened" -- need to sort out which tests pass reliably on android and update test manifests accordingly before attempting to run in continuous integration;
 - no android mochitest-chrome jobs running anywhere on treeherder, no try support, etc.

I have plans to sort out the mach command within the next couple of weeks, but don't have specific plans for getting these running on treeherder. Is there still interest in getting Android mochitest-chrome on treeherder? Is there a sub-set of tests that's particularly important?
Flags: needinfo?(gbrown)
(Assignee)

Comment 31

2 years ago
Created attachment 8615675 [details] [diff] [review]
(4) work around options.dm also in makeTestConfig

The landed patches have been working fine on Android, but now my attempts to run mochitest-chrome tests on Android with the mach command always fail with:

 0:22.10 LOG: MainThread ERROR Automation Error: Exception caught while running tests
Traceback (most recent call last):
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/runtestsremote.py", line 720, in run_test_harness
    retVal = mochitest.runTests(options)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/runtests.py", line 2111, in runTests
    return self.runMochitests(options, testsToRun, onLaunch)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/runtests.py", line 2048, in runMochitests
    result = self.doTests(options, onLaunch, testsToRun)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/runtests.py", line 2231, in doTests
    self.buildURLOptions(options, self.browserEnv)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/runtestsremote.py", line 221, in buildURLOptions
    retVal = Mochitest.buildURLOptions(self, options, env)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/runtests.py", line 601, in buildURLOptions
    self.makeTestConfig(options)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/runtests.py", line 2557, in makeTestConfig
    content = json.dumps(d)
  File "/usr/lib/python2.7/json/__init__.py", line 243, in dumps
    return _default_encoder.encode(obj)
  File "/usr/lib/python2.7/json/encoder.py", line 207, in encode
    chunks = self.iterencode(o, _one_shot=True)
  File "/usr/lib/python2.7/json/encoder.py", line 270, in iterencode
    return _iterencode(o, 0)
  File "/usr/lib/python2.7/json/encoder.py", line 184, in default
    raise TypeError(repr(o) + " is not JSON serializable")
TypeError: <droid.DroidADB object at 0x287abd0> is not JSON serializable

...very similar to the issue we discussed in Comment 18, etc. Now the offending option is options.dm. Here's a simple patch to avoid that case. I would prefer a more robust solution that guards against new non-JSON-serializable options, but cannot think of one.
Attachment #8615675 - Flags: review?(cmanchester)
Comment on attachment 8615675 [details] [diff] [review]
(4) work around options.dm also in makeTestConfig

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

::: testing/mochitest/runtests.py
@@ +2554,5 @@
> +        # strip certain unnecessary items to avoid serialization errors in json.dumps()
> +        d = dict((k, v) for k, v in options.__dict__.items() if
> +            ((not k.startswith('log_') or not
> +              any([k.endswith(fmt) for fmt in commandline.log_formatters.keys()]))) and
> +             (not k=='dm'))

What about a whitelist of things actually read from testConfig.js? Searching a bit, it looks like we use a few of the values (manifestFile, testRoot, and maybe a few others.
(Assignee)

Comment 33

2 years ago
Created attachment 8616211 [details] [diff] [review]
(4b) work around options.dm also in makeTestConfig

Thanks Chris, that's a good point. I want to me a little more permissive though, so here's a derivative idea: Allow options with string or numeric types. That should cover the known required parameters and leaves room for plenty more.
Attachment #8615675 - Attachment is obsolete: true
Attachment #8615675 - Flags: review?(cmanchester)
Attachment #8616211 - Flags: review?(cmanchester)
Comment on attachment 8616211 [details] [diff] [review]
(4b) work around options.dm also in makeTestConfig

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

Even better, thanks!

::: testing/mochitest/runtests.py
@@ +2552,5 @@
>                  "MOZ_HIDE_RESULTS_TABLE"] == "1":
>              options.hideResultsTable = True
>  
> +        # strip certain unnecessary items to avoid serialization errors in json.dumps()
> +        d = dict((k, v) for k, v in options.__dict__.items() if v == None or 

nit: I think we usually use 'is None'
Attachment #8616211 - Flags: review?(cmanchester) → review+

Comment 35

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d0e1a4cdc46
https://hg.mozilla.org/mozilla-central/rev/8d0e1a4cdc46
Two comments in support of this work:

* I would like to enable a few browser tests on Android as part of Bug 1174458.  That ticket tracks moving some browser/ stuff into toolkit/; the relevant tests are not particularly tied to desktop and will likely work on Android with some small tweaks.

* It is possible that most Robocop Javascript tests could migrate to browser.  gbrown surely understands this space better than me, but I think we win when we have less compiled Java tests and more standardization across platforms (and especially tools!).
(Assignee)

Updated

2 years ago
Flags: needinfo?(gbrown)
(Assignee)

Comment 38

2 years ago
Created attachment 8632083 [details] [diff] [review]
(5) update chrome.ini files for android

Here's a quick-and-dirty manifest update for mochitest-chrome on Android.

Inspired by b2g, I took a zero tolerance approach and added a default skip-if to the manifest whenever any single test failed. (There are many more tests that pass on Android, but I'll leave it to others to sort out which ones and/or investigate failures.)

This gives me a clean mochitest-chrome run locally and on try, with over 1000 test passes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53a454481dfe (note that M-1 here is actually mochitest-chrome).
Flags: needinfo?(gbrown)
Attachment #8632083 - Flags: review?(jgriffin)
Comment on attachment 8632083 [details] [diff] [review]
(5) update chrome.ini files for android

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

Nice!
Attachment #8632083 - Flags: review?(jgriffin) → review+

Comment 40

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f07416ab9bb
(Assignee)

Updated

2 years ago
Depends on: 1182691
https://hg.mozilla.org/mozilla-central/rev/6f07416ab9bb
(Assignee)

Comment 42

2 years ago
A mochitest-chrome job is now running on mozilla-central on Android 2.3 opt, Android 4.3 opt, and Android 4.3 debug; many tests are skipped (Comment 38) but the suite is functional and stable.

I filed bug 1184186 to follow up on Comment 37: I will investigate whether mochitest-chrome can be used for some Robocop tests; if that fails, I'll have a closer look at browser-chrome.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Awesome - thanks gbrown!
You need to log in before you can comment on or make changes to this bug.