Closed Bug 1393326 Opened 2 years ago Closed 2 years ago

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

Categories

(Testing :: Mochitest, defect)

Version 3
defect
Not set

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: birtles, Assigned: gbrown)

References

Details

Attachments

(1 file)

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.)
Flags: needinfo?(ahalberstadt)
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?
How is the prefs= annotation supposed to be used if any tests that depend on it can't be run on Android?
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.
Flags: needinfo?(ahalberstadt)
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!
(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.)
ok, lets give it a try- possibly we will have a handful of failures at first, but some other failures might go away.
Were you, or Joel, perhaps, going to try getting this to work?
Flags: needinfo?(ahalberstadt)
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!
Flags: needinfo?(ahalberstadt)
Aha! Thanks. I'll make a brief attempt.
Assignee: nobody → gbrown
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
Depends on: 1435433
Depends on: 1435624
Depends on: 1436262
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.
run-by-manifest is restarting the browser between each manifest.  This isolates tests from being affected by previous tests in other manifests.
Depends on: 1436642
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
Attachment #8950111 - Flags: review?(jmaher)
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.
Attachment #8950111 - Flags: review?(jmaher) → review+
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa04a602eba
Enable runByManifest for Android mochitests and run Android mochitest-chrome in more chunks; r=jmaher
https://hg.mozilla.org/mozilla-central/rev/cfa04a602eba
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
\o/

Thank you Geoff!
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
Depends on: 1427236
You need to log in before you can comment on or make changes to this bug.