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

RESOLVED FIXED in Firefox 67

Status

enhancement
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: jgraham, Assigned: KWierso)

Tracking

Version 3
mozilla67
Points:
---

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

5 months ago

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

Reporter

Comment 1

5 months ago

kwierso: are you planning to take this one?

Flags: needinfo?(wkocher)
Assignee

Updated

5 months ago
Assignee: nobody → wkocher
Flags: needinfo?(wkocher)
Assignee

Comment 2

5 months ago

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?)

Flags: needinfo?(james)
Reporter

Comment 3

5 months ago

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.

Flags: needinfo?(james)
Assignee

Comment 4

5 months ago

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.

Flags: needinfo?(james)
Assignee

Comment 5

5 months ago

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.

Assignee

Comment 6

5 months ago

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

Flags: needinfo?(james)
Assignee

Updated

5 months ago
Attachment #9038687 - Flags: review?(james)
Assignee

Comment 7

5 months ago

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?

Flags: needinfo?(james)
Reporter

Comment 8

5 months ago

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.

Flags: needinfo?(james)
Reporter

Updated

5 months ago
Attachment #9038687 - Flags: review?(james)
Assignee

Comment 11

4 months ago

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

Attachment #9041969 - Attachment is obsolete: true

Comment 12

4 months ago
Pushed by wkocher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac194fa2a330
Don't apply unittest-features prefs when running Firefox stable r=jgraham

Comment 13

4 months ago
bugherder
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15442 for changes under testing/web-platform/tests
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.