Last Comment Bug 1026290 - Add support for mochitest-chrome to Android
: Add support for mochitest-chrome to Android
Status: RESOLVED FIXED
[LOE:M]
: leave-open
Product: Testing
Classification: Components
Component: Mochitest Chrome (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: Geoff Brown [:gbrown]
:
:
Mentors:
Depends on: 1182691
Blocks: 780955 797164
  Show dependency treegraph
 
Reported: 2014-06-16 17:16 PDT by Matthew N. [:MattN] (PM if requests are blocking you)
Modified: 2015-07-15 14:20 PDT (History)
22 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
(1) delay redirect until after MozAfterPaint (1.10 KB, patch)
2014-11-19 12:49 PST, Geoff Brown [:gbrown]
jmaher: review+
Details | Diff | Splinter Review
(2) Do not filter log* parameters from testConfig.js (1.14 KB, patch)
2014-11-24 15:59 PST, Geoff Brown [:gbrown]
no flags Details | Diff | Splinter Review
(2) do not filter logFile parameter from testConfig.js (1.29 KB, patch)
2014-11-24 19:04 PST, Geoff Brown [:gbrown]
cmanchester: review+
Details | Diff | Splinter Review
(3) push chrome test files to device (5.08 KB, patch)
2014-11-25 09:04 PST, Geoff Brown [:gbrown]
jmaher: review+
Details | Diff | Splinter Review
(4) work around options.dm also in makeTestConfig (1.54 KB, patch)
2015-06-04 16:00 PDT, Geoff Brown [:gbrown]
no flags Details | Diff | Splinter Review
(4b) work around options.dm also in makeTestConfig (1.75 KB, patch)
2015-06-05 13:33 PDT, Geoff Brown [:gbrown]
cmanchester: review+
Details | Diff | Splinter Review
(5) update chrome.ini files for android (22.92 KB, patch)
2015-07-10 06:04 PDT, Geoff Brown [:gbrown]
jgriffin: review+
Details | Diff | Splinter Review

Description User image Matthew N. [:MattN] (PM if requests are blocking you) 2014-06-16 17:16:19 PDT
+++ 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 User image :Paolo Amadini 2014-06-17 16:19:17 PDT
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.
Comment 2 User image Joel Maher ( :jmaher) 2014-06-17 16:32:37 PDT
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.
Comment 3 User image Geoff Brown [:gbrown] 2014-10-28 10:54:58 PDT
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
Comment 4 User image Geoff Brown [:gbrown] 2014-10-28 17:07:24 PDT
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.
Comment 5 User image Joel Maher ( :jmaher) 2014-10-28 17:16:48 PDT
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.
Comment 6 User image Geoff Brown [:gbrown] 2014-10-30 15:02:08 PDT
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.
Comment 7 User image Geoff Brown [:gbrown] 2014-11-19 09:24:23 PST
(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.
Comment 8 User image Geoff Brown [:gbrown] 2014-11-19 12:31:39 PST
(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
Comment 9 User image Geoff Brown [:gbrown] 2014-11-19 12:49:39 PST
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
Comment 10 User image Joel Maher ( :jmaher) 2014-11-19 12:51:19 PST
Comment on attachment 8525567 [details] [diff] [review]
(1) delay redirect until after MozAfterPaint

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

nice and simple.
Comment 11 User image Geoff Brown [:gbrown] 2014-11-19 14:45:44 PST
(1) https://hg.mozilla.org/integration/mozilla-inbound/rev/6d8adc89991c
Comment 12 User image Carsten Book [:Tomcat] 2014-11-20 02:45:45 PST
https://hg.mozilla.org/mozilla-central/rev/6d8adc89991c
Comment 13 User image Geoff Brown [:gbrown] 2014-11-24 08:08:03 PST
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.
Comment 14 User image Geoff Brown [:gbrown] 2014-11-24 08:15:58 PST
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.)
Comment 15 User image Geoff Brown [:gbrown] 2014-11-24 15:59:49 PST
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
Comment 16 User image Chris Manchester (:chmanchester) 2014-11-24 16:12:47 PST
(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?
Comment 17 User image Geoff Brown [:gbrown] 2014-11-24 18:09:13 PST
(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.
Comment 18 User image Chris Manchester (:chmanchester) 2014-11-24 18:33:13 PST
(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()])))
Comment 19 User image Geoff Brown [:gbrown] 2014-11-24 19:04:40 PST
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!
Comment 20 User image Ahmed Kachkach [:akachkach] 2014-11-24 21:54:41 PST
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)
Comment 21 User image Geoff Brown [:gbrown] 2014-11-25 09:04:27 PST
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.
Comment 22 User image Joel Maher ( :jmaher) 2014-11-25 09:46:12 PST
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!
Comment 23 User image Chris Manchester (:chmanchester) 2014-11-25 10:04:40 PST
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.
Comment 25 User image Geoff Brown [:gbrown] 2014-11-25 22:21:44 PST
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
Comment 26 User image Geoff Brown [:gbrown] 2014-11-25 22:38:28 PST
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.
Comment 29 User image Matthew N. [:MattN] (PM if requests are blocking you) 2015-03-18 11:39:15 PDT
Hey Geoff, what's the status of this? I don't see it on TreeHerder and there is leave-open here.
Comment 30 User image Geoff Brown [:gbrown] 2015-03-18 12:35:39 PDT
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?
Comment 31 User image Geoff Brown [:gbrown] 2015-06-04 16:00:35 PDT
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.
Comment 32 User image Chris Manchester (:chmanchester) 2015-06-04 16:21:50 PDT
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.
Comment 33 User image Geoff Brown [:gbrown] 2015-06-05 13:33:55 PDT
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.
Comment 34 User image Chris Manchester (:chmanchester) 2015-06-08 10:41:53 PDT
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'
Comment 36 User image Wes Kocher (:KWierso) 2015-06-09 19:03:12 PDT
https://hg.mozilla.org/mozilla-central/rev/8d0e1a4cdc46
Comment 37 User image Nick Alexander :nalexander 2015-06-19 14:22:56 PDT
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!).
Comment 38 User image Geoff Brown [:gbrown] 2015-07-10 06:04:36 PDT
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).
Comment 39 User image Jonathan Griffin (:jgriffin) 2015-07-10 09:38:32 PDT
Comment on attachment 8632083 [details] [diff] [review]
(5) update chrome.ini files for android

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

Nice!
Comment 41 User image Carsten Book [:Tomcat] 2015-07-13 04:22:49 PDT
https://hg.mozilla.org/mozilla-central/rev/6f07416ab9bb
Comment 42 User image Geoff Brown [:gbrown] 2015-07-15 12:01:11 PDT
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.
Comment 43 User image Bobby Holley (:bholley) (busy with Stylo) 2015-07-15 14:20:23 PDT
Awesome - thanks gbrown!

Note You need to log in before you can comment on or make changes to this bug.