1393326 - Android test runner needs to set --run-by-manifest ?

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

In bug 1382841 I tried to use the new prefs= annotation to tidy up some tests. It works well but when I ran it through try[1] on Android I get:

   0 ERROR parsing dom/animation/test/mochitest.ini: runByManifest mode must be enabled to set the `prefs` key

See: https://treeherder.mozilla.org/logviewer.html#?job_id=125386170&repo=try&lineNumber=1762

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c45a76f782d50b7eb79e34ec0c8ac58c31ba402&selectedJob=125386170

(This is likely the wrong component for this bug so feel free to update it accordingly.)

Comment 1

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

for android there is a very high cost in startup which is why restarting the browser an extra 10+ times/run would not be possible.  If we were to do this we could switch to more chunks, probably from 48 to at least 75.

Is this needed for Android?

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

How is the prefs= annotation supposed to be used if any tests that depend on it can't be run on Android?

Comment 3

[Andrew Halberstadt [:ahal]]

Crap I forgot Android doesn't use --run-by-manifest :/. That's a bummer.

The prefs annotation was a specific request to solve a particular use case where the test needed to set a pref before startup. I guess for now new tests that depend on that will need to skip-if Android, and tests that can set prefs via SpecialPowers should do that instead.

I think ideally we'd want --run-by-manifest on Android too as that should help reduce intermittents. But it's a trade off against AWS usage. It would also be possible to get this working so that even without '--run-by-manifest', the harness is able to detect manifests that set a pref and run them separately. This way we'd only restart the browser once per manifest that has prefs in it (as opposed to every manifest). If we decide --run-by-manifest is not desired on Android, this might be a good compromise.

Comment 4

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

possibly we just run one manifest per chunk for android and then we get run-by-manifest :)  if we did that, I would want to make sure that we are at least testing something on each chunk instead of spending 15+ minutes spinning up the emulator just to find out that specific directory doesn't have any tests to run on android!

Comment 5

[Geoff Brown [:gbrown]]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #4)
> possibly we just run one manifest per chunk for android and then we get
> run-by-manifest :)  if we did that, I would want to make sure that we are at
> least testing something on each chunk instead of spending 15+ minutes
> spinning up the emulator just to find out that specific directory doesn't
> have any tests to run on android!

It's surely not that bad, is it? The mozharness script for a job starts the emulator and then calls runtestsremote.py. runtestsremote.py in run-by-manifest mode just needs to start/stop the browser -- it can and should keep using the same emulator session. We don't need one manifest per task, do we?

(Starting/stopping the browser is more expensive on Android than on desktop, but I expect it to be less than 1 minute per browser restart.)

Comment 6

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

ok, lets give it a try- possibly we will have a handful of failures at first, but some other failures might go away.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Were you, or Joel, perhaps, going to try getting this to work?

Comment 8

[Geoff Brown [:gbrown]]

Did I do this right?

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

Comment 9

[Andrew Halberstadt [:ahal]]

Brian: It's still on the radar, though no one's had much of chance the last couple quarters to look at it. Maybe we can find some time to test it out a little this quarter or next, though can't guarantee anything :).

Geoff: No, you'll need to flip this:
https://searchfox.org/mozilla-central/rev/eeb7190f9ad6f1a846cd6df09986325b3f2c3117/testing/mochitest/runtestsremote.py#312

I'd be very shocked if it were that easy, but I guess you never know!

Comment 10

[Geoff Brown [:gbrown]]

Aha! Thanks. I'll make a brief attempt.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Thanks for giving it a try Geoff! It'd be extra-nice to get this fixed, because this is actually making us lose a lot of test coverage for CSS properties that are preffed of by default, and the way to work around it is kind of ugly, see https://bugzilla.mozilla.org/attachment.cgi?id=8947678

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Though I am misunderstanding what the problem here is, but I guess most mochitest-plain does not require browser restart, it just needs to reload the content after setting the pref value.

Comment 13

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

run-by-manifest is restarting the browser between each manifest.  This isolates tests from being affected by previous tests in other manifests.

Comment 14

[Geoff Brown [:gbrown]]

Attachment: enable run-by-manifest on Android mochitests

The only remaining issue I see is that there is now more variation in task run time for some plain mochitest chunks. In particular, some chunks have more short manifests so start/stop more than other chunks -- and therefore have longer run times. Nothing is timing out, so maybe this is OK and the best we can do, but I'd welcome suggestions for improvement.

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

Comment 15

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

Comment on attachment 8950111 [details] [diff] [review]
enable run-by-manifest on Android mochitests

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

I think we still have improvements to make on desktop, so ensuring we are running our tests the same way on all platforms allows us to make better decisions.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa04a602eba
Enable runByManifest for Android mochitests and run Android mochitest-chrome in more chunks; r=jmaher

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/cfa04a602eba

Comment 18

[Hiroyuki Ikezoe (:hiro)]

\o/

Thank you Geoff!

Comment 19

[Ionuț Goldan [:igoldan]]

This updated our baselines for Resident Memory:

== Change summary for alert #11516 (as of Mon, 12 Feb 2018 14:06:14 GMT) ==

Improvements:

 23%  Resident Memory android-4-3-armv7-api16 opt      189,765,362.08 -> 146,380,418.08
 18%  Resident Memory android-4-2-x86 opt              189,387,687.25 -> 154,936,306.50

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11516