1520806 - Don't apply unittest-features prefs when running Firefox stable

Status: RESOLVED FIXED

(Testing :: web-platform-tests, enhancement)

Description

[James Graham [:jgraham]]

In the runinfo we have the browser_channel [1]
We can pass that into the Firefox class [2]
We can use that to select which pref set gets loaded [3]

So I think we need to add a web-platform-tests-stable set of prefs to the profiles.json file and then thread the information that we're running stable through to the place where we load the prefs so we can select that set rather than the full set.

[1] https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/wpttest.py#97
[2] https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox.py#88
[3] https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox.py#311

Comment 1

[James Graham [:jgraham]]

kwierso: are you planning to take this one?

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm trying to wrap my head around how this all would work. Hopefully you can help walk me through it a bit.

So at the moment, there are a number of places where prefs can be set to affect wpt tests:

• WPT test manifests (like [1]).
• The unittest-features profile user.js, and the other profiles that get applied [2] for web-platform-tests.
• I think whatever comes out of all.js[3] for the current build get applied? There's #ifdefs for EARLY_BETA_OR_EARLIER and NIGHTLY_BUILD that can affect the set of prefs used, among others.

If I understand things correctly, the priority of those preferences goes 3, 2, 1.

With the proposed changes in this bug, we would be swapping out [2] for a different set of preferences (web-platform-tests-stable) based on the release channel, right? Or just not including unittest-features, but leave the others?

Where is that other set of preferences being defined? It would very quickly fall out of date if it's a hardcoded list of prefs that needs someone to manually update.

Can we instead just ignore the unittest-features profile on non-mozilla-central builds and let all.js handle the list of prefs we use?

None of this prevents prefs set in wpt manifests from overriding these preferences, and wpt tests really should be setting dependent prefs in their manifests, so I'm not sure how we can run tests against a set of "stable" Firefox prefs for release without adding additional complexity to test manifests to say something like "don't set this pref if run against release" or "expect this test to fail on release builds".

[1] https://searchfox.org/mozilla-central/source/testing/web-platform/meta/2dcontext/imagebitmap/createImageBitmap-origin.sub.html.ini#2
[2] https://searchfox.org/mozilla-central/source/testing/profiles/profiles.json#9
[3] https://hg.mozilla.org/mozilla-central/file/tip/modules/libpref/init/all.js

(As an aside, I wonder how many prefs in https://searchfox.org/mozilla-central/source/testing/profiles/web-platform/user.js should be folded into the unittest-stability profile?)

Comment 3

[James Graham [:jgraham]]

I think the point that I wasn't clear on is that we only really care about the difference between stable and nightly for the upstream use in generating wpt.fyi results. I think that's the only codepath the passes in a browser_channel argument, so it should be safe to just make the prefset conditional on that without affecting release branch builds of gecko on our CI. But it's probably worth checking that that's the case.

So for upstream we don't have any pref annotations (although that may change; if it does we'll do it per channel) and all.js is whatever's shipped with the browser. So the only point of difference from the standard release build is the prefs we are applying in the profiles.json set i.e. your [2]. The idea is to reduce that set for stable release builds running outside gecko ci only to the minimal set needed for tests to actually run.

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I wasn't able to test this codepath from mozilla-central (or at least, I couldn't figure out how to test it), so I did this as a commit to the wpt repo on github.

I think this does what we want it to do:

I'm able to access browser_channel from https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox.py#311 and it should now omit the unittest-features profile from pref_paths if browser_channel is not 'nightly' (which I think the only options are 'nightly' and 'stable', with a bunch of aliases for other browsers, plus 'None' when invoked via mach?).

When I run ./wpt run firefox pointerevents/pointerevent_pointermove.html --log-mach ~/Desktop/wptrun.log --browser-channel nightly it runs the test successfully as expected, since all of the profiles are included.

When I run ./wpt run firefox pointerevents/pointerevent_pointermove.html --log-mach ~/Desktop/wptrun.log --browser-channel stable it hangs for many minutes after printing "Running 1 tests in web-platform-tests", eventually printing these stack traces: https://gist.github.com/KWierso/af18fc662644b60c80ef250853ccbb1d

I'm guessing release Firefox isn't configured out of the box with marionette support? Is that a compile-time thing, or is there a pref we can set in the unittest-stability or web-platform pref profiles to flip it on?

Here's the commit: https://github.com/KWierso/wpt/commit/c935d6d00edd6c893c17545e7c886fd5d3ce72c1?diff=unified

If this looks correct or at least along the lines I need to proceed, I can either submit this as a PR to the github repo or port it over to m-c and push it to phabricator for review, whichever works better for you.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Bug 1520806 - Don't apply unittest-features prefs when running Firefox stable

This skips over adding the unittest-features profile's user.js prefs if the browser channel is not Nightly. This should in theory mean the wpt run will only use preferences necessary for the tests to technically run, plus whatever the version of Firefox used for the tests includes as default out of the box prefs.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Meh, here it is as an m-c patch. Nothing breaks with this running via mach.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

So, I did a try push to see what automation thinks of this patch: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=success%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception&classifiedState=unclassified&group_state=expanded&revision=5648781033af2c859e7a2ff7110a7673d170c336

It looks like it isn't applying the unittest-features profile. I guess I need to find out what browser_channel is getting applied for these builds?

Comment 8

[James Graham [:jgraham]]

AH, maybe we need to allow the argument to be None? I thought we always normalised it to a string defaulting to nightly, but perhaps not.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

So I did a try run at https://hg.mozilla.org/try/rev/acb5932400eddade6dc5cf010acec05ff15cf421 where I defaulted browser_channel to "nightly" in a new place to see if that worked https://hg.mozilla.org/try/rev/acb5932400eddade6dc5cf010acec05ff15cf421 which it did not.

Then I did a try run at https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=success%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception&classifiedState=unclassified&group_state=expanded&searchStr=linux%2Cx64%2Cdebug&selectedJob=226029864&revision=98098e010c6d211c988a7fa15a13bc14c5a5c2bb with that change AND allowing the argument to be None https://hg.mozilla.org/try/rev/3af696bafa85fb34c3f1f5fa415e9fdf34f45d84 and that appears to be working, though the results are still coming in.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Bug 1520806 - Try defaulting to 'nightly' channel in another place r?jgraham

Depends on D17428

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This appears to work. I can fold this into the first revision before landing this.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac194fa2a330
Don't apply unittest-features prefs when running Firefox stable r=jgraham

Comment 13

[Cristina Coroiu [:ccoroiu]]

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

Comment 14

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15442 for changes under testing/web-platform/tests

Comment 15

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged