1736859 - Ensure all test harnesses are explicitly enabling or disabling fission

Status: RESOLVED FIXED

(Testing :: General, task, P1)

Description

[Andrew Halberstadt [:ahal]]

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:

1. We'll lose non-fission test coverage
2. 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.

Comment 1

[Andrew Halberstadt [:ahal]]

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:

1. Make sure that we override this value when enabling fission at runtime
2. Make sure harnesses are all setting up the test manifest sandbox appropriately (by inspecting the pref rather than CLI arguments)
3. Check for suites / tasks that don't use that base/user.js file

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

:ahal, any idea whom this bug should be assigned to? Thanks!

Comment 3

[Andrew Halberstadt [:ahal]]

Yes, I'm working on it. Thanks for pointing that out :)

Comment 4

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1736859 - [mochitest] Dump some prefs that are set in TestRunner.js, r?gbrown

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.

Comment 5

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1736859 - [testing/profiles] Explicitly disable fission in all test harnesses, r?gbrown

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

Comment 6

[Andrew Halberstadt [:ahal]]

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.

Comment 7

[Andrew Halberstadt [:ahal]]

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

[Joel Maher ( :jmaher ) (UTC -8)]

we also have some source-test items, and there is the build-pgo process to consider as well.

Comment 9

[Andrew Halberstadt [:ahal]]

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.

Comment 10

[Andrew Halberstadt [:ahal]]

(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.

Comment 11

[Andrew Halberstadt [:ahal]]

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!

Comment 12

[Andrew Halberstadt [:ahal]]

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

[Chris H-C :chutten]

(In reply to Andrew Halberstadt [:ahal] from comment #11)

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!

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?

Comment 14

[Andrew Halberstadt [:ahal]]

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

[Chris H-C :chutten]

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?

Comment 16

[Andrew Halberstadt [:ahal]]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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 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.

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

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Andrew Halberstadt [:ahal] from comment #12)

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?

:mccr8 you might be most familiar with awsy at this time, otherwise my team would be happy to help (either :sparky or :alexandrui)

Comment 19

[Andrew Halberstadt [:ahal]]

(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 to false in geckoinstance.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?

Comment 20

[Andrew Halberstadt [:ahal]]

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

[Greg Mierzwinski [:sparky]]

(In reply to Andrew Halberstadt [:ahal] from comment #7)

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.

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

Comment 22

[Chris H-C :chutten]

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.

Comment 23

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1736859 - [marionette] Explicitly turn off 'fission.autostart' if --enable-fission is not used, r?jdescottes

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

[James Graham [:jgraham]]

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

Comment 25

[Andrew Halberstadt [:ahal]]

Sounds like we have awsy covered and James has replied for the marionette patch. Clearing remaining NI's.

Comment 26

[Andrew Halberstadt [:ahal]]

Here's a try run which simulates flipping the pref (I think):
https://treeherder.mozilla.org/jobs?repo=try&revision=aff29ac29bd271978551067df11a21851311672c

Comment 27

[Andrew McCreight [:mccr8]]

(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!

Comment 28

[Andrew Halberstadt [:ahal]]

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.

Comment 29

[Andrew Halberstadt [:ahal]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0140d91697c6
[mochitest] Dump some prefs that are set in TestRunner.js, r=gbrown
https://hg.mozilla.org/integration/autoland/rev/09d969a5b70e
[testing/profiles] Explicitly disable fission in all test harnesses, r=gbrown
https://hg.mozilla.org/integration/autoland/rev/edd0ea14fcca
[marionette] Explicitly turn off 'fission.autostart' if --enable-fission is not used, r=webdriver-reviewers,jgraham

Comment 31

[Alexandru Michis [:malexandru]]

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

[Chris Peterson [:cpeterson]]

Setting status-firefox94=wontfix because we don't need to uplift this test fix to Beta.

Comment 33

[Andrew Halberstadt [:ahal]]

It occurred to me that I didn't catch puppeteer here. Will submit another patch shortly.

Comment 34

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1736859 - Explicitly disable fission if '--enable-fission' is not set in |mach puppeteer-test|, r?jgraham

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b24f9ac43772
Explicitly disable fission if '--enable-fission' is not set in |mach puppeteer-test|, r=jgraham,webdriver-reviewers

Comment 36

[Cosmin Sabou [:CosminS]]

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