Closed Bug 1736859 Opened 3 years ago Closed 3 years ago

Ensure all test harnesses are explicitly enabling or disabling fission

Categories

(Testing :: General, task, P1)

Default
task

Tracking

(Fission Milestone:Future, firefox-esr78 wontfix, firefox-esr91 wontfix, firefox93 wontfix, firefox94 wontfix, firefox95 fixed)

RESOLVED FIXED
95 Branch
Fission Milestone Future
Tracking Status
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:

  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.

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

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

Flags: needinfo?(ahal)

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

Assignee: nobody → ahal
Flags: needinfo?(ahal)

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.

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

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.

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.

Flags: needinfo?(gmierz2)

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

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.

Flags: needinfo?(hskupin)

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

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!

Flags: needinfo?(chutten)

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?

Flags: needinfo?(dave.hunt)

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

Flags: needinfo?(chutten) → needinfo?(rpierzina)

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.

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?

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.

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

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

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

Flags: needinfo?(dave.hunt) → needinfo?(continuation)

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

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

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.

(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

Flags: needinfo?(gmierz2)

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.

Flags: needinfo?(rpierzina) → needinfo?(james)
Attachment #9247338 - Attachment description: WIP: Bug 1736859 - [mochitest] Dump some prefs that are set in TestRunner.js → Bug 1736859 - [mochitest] Dump some prefs that are set in TestRunner.js, r?gbrown
Attachment #9247339 - Attachment description: WIP: Bug 1736859 - Explicitly disable fission in all test harnesses that use 'testing/profiles' → Bug 1736859 - [testing/profiles] Explicitly disable fission in all test harnesses, r?gbrown

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

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

Flags: needinfo?(james)

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

Flags: needinfo?(hskupin)
Flags: needinfo?(continuation)

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

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

Severity: -- → N/A
Fission Milestone: --- → Future

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.

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.

Pushed by ahalberstadt@mozilla.com: 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

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

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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.

Pushed by ahalberstadt@mozilla.com: 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
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Regressions: 1744581
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: