1603078 - Run remote(pup) with Fission enabled

Status: RESOLVED FIXED

(Remote Protocol :: Agent, task, P1)

Description

[Andreas Tolfsen ❲:ato❳]

We should run the Puppeteer test suite (known on Treeherder as remote(pup)) with Fission enabled so we can track progress towards remote-fission.

This should just be a matter of adding a flag to ./mach puppeteer-test that sets the fission.autostart preference, and create a new task entry in taskcluster/ci/source-test/remote.yml.

Comment 1

[Chris Peterson [:cpeterson]]

Tracking for Fission Nightly (M6)

Comment 2

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1603078: remote: fix passing extra prefs to Puppeteer

Puppeteer accepts an environmental variable EXTRA_LAUNCH_OPTIONS
that is a JSON encoded object as a string, containing additional
configuration to use when running tests.

This takes an extraPrefsFirefox key which is an object
mapping preference names to values. This is extracted in
remote/test/puppeteer/lib/Launcher.js:390 and later written to a
fresh profile in :515.

It appears remote/mach_commands.py has left out the "Firefox" appendix.

Comment 3

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1603078: remote: add --enable-fission to "./mach puppeteer-test"

The "./mach puppeteer-test" command now takes a new flag,
--enable-fission, which will set the fission.autostart preference to true.

Comment 4

[Andreas Tolfsen ❲:ato❳]

This changeset is not complete yet, but I’ve submitted a couple of patches to get us started.

Comment 5

[Andreas Tolfsen ❲:ato❳]

You did some refactoring of how Fission test jobs are configured for TaskCluster recently. What do I have to do to set up a Fission variant for the remote(pup) job?

As you can see from D58512, I’ve already added an --enable-fission flag to ./mach puppeteer-test. I assume I need to include that flag somewhere?

Comment 6

[Andrew Halberstadt [:ahal]]

Is remote(pup) a test task? If so you'll need to make sure your task:
A) Has a variants key that includes "fission"
B) Sets fission-run-on-projects (this is the same as run-on-projects but only applies to fission tasks)

Enabling the fission variant will pass:

            'mozharness': {
                'extra-options': ['--setpref=fission.autostart=true',
                                  '--setpref=dom.serviceWorkers.parent_intercept=true',
                                  '--setpref=browser.tabs.documentchannel=true'],
            },

Note this goes into mozharness not the mach command. If your harness supports --setpref I'd recommend making --enable-fission a simple alias to the above, and then updating your internal logic to look for the fission.autostart pref to determine whether or not it is running with fission.

If your harness doesn't support --setpref or it is not a test kind, you're off the beaten path and will need to figure out some other way of hooking it all up properly.

Comment 7

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

Andrew, the puppeteer tests are source-tests, which simply fetch the binary via a fetch task. So I assume the alias thing would somewhat work then?

Comment 8

[Andrew Halberstadt [:ahal]]

Yeah, though that's more for the harness side of things.

In the taskgraph side, you'll need to decide how to define fission or not. E.g, you could simply duplicate the entire task in the .yml files, or you could implement a fission: true key in the task definition that you then read from a transform that splits the task into two (one fission and one non-fission). If it's just a single task, might be easiest to just duplicate the whole thing in the configs and tweak the necessary bits.

Comment 9

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1603078: taskcluster: add remote(pup-fis) job;

Because the Puppeteer unit tests run as source tasks and not test
tasks, we can't rely on the generic technique for enabling Fission
available to other test tasks based on mozharness.

Since the puppeteer task is a simple, opt-in, non-scheduled job we
duplicate it as puppeteer-fis and pass --enable-fission to "./mach
puppeteer-test" instead. This achieves the same effect.

Comment 10

[Andreas Tolfsen ❲:ato❳]

Just a quick follow-up question:

Are the dom.serviceWorkers.parent_intercept and browser.tabs.documentchannel preferences important to us?

Comment 11

[Andreas Tolfsen ❲:ato❳]

For the record I’ve filed bug 1606812 to employ mozharness for the Puppeteer tests. Not considering this a blocker, but it is something we should look into at some point.

Comment 12

[Andrew Halberstadt [:ahal]]

I have no idea, 301 :kmag.

Comment 13

[Chris Peterson [:cpeterson]]

(In reply to Andreas Tolfsen ❲:ato❳ from comment #10)

Are the dom.serviceWorkers.parent_intercept and browser.tabs.documentchannel preferences important to us?

I think the plan is to enabled these prefs by default and ride the trains in 73.

@ Matt Woodrow, who is working on Document Channel and can answer definitively.

Comment 14

[Matt Woodrow (:mattwoodrow)]

(In reply to Chris Peterson [:cpeterson] from comment #13)

(In reply to Andreas Tolfsen ❲:ato❳ from comment #10)

Are the dom.serviceWorkers.parent_intercept and browser.tabs.documentchannel preferences important to us?

I think the plan is to enabled these prefs by default and ride the trains in 73.

@ Matt Woodrow, who is working on Document Channel and can answer definitively.

Both of these prefs are going to be required for fission to work properly, the latter will almost certainly affect these tests.

They both are true by default on Nightly, and the plan is for them to ride with 73. That said, there's probably no harm in explicitly setting them (like we do for other --enable-fission options), and we can remove them all at the same time.

Comment 15

[Andreas Tolfsen ❲:ato❳]

The remote agent only ships on Nightly which would mean that these preferences will become defaults before we start shipping on other release channels.

However, as Matt says there is probably little harm in setting them explicitly. This would allow us to run tests more reliably against other release branches if we would wish.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44d98b5ee706
remote: fix passing extra prefs to Puppeteer r=remote-protocol-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/74eea3cb0eac
remote: add --enable-fission to "./mach puppeteer-test" r=remote-protocol-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/54b9d2b12988
taskcluster: add remote(pup-fis) job; r=ahal

Comment 17

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/44d98b5ee706
https://hg.mozilla.org/mozilla-central/rev/74eea3cb0eac
https://hg.mozilla.org/mozilla-central/rev/54b9d2b12988

Comment 18

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

With Fission enabled we only run 267 out of 638 tests. In the case below it is due to navigation times out for Firefox Browser Page Page.waitForNavigation should work with history.pushState() (navigation.spec.js:374:5).

Ran 267 (ok - 56, failed - 177, skipped - 34) of 638 test(s)

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=284595705&repo=mozilla-central&lineNumber=13422

I don't think we should investigate that right now until navigation is making use of the JSWindowActor implementation (bug 1565162).