Ensure all test harnesses are explicitly enabling or disabling fission
Categories
(Testing :: General, task, P1)
Tracking
(Fission Milestone:Future, firefox-esr78 wontfix, firefox-esr91 wontfix, firefox93 wontfix, firefox94 wontfix, firefox95 fixed)
People
(Reporter: ahal, Assigned: ahal)
References
Details
Attachments
(4 files)
In bug 1732358 we will be switching the default value for fission.autostart
to True, sometime in the Nightly 96 cycle (which starts Nov 1st).
Once this happens, any test harness that isn't explicitly turning fission off will magically start running with fission. This means that:
- We'll lose non-fission test coverage
- Task labels will be wrong (as well as consumers that depend on them)
Before this switch happens, we should make sure that all of our test harnesses either explicitly enable or disable fission so that flipping the pref has no effect on our CI or test coverage.
Assignee | ||
Comment 1•3 years ago
|
||
Solving this might be as easy as adding fission.autostart=false
to https://searchfox.org/mozilla-central/source/testing/profiles/base/user.js
However, there are a few other considerations:
- Make sure that we override this value when enabling fission at runtime
- Make sure harnesses are all setting up the test manifest sandbox appropriately (by inspecting the pref rather than CLI arguments)
- Check for suites / tasks that don't use that
base/user.js
file
Comment 2•3 years ago
|
||
:ahal, any idea whom this bug should be assigned to? Thanks!
Assignee | ||
Comment 3•3 years ago
|
||
Yes, I'm working on it. Thanks for pointing that out :)
Assignee | ||
Comment 4•3 years ago
|
||
This will help us validate that are active in the test run are actually the
ones that the Python side of the harness is reporting.
Assignee | ||
Comment 5•3 years ago
|
||
Currently, many of the test harnesses are relying on the default value of this
pref to turn it off. Once the default value switches to 'true', they will all
suddenly start running with fission.
We explicitly set it to false to avoid this. Tasks that enable fission via
config will override this value.
Once the default value is switched and riding the trains, we can look into
removing this and adding configuration to disable rather than enable it.
Depends on D129308
Assignee | ||
Comment 6•3 years ago
|
||
I've been doing some verifications, and looks like adding the pref to testing/profiles/base/user.js
works for all suites that use this system. This includes:
- mochitest (all flavors)
- wpt
- reftest / crashtest
- xpcshell (though we don't run with fission here)
- raptor
- talos
There are still some suites that don't use the above profile system and are at risk here:
- awsy
- firefox-ui
- marionette
- puppeteer
- telemetry-tests-client
I'm unsure about browsertime
. I think this uses the same profile as raptor
, but someone on the perftest team should confirm.
There may also be some python-test
tests that spin up Firefox, but it should be fine if we suddenly start using fission for those.
Assignee | ||
Comment 7•3 years ago
|
||
Hey Greg, do you know if browsertime
tasks use the testing/profiles/raptor
profile for setting prefs? If not they are at risk due to the impending fission.autostart
default switch.
Comment 8•3 years ago
|
||
we also have some source-test items, and there is the build-pgo process to consider as well.
Assignee | ||
Comment 9•3 years ago
|
||
Hey Henrik, the fission.autostart
pref is about to switch defaults. This means any test harness that doesn't explicitly turn it off. I've been scanning some harnesses and looks like marionette
, puppeteer
and firefox-ui
tests are all at risk here. Can you confirm whether they are explicitly turning off fission or not?
Would you be able to work on a patch if they aren't? Or if not, could you point me towards the best place for setting prefs in each? Thanks.
Assignee | ||
Comment 10•3 years ago
|
||
(In reply to Joel Maher ( :jmaher ) (UTC -0800) from comment #8)
we also have some source-test items, and there is the build-pgo process to consider as well.
Yeah, the source-test
stuff is the Python tests I mentioned. I don't think there's anything in there that will be troublesome, but I could be wrong. As for build pgo
do you mean the profileserver stuff? That actually does use the testing/profiles
mechanism afaict, but I've never really understood what this does... so would be good for someone else to verify.
Assignee | ||
Comment 11•3 years ago
|
||
Hey Chris, the fission.autostart
pref is about to switch defaults. This means telemetry-tests-client
will magically become fission-only (even the non-Fission version) unless we explicitly start turning this off.
Is there any chance you can work on a patch here, or failing that can you point me towards the best place to set prefs? Thanks!
Assignee | ||
Comment 12•3 years ago
|
||
Hey Dave, fission.autostart
is about to flip defaults and awsy
is one of the suites at risk of changing behaviour when it does. Is that something your team owns? Or do you know who is a good point of contact if not?
Comment 13•3 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #11)
Hey Chris, the
fission.autostart
pref is about to switch defaults. This meanstelemetry-tests-client
will magically become fission-only (even the non-Fission version) unless we explicitly start turning this off.Is there any chance you can work on a patch here, or failing that can you point me towards the best place to set prefs? Thanks!
Bringing in Raphael for tt(c) knowledge.
Prefs are in toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/runner.py
. To my knowledge Fission or non-Fission doesn't really matter to tt(c) (we're happy to cover whatever situation is most "normal"), so which would you prefer?
Assignee | ||
Comment 14•3 years ago
|
||
They currently run both with and without:
https://treeherder.mozilla.org/jobs?repo=mozilla-central&searchStr=telemetry-tests
So the issue is that the non-fission task will also start running with it enabled. Once fission
is default, maybe we can turn off the non-fission tasks. But to avoid a coordination dance, it's probably best to just explicitly turn it off there for now.
Comment 15•3 years ago
|
||
But if we explicitly turn it off, won't we be testing non-fission in both configs? Or is that the beauty of .autostart
, that setting it to false
doesn't prevent fission configs from overriding us?
Assignee | ||
Comment 16•3 years ago
|
||
Yes it would, but I meant that we should either explicitly turn it off or explicitly turn it on. But I assume that the ttc-fis
tasks are already explicitly turning it on, so we don't need to worry about that case.
Comment 17•3 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #9)
Hey Henrik, the
fission.autostart
pref is about to switch defaults. This means any test harness that doesn't explicitly turn it off. I've been scanning some harnesses and looks likemarionette
,puppeteer
andfirefox-ui
tests are all at risk here. Can you confirm whether they are explicitly turning off fission or not?Would you be able to work on a patch if they aren't? Or if not, could you point me towards the best place for setting prefs in each? Thanks.
I'm off for a week and will be back beginning of November. So if a patch is needed before someone else has to work on it. Also note that firefox-ui, telemetry-client and awsy are all dependent on Marionette. But we sometime still have to release a Marionette client package and official releases should not have Fission turned off. So changing fission.autostart
to false
in geckoinstance.py
is not an option for us.
Andrew, why can't we do that similar to e10s
and use the --disable-fission
flag for test suites? That would make things way easier and do not require harness code changes.
Comment 18•3 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #12)
Hey Dave,
fission.autostart
is about to flip defaults andawsy
is one of the suites at risk of changing behaviour when it does. Is that something your team owns? Or do you know who is a good point of contact if not?
:mccr8 you might be most familiar with awsy at this time, otherwise my team would be happy to help (either :sparky or :alexandrui)
Assignee | ||
Comment 19•3 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] (away 10/23 - 10/31) from comment #17)
I'm off for a week and will be back beginning of November. So if a patch is needed before someone else has to work on it. Also note that firefox-ui, telemetry-client and awsy are all dependent on Marionette. But we sometime still have to release a Marionette client package and official releases should not have Fission turned off. So changing
fission.autostart
tofalse
ingeckoinstance.py
is not an option for us.
It doesn't have to be in the harness itself, it could be in the task configs instead. We just need to make sure that pref is explicitly set in both the fission and non-fission tasks.
But are you implying that fission.autostart
already defaults to True in Marionette and the harnesses that use it? If so then there's no action needed here.
Andrew, why can't we do that similar to
e10s
and use the--disable-fission
flag for test suites? That would make things way easier and do not require harness code changes.
That's the ideal end state, but the deadline here is tight and switching all the harnesses to --disable-fission
will be a fair bit of work.
Also if you are out and more work is needed, who is a good person to follow-up with these days?
Assignee | ||
Comment 20•3 years ago
|
||
So according to Henrik, tt-c, awsy, marionette and firefox-ui all use the marionette driver. If that's the case, I think we just have to add an:
else:
self.prefs.update("fission.autostart=false")
clause here. In Henrik's absence, I'm not sure who is the best person to confirm / review.. maybe Raphael will know.
Assuming my guess above is accurate, this just leaves puppeteer
.
Comment 21•3 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #7)
Hey Greg, do you know if
browsertime
tasks use thetesting/profiles/raptor
profile for setting prefs? If not they are at risk due to the impendingfission.autostart
default switch.
Hi :ahal, yes we make use of the profile in testing/profiles/raptor
along with these ones: https://searchfox.org/mozilla-central/source/testing/profiles/profiles.json#5
Updated•3 years ago
|
Comment 22•3 years ago
|
||
Raphael's on PTO for the week, so I'm clearing his ni? for now. From hglog, maybe jdescottes knows the answer to ahal's question in c20? Ack, alas they're out, too. Perhaps jgraham? His name's in the now-archived marionette-reviewers
phab group, so he might know things about the marionette harness and prefs.
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 23•3 years ago
|
||
The 'fission.autostart' pref is about to change defaults which means that if we
don't explicitly disable it when '--enable-fission' is not passed in, we'll
actually be running with it enabled.
Eventually we should remove '--enable-fission' in favour of '--disable-fission',
but that can be follow-up work once the pref has been switched.
Depends on D129309
Comment 24•3 years ago
|
||
Random note: changing testing/profiles/base/user.js
is going to also affect the wpt tests we run upstream (i.e. the ones that show up on https://wpt.fyi) I think we want that to follow whatever the default for shipping is rather than relying on explicit opt-in.
Regarding marionette, I think that change would probably work (and I probably am the right person to ask given PTO this week).
Assignee | ||
Comment 25•3 years ago
|
||
Sounds like we have awsy
covered and James has replied for the marionette patch. Clearing remaining NI's.
Assignee | ||
Comment 26•3 years ago
|
||
Here's a try run which simulates flipping the pref (I think):
https://treeherder.mozilla.org/jobs?repo=try&revision=aff29ac29bd271978551067df11a21851311672c
Comment 27•3 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #25)
Sounds like we have
awsy
covered and James has replied for the marionette patch. Clearing remaining NI's.
Thanks for the fix!
Updated•3 years ago
|
Assignee | ||
Comment 28•3 years ago
|
||
Try run is looking good so far, did a few retriggers.
Also toolkit/components/featuregates/test/unit/test_FeatureGate.js
is failing, but this test looks like it is designed to prevent features from accidentally shipping before intended, so I think it's ok / expected that it is failing. I'm assuming there's some out-of-tree bit it is checking to verify whether the pref should be on or not.
Assignee | ||
Comment 29•3 years ago
|
||
Aside from test_FeatureGate.js
, there were a couple perma (or highly intermittent) fails in my push. But I verified and the tasks are running with the expected fission value. So I don't think they're related to my push...
Given Friday is a day off, and the merge day is Monday.. I think it's best to land this now and look for fallout on autoland.
Comment 30•3 years ago
|
||
Comment 31•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0140d91697c6
https://hg.mozilla.org/mozilla-central/rev/09d969a5b70e
https://hg.mozilla.org/mozilla-central/rev/edd0ea14fcca
Comment 32•3 years ago
|
||
Setting status-firefox94=wontfix because we don't need to uplift this test fix to Beta.
Assignee | ||
Comment 33•3 years ago
|
||
It occurred to me that I didn't catch puppeteer
here. Will submit another patch shortly.
Assignee | ||
Comment 34•3 years ago
|
||
This ensures that when the default value of 'fission.autostart' changes, we
don't accidentally start running with fission enabled even without
'--enable-fission'.
As a follow-up, we should switch the flag to '--disable-fission' once the pref
has flipped.
Comment 35•3 years ago
|
||
Comment 36•3 years ago
|
||
bugherder |
Description
•