Reduce the set of prefs in the default unittest set to only those required for stable/reliable tests

RESOLVED FIXED in Firefox 66

Status

enhancement
P3
normal
RESOLVED FIXED
Last year
7 months ago

People

(Reporter: jgraham, Assigned: KWierso)

Tracking

Version 3
mozilla66
Points:
---

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment)

The uniitest user.js file seems to be a mix of things that are required to make running tests run (and do so reliably), and optional features that are enabled in tests. For wpt we would like to be able to run against stable builds with a featureset that's close to what we ship. Therefore it would be good to move all the prefs that are just enabling non-shipping features into some other harness-specific location so that the minimal unittest set is changing only what is necessary to make the tests run.
Priority: -- → P3

FWIW, I did a bunch of try pushes with the various user.js prefs disabled.

This was just a baseline push of the current tip of m-c: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=33f2fa1ece788dbb635aa2219434b3c8cce1476f

This commented out browser.sessionstore.resume_from_crash: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=efc62bc8ab1161dbffa79022551c7c9bcccd9202

This commented out extensions.autoDisableScopes: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=79a7d11b8eb61c2d8eacee416b2a2822064c1a51

This commented out extensions.update.notifyUser: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=998cf0a9e765f53f8f69f43b88ece2e01dfb57d0

This commented out focusmanager.testmode: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=e5374316d03de78eb65b7b6d4245b640a604ff8e

This commented out media.navigator.streams.fake: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=838db54a852afcb17cef0b14f82b0ad0fbe168d9

This commented out network.preload: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=7b1111ab14117a2c6bf7631f42994b6d96d4887e

This commented out network.proxy.type: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=1d07ff0b24b6c856187a83edf9afe149bab0cf88

This commented out toolkit.startup.max_resumed_crashes: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=411e0a416bb66507b46f45d3825082c53d77a35b

This commented out gfx.font_ahem_antialias_none: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=ae7dbbc6f1ef574b0f8e0aa2592ca95b9d521ad5

This commented out network.http.phishy-userpass-length: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=9fb57dd201c7b27ed8703351f126a5dd99feb4a4

This commented out the whole block of safebrowsing prefs: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=b22cf52182b258a657c2de769ca6325fdd94368e

And this commented out dom.disable_beforeunload: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=72e5f059f3d9886205af0fc36f55b16d0bcd5a44

I didn't do a try push for commenting out places.history.enabled, as the comment above it makes it sound like that would do a lot of I/O. It may not need to be set in user.js, but it should probably be set somewhere for sure.

I haven't looked closely at the results, but some things jump out:

Commenting out the phishy userpass length pref seems to cause /fetch/security/embedded-credentials.tentative.sub.html to fail everywhere, and /html/webappapis/dynamic-markup-insertion/opening-the-input-stream/ignore-opens-during-unload.window.html crashed on both linux debug runs, but not on other platforms. There's also maybe an ASAN leak?

Commenting out the ahem antialiasing pref seems to have broken all non-linux wpt-reftest chunks in multiple tests.

Commenting out the media.navigator.streams.fake pref seems to cause a few test failures (/mediacapture-image/ImageCapture-creation.https.html and /feature-policy/reporting/camera-report-only.https.html, maybe more).

Wes, since you have started looking at this can you finish it off

Assignee: nobody → wkocher

So to be clear the goal is to move out any prefs that are just enabling features into a seperate pref set. So I think this is about knowing intent rather than something that can be figured out from try pushes. All the ones you mentioned are about test correctness/reliability rather than enabling non-default features, so they seem fine to apply to stable builds.

So for example in the unittest set, the following look like they are enabling features and so should be in a different pref set:

https://searchfox.org/mozilla-central/source/testing/profiles/unittest/user.js#122,173-182,208-210,238

That list isn't exhaustive, but hopefully it's indicative of the kind of thing we want to look for here.

So this would involve adding an additional file referenced like these[1], putting these prefs into that new file, and then only having the harnesses that need those prefs look up those files?

Any chance you know what controls which user.js files get applied in the various harnesses? The closest I can see is just the search results for [2], but that doesn't really help me.

  1. https://searchfox.org/mozilla-central/source/testing/profiles/moz.build#8-15
  2. https://searchfox.org/mozilla-central/search?q=TEST_HARNESS_FILES&path=
Flags: needinfo?(james)

Just found https://searchfox.org/mozilla-central/source/testing/profiles/profiles.json which I assume is where I'd need to also reference this new testing/profiles/[non-shipping-features-profile]/user.js file for the various harnesses that currently depend on unittest?

The unittest/user.js file contained a mix of preferences that ensure tests can run (switching web services to dummy servers to prevent hitting networks, etc) and preferences that enable features that aren't shipping by default (turning on touch events, enabling experimental css features, etc).

In the future, we're going to want to run tests of only the features that are being shipped in release builds (or in beta builds, or in esr builds, etc), so we need to move feature-enabling preferences into a different file to make it possible to run tests with various sets of features enabled.

This commit just moves feature-enabling prefs into a new file and then includes that file everywhere unittest prefs were already being included, so it should have no functional difference in the set of preferences being set in test runs.
Attachment #9035495 - Attachment description: Bug 1485386 - Move feature-enabling preferences out of unittest/user.js into a separate file f?jgraham → Bug 1485386 - Move feature-enabling preferences out of unittest/user.js into a separate file
Flags: needinfo?(james)

Just for fun, I did a try push where these moved prefs are not included in the various profiles[1].

Lots of failures, unsurprisingly. Wonder how many of these should just be set by tests that need them?

  1. https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=9b32bbe074ee7f5bb8e2ac6e4e698f1c3ee8c1c6
Pushed by wkocher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b9ab3e3e109a
Move feature-enabling preferences out of unittest/user.js into a separate file r=ahal

So, what would be the next step(s)? Actually moving these prefs into test/harness manifest metadata until we get to the point where the new unittest-features profile can be removed since it doesn't contain anything?

Flags: needinfo?(james)

Do you know why mozprofile would say the renamed unittest-stability profile wouldn't exist? Is there somewhere else I'd need to edit?

Flags: needinfo?(wkocher) → needinfo?(ahal)

Oh right, sorry I should have caught this in review (I forgot it needed to be done). You'll also have to make sure it gets copied into the tests.zip:
https://searchfox.org/mozilla-central/source/testing/profiles/moz.build#18

Just copy all profiles to all harnesses even if the harness doesn't use it (like wpt). At runtime only the profiles in profiles.json will be used anyway.

Flags: needinfo?(ahal)

If we have to do another round of fixes here anyway, could I bikeshed that we rename unittest-stability to unitest-required. Some of the prefs are to make the harnesses work at all, not just for stability. (I'm not super bothered if this doesn't happen, but I think it's epsilon clearer).

Flags: needinfo?(james)

(In reply to James Graham [:jgraham] from comment #13)

If we have to do another round of fixes here anyway, could I bikeshed that we rename unittest-stability to unitest-required

wfm

Pushed by wkocher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0e2384b8497d
Move feature-enabling preferences out of unittest/user.js into a separate file r=ahal
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Could someone please look at out documentation and update it?
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing#Need_to_set_preferences_for_test-suites

I'm looking into Bug 1519538 where some prefs that we currently set in runtests.py, don't seem to apply anymore to some test harnesses, and I have no idea where I should move those prefs, because there are too many locations (common, perf, unittest-required, unittest-features, ...).

Keywords: dev-doc-needed

Thanks for pointing that out! I'll spend some time updating that.

Tl;dr look at testing/profiles/profiles.json to see which profiles apply to which test suites, and choose accordingly. The current profile structure isn't super well thought out or anything, so feel free to add new profiles and/or re-organize as needed.

I updated it.

Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.