Closed Bug 1470266 Opened 2 years ago Closed Last year

setup a sw-e10s automation branch to run tests with pref flipped

Categories

(Core :: DOM: Service Workers, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: bkelly, Assigned: ahal)

References

(Blocks 1 open bug)

Details

(Whiteboard: SW-MUST[wptsync upstream])

Attachments

(8 files)

46 bytes, text/x-phabricator-request
jmaher
: review+
Details | Review
46 bytes, text/x-phabricator-request
jmaher
: review+
Details | Review
46 bytes, text/x-phabricator-request
jmaher
: review+
Details | Review
46 bytes, text/x-phabricator-request
jmaher
: review+
Details | Review
46 bytes, text/x-phabricator-request
jmaher
: review+
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
We are getting closer to having the service worker e10s redesign complete.  While not ready to enable yet, many tests are green right now.  Ultimately we will probably have the pref enabled on nightly for a release or so while things are polished, but still need to let the legacy mode ride the trains.

To avoid regressions we will likely need a separate branch to run tests in whatever mode nightly is not running.  So to start it would be a branch with the pref flipped on.  After we enable on nightly it would be a branch with the pref flipped off.

The reason we probably need a branch here is that service workers touch a huge amount of code:

* Network
* Document loading
* Devtools
* Its own complex state machine

It is easy to regress whatever mode is not enabled by default on nightly.  We want to catch this before the regression hits beta branch.

Joel, how do we make this happen?
Flags: needinfo?(jmaher)
a few options come to mind:
1) run this in parallel on m-c only (downsides are regressions are harder to track down)
2) run this on a single platform (i.e. not OSX) per commit and let SETA help manage the load

I assume the test suites would be:
* xpcshell (a lot of networking)
* mochitest-browser-chrome-devtools
* wpt || mochitest (document loading)

would we need everything?  perf tests?

To get something in parallel we need to do a couple things:
1) edit the .yml files that define a test https://searchfox.org/mozilla-central/source/taskcluster/ci/test/mochitest.yml#210 and make a copy of the suite you want, then add a mozharness option for the pref you want.
2) pick which platforms you want to run this on: https://searchfox.org/mozilla-central/source/taskcluster/ci/test/test-sets.yml#366

I am sure I am overlooking some details, but this should get the conversation moving in the right direction.
Flags: needinfo?(jmaher)
I guess I was thinking something similar to how stylo was done.  Probably linux 32-bit debug would be a reasonable place to start since it runs e10s and non-e10s tests.  (While the pref makes us handle e10s better, it also changes how we work in non-e10s.)

I don't think we could just change normal test running for that platform to use the new pref yet, though.  There is going to be a period where that mode is mostly green, but still has a bit of orange while things get polished.  For example, we will probably have some broken devtools tests for a while since they need to be able to work with the final implementation to know how to adjust devtools to fix them.
Whiteboard: SW-MUST
Priority: -- → P2
the request here was for sw-e10s, so do we need to duplicate e10s and non-e10s tests?  as soon as we are running geckoview in packet.net with e10s and web-platform-tests on fennec/geckoview, I see us turning off non-e10s tests for linux- that is still at least a month out.

Do we need this on opt/debug?  I would really focus on linux64, as linux32 as a platform has fewer tests in general than linux64.
I can't speak to future plans to migrate tier 1 platforms to e10s.  If that is really happening in the near term, then great!

If mozilla is going to be shipping a non-e10s tier 1 platform, though, then I think its important to run tests for both e10s and non-e10s.  This code will be following a different path when the sw pref is flipped whether the browser is in e10s or non-e10s.  There could be regressions in either case.

Anyway, I just wanted to get the ball rolling here.  I'll leave the specifics to Blake, Andrew, and Marion.
asuth: I think this may be worthwhile to setup
Flags: needinfo?(bugmail)
(In reply to Joel Maher ( :jmaher ) (UTC+2) from comment #3)
> the request here was for sw-e10s, so do we need to duplicate e10s and
> non-e10s tests?  as soon as we are running geckoview in packet.net with e10s
> and web-platform-tests on fennec/geckoview, I see us turning off non-e10s
> tests for linux- that is still at least a month out.

Re-stating what Ben said here: Yes.  We need test coverage for both e10s and non-e10s for both states of the "dom.serviceWorkers.parent_intercept" boolean preference.  The preference switches between two radically different implementations, and each implementation behaves radically differently for e10s and non-e10s.

> Do we need this on opt/debug?  I would really focus on linux64, as linux32
> as a platform has fewer tests in general than linux64.

We want debug for assertion checking.  We don't really need opt at this time. linux64 is fine.  Thanks!
Flags: needinfo?(bugmail) → needinfo?(jmaher)
thanks for clarifying this.

So linux64 unittests with tests in both e10s/non-e10s (where applicable) and having this preference set:
dom.serviceWorkers.parent_intercept

to value:
??

this will be on mozilla-central only, or all integration branches?
Flags: needinfo?(jmaher) → needinfo?(bugmail)
(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #7)
> thanks for clarifying this.
> 
> So linux64 unittests with tests in both e10s/non-e10s (where applicable) and
> having this preference set:
> dom.serviceWorkers.parent_intercept
> 
> to value:
> ??

Currently: true

(And then we'll flip this in bug 1456995.  When that lands, setting the pref to `true` to nightly, we'll set it to `false` for this set of tests.)

> this will be on mozilla-central only, or all integration branches?

Just mozilla-central.
Flags: needinfo?(bugmail)
:ahal can you put this bug on your list to work on this month?
Flags: needinfo?(ahal)
Sure, I'll keep the needinfo for now as a reminder.

Aside (which is totally out of scope for this bug): we should really make "e10s" the default configuration and remove all those e10s labels from our treeherder symbols.
Assignee: nobody → ahal
Status: NEW → ASSIGNED
Flags: needinfo?(ahal)
Just noting that enabling this for mochitest, reftest and web-platform (desktop only), we add ~1800 new tasks. This will be higher if we include mobile/additional suites.

Andrew, is there a code name (like e10s) that developers will recognize this change by? I'm looking for something better than "serviceworker redesign" for things like cli flags and task descriptions.
Flags: needinfo?(bugmail)
(to clarify, I'm already using -sw for task symbols/labels)
So, we've been calling this "ServiceWorkers-e10s" which is what the meta-bug Bug 1231208's alias is.  And our cool wiki tracking board is similarly named at https://wiki.mozilla.org/DOM/Workers-Storage/projects/e10s-sw.

If you think "sw-e10s" is too ambiguous then "ServiceWorker Parent Intercept" is an also appropriate if somewhat misleading term that also happens to be what we named the preference, so that works out well.  And then "sw-pi" or "swpi" maybe works when you need to distill that down.  "-sw" seems great as a task symbol/label suffix.


In terms of 1800 new tasks, is that after limiting this to only linux64 debug?
Flags: needinfo?(bugmail)
(In reply to Andrew Sutherland [:asuth] from comment #13)
> So, we've been calling this "ServiceWorkers-e10s" which is what the meta-bug
> Bug 1231208's alias is.  And our cool wiki tracking board is similarly named
> at https://wiki.mozilla.org/DOM/Workers-Storage/projects/e10s-sw.
> 
> If you think "sw-e10s" is too ambiguous then "ServiceWorker Parent
> Intercept" is an also appropriate if somewhat misleading term that also
> happens to be what we named the preference, so that works out well.  And
> then "sw-pi" or "swpi" maybe works when you need to distill that down. 
> "-sw" seems great as a task symbol/label suffix.

Thanks, those should cover all the bases.

> In terms of 1800 new tasks, is that after limiting this to only linux64
> debug?

Oops, missed that spec. Yeah that'll cut it down a lot more.
Whiteboard: SW-MUST → SW-MUST, [leave-open]
I almost forgot to update the regexes in moztest.resolve when creating the -sw
variant of task. This adds a test to make sure we don't forget more things in
the future.
These regexes are used for things like determining which tasks to run given a
"path" int |mach try|. Previously, we used patterns like:

mochitest-chrome-(?:e10s)?(?:-1)?$

This would match both e10s and non-e10s versions of a task with either no
chunks, or only selecting chunk 1. But we keep adding other configurations, e.g
-gpu, -no-accel, -sw, etc.  Each time we create a new possibility we need to
remember to update these task regexes (or else lose test coverage when using
paths with |mach try|).

Instead of individually listing every possibility, let's use a pattern like
this:

mochitest-chrome($|.*(-1|[^0-9])$)

This also selects tasks that are either chunk 1 or don't have any chunks.  But
it allows for arbitrary strings in-between. This regex doesn't need to be
updated when we add configurations like -sw.

Depends on D7119
Comment on attachment 9012689 [details]
Bug 1470266 - [moztest.resolve] Create a unittest for the task regexes, r?jmaher

Joel Maher ( :jmaher ) (UTC-4) has approved the revision.
Attachment #9012689 - Flags: review+
Comment on attachment 9012690 [details]
Bug 1470266 - [moztest.resolve] Make task regexes more resilient to change, r?jmaher

Joel Maher ( :jmaher ) (UTC-4) has approved the revision.
Attachment #9012690 - Flags: review+
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67da8157c1d6
[moztest.resolve] Create a unittest for the task regexes, r=jmaher
https://hg.mozilla.org/integration/autoland/rev/3de6b966a246
[moztest.resolve] Make task regexes more resilient to change, r=jmaher
Here's an initial try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f45e0f532b0a589e7cf0d1604966225c1c98f88

Still have a few things left to implement:
1. Adding an 'sw' key to the manifests so we can skip tests based on this mode
2. Adding logging to the various runners so we can tell if it's enabled by inspecting a log

Note, I decided to implement this by passing in --setpref (rather than creating a custom argument for it). Though I can definitely create a custom cli argument if desired. I therefore skipped some harnesses that don't have the ability to accpet prefs on the command line yet, like xpcshell, marionette, raptor and compiled tests. If some or all of these are important to have, I'd propose implementing them in a follow-up.

Finally, do we care about talos for this configuration yet?
Looks like there are a few serviceworker test failures in that try run that'll need to be fixed or disabled.
(In reply to Andrew Halberstadt [:ahal] from comment #20)
> I therefore skipped some harnesses that don't have
> the ability to accpet prefs on the command line yet, like xpcshell,
> marionette, raptor and compiled tests. If some or all of these are important
> to have, I'd propose implementing them in a follow-up.

We can get by without xpcshell tests initially.  We're probably fine without marionette/raptor/compiled tests for now.  But there are a ton of xpcshell tests under netwerk/test/unit that are potentially relevant and whose breakage would be _very bad_, so it'd want to be a fairly prompt follow-up.

> Finally, do we care about talos for this configuration yet?
 
I think we can handle talos via try pushes for our performance-related needs.
The mozharness scripts have a lot of special case arguments for one off
configurations, stuff like --e10s, --enable-webrender and --gpu-required.

Many of these command line args ultimately only end up setting an extra pref in
the test harnesses. Instead, let's just give mozharness the ability to set
prefs directly via --setpref. This way we can pass them through from taskgraph
without needing to add extra configuration to mozharness when making changes
like this.
(In reply to Andrew Sutherland [:asuth] from comment #22)
> We can get by without xpcshell tests initially.  We're probably fine without
> marionette/raptor/compiled tests for now.  But there are a ton of xpcshell
> tests under netwerk/test/unit that are potentially relevant and whose
> breakage would be _very bad_, so it'd want to be a fairly prompt follow-up.

Ok, I'll handle it in this bug, though it might not land at the same time as the rest (might as well land what's finished).
Btw it looks like we don't run xpcshell with e10s *anywhere* at the moment:
https://searchfox.org/mozilla-central/source/taskcluster/ci/test/xpcshell.yml#58

So that might lower the priority of getting them running (but I'll tackle it nonetheless).
(In reply to Andrew Halberstadt [:ahal] from comment #25)
> Btw it looks like we don't run xpcshell with e10s *anywhere* at the moment:
> https://searchfox.org/mozilla-central/source/taskcluster/ci/test/xpcshell.
> yml#58

The xpcshell tests do this themselves via run_test_in_child(...) at https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/testing/xpcshell/head.js#1225

Here's a particularly important example netwerk test doing it for example:
https://searchfox.org/mozilla-central/source/netwerk/test/unit_ipc/test_synthesized_response_wrap.js

Other components do it slightly different ways; for example, indexedDB does some trickery with this manifest:
https://searchfox.org/mozilla-central/source/dom/indexedDB/test/unit/xpcshell-child-process.ini
and this helper logic:
https://searchfox.org/mozilla-central/source/dom/indexedDB/test/unit/xpcshell-head-child-process.js
Comment on attachment 9012928 [details]
Bug 1470266 - [mozharness] Add --setpref argument to main desktop scripts, r?jmaher

Joel Maher ( :jmaher ) (UTC-4) has approved the revision.
Attachment #9012928 - Flags: review+
Keywords: leave-open
Whiteboard: SW-MUST, [leave-open] → SW-MUST
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f251a29c5846
[mozharness] Add --setpref argument to main desktop scripts, r=jmaher
From my earlier try run, looks like reftest doesn't have any problems so I'm going to get that landed while I attempt to green up the other suites.
This adds a transform which will evaluate the 'serviceworker-e10s' key. If
False, the task is returned as is. If True, the task is run with the
'dom.serviceWorkers.parent_intercept' pref enabled. If 'both' the task is split
into two tasks, one with the pref, and one without.
This duplicates all the reftest tasks except with
dom.serviceWorkers.parent_intercept set to true. For now they are only run on
mozilla-central with linux64/debug.

Depends on D7480
Comment on attachment 9013705 [details]
Bug 1470266 - [taskgraph] Add ability to schedule serviceworker-e10s test tasks in taskgraph, r?jmaher

Joel Maher ( :jmaher ) (UTC-4) has approved the revision.
Attachment #9013705 - Flags: review+
Comment on attachment 9013706 [details]
Bug 1470266 - [ci] Schedule serviceworker-e10s reftest tasks with linux64/debug on mozilla-central, r?jmaher

Joel Maher ( :jmaher ) (UTC-4) has approved the revision.
Attachment #9013706 - Flags: review+
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e6405e7631c
[taskgraph] Add ability to schedule serviceworker-e10s test tasks in taskgraph, r=jmaher
https://hg.mozilla.org/integration/autoland/rev/6eec41ce0c94
[ci] Schedule serviceworker-e10s reftest tasks with linux64/debug on mozilla-central, r=jmaher
This duplicates all the mochitest, based tests except with
dom.serviceWorkers.parent_intercept set to true. For now they are only run on
mozilla-central with linux64/debug.
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/423ba8ffc8d7
[ci] Schedule serviceworker-e10s mochitest tasks with linux64/debug on mozilla-central, r=jmaher
Andrew, Ben, I ended up needing to disable the tests in dom/serviceworkers/test/mochitest.ini, which is obviously not ideal. But it was causing sporadic test timeouts and memory leaks so figured I'd get most of the tests going and leave it to you guys to re-enable that directory later. There were some other misc tests disabled as well, you'll be able to see them all at this link (once the latest commit merges and searchfox updates):
https://searchfox.org/mozilla-central/search?q=serviceworker_e10s&case=true&path=ini%24

I'll work on wpt next, and finish off with xpcshell (which will be the harder).
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf17ad5b022f
[ci] Schedule serviceworker-e10s mochitest tasks with linux64/debug on mozilla-central, r=jmaher
Flags: needinfo?(ahal)
Backed out for breaking test-verify.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success,testfailed&fromchange=bf17ad5b022fd82b89695a945a687b1a6aef5f7d&tochange=28cbada91897aad669cee5b8780bcda5eb081a06&searchStr=tv&selectedJob=203667449

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=203667449&repo=autoland&lineNumber=1662

Backout link: https://hg.mozilla.org/integration/autoland/rev/28cbada91897aad669cee5b8780bcda5eb081a06

[task 2018-10-05T15:22:28.927Z] 15:22:28     INFO -      options.extraPrefs = self.parseExtraPrefs(options.extraPrefs)
[task 2018-10-05T15:22:28.928Z] 15:22:28     INFO -    File "/builds/worker/workspace/build/tests/mochitest/runtests.py", line 915, in parseExtraPrefs
[task 2018-10-05T15:22:28.929Z] 15:22:28     INFO -      prefs = dict(parseKeyValue(prefs, context='--setpref='))
[task 2018-10-05T15:22:28.929Z] 15:22:28     INFO -    File "/builds/worker/workspace/build/tests/mochitest/runtests.py", line 804, in parseKeyValue
[task 2018-10-05T15:22:28.930Z] 15:22:28     INFO -      (context, ','.join(missing)), errors=missing)
[task 2018-10-05T15:22:28.931Z] 15:22:28     INFO -  TypeError: not all arguments converted during string formatting
[task 2018-10-05T15:22:28.975Z] 15:22:28    ERROR - Return code: 1
[task 2018-10-05T15:22:28.975Z] 15:22:28     INFO - TinderboxPrint: mochitest-browser-chrome<br/>11/0/0
[task 2018-10-05T15:22:28.975Z] 15:22:28    ERROR - # TBPL FAILURE #
[task 2018-10-05T15:22:28.976Z] 15:22:28  WARNING - setting return code to 2
[task 2018-10-05T15:22:28.976Z] 15:22:28    ERROR - TinderboxPrint: Per-test run of .../browser_ext_tabs_highlight.js<br/>: FAILURE
Flags: needinfo?(ahal)
QA Contact: mdaly
Flags: needinfo?(ahal)
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3d2029e7b26
[ci] Schedule serviceworker-e10s mochitest tasks with linux64/debug on mozilla-central, r=jmaher
Hey Ben, just want to make sure someone is aware that dom/serviceworkers/test/mochitest.ini had to be disabled to get the mochitests running (due to frequent timeouts and memory leaks). You can see the full set of disabled mochitests here:
https://searchfox.org/mozilla-central/search?q=serviceworker_e10s&case=true&path=ini%24

I'm not going to spend time greening these up, so please pass this on to whoever needs to know. Thanks!
Flags: needinfo?(ben)
301 :asuth
Flags: needinfo?(ben) → needinfo?(bugmail)
I started looking into WPT, but it makes bug 1400716 permafail. That bug isn't associated with any particular test, so I guess we'll have to fix it before we can enable wpt. At least we have a way to reproduce it now.
Depends on: 1400716
No longer depends on: 1400716
On closer inspection I don't think I'm hitting bug 1400716 (both issues just happen to have the same failure summary string)
This duplicates all web-platform-test mozharness based tests except with
dom.serviceWorkers.parent_intercept set to true.
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c77616575b61
[ci] Schedule serviceworker-e10s web-platform-test tasks with linux64/debug on mozilla-central, r=jgraham,jmaher
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13577 for changes under testing/web-platform/tests
Whiteboard: SW-MUST → SW-MUST[wptsync upstream]
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Depends on: 1460914
Keywords: leave-open
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67ff9b8c78bc
[ci] Schedule serviceworker-e10s xpcshell tasks with linux64/debug on mozilla-central, r=jmaher
https://hg.mozilla.org/mozilla-central/rev/67ff9b8c78bc
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Why is this not running on try or autoland? I was just backed out of central, which seems like a bug to me (it wasted a lot of my time and delayed an important landing).
Flags: needinfo?(ahal)
I don't think you're supposed to get backed out if you make these builds orange?
Flags: needinfo?(bugmail)
(In reply to Johann Hofmann [:johannh] from comment #58)
> Why is this not running on try or autoland? I was just backed out of
> central, which seems like a bug to me (it wasted a lot of my time and
> delayed an important landing).

I don't really have a stake in this besides being the one to implement the configuration changes, I made it central-only based on comment 8.


(In reply to Andrew Sutherland [:asuth] from comment #59)
> I don't think you're supposed to get backed out if you make these builds
> orange?

Tier 2 means that bustage can be tolerated for short periods of time (hours or days if the owner of the task says it's alright and fix is being worked on). Sometimes if a sheriff can't find anyone to clarify, they'll just back out to be safe. But typically tier-2 issues still need to be fixed in a reasonable time frame.

Tier 3 is the one where bustage doesn't get backed out (and it's invisible by default).
Flags: needinfo?(ahal)
Actually, looking at central these are tier 1 anyway. I agree with Johann they should either run on the integration branches or be bumped to tier 3.
See Also: → 1577912
You need to log in before you can comment on or make changes to this bug.