Closed Bug 1393326 Opened 2 years ago Closed 2 years ago
Android test runner needs to set --run-by-manifest ?
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 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  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.)
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.
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?
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!
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
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.
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 firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa04a602eba Enable runByManifest for Android mochitests and run Android mochitest-chrome in more chunks; r=jmaher
\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: 1439037
You need to log in before you can comment on or make changes to this bug.