If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Run test harnesses with e10s enabled by default and provide an option to disable it

RESOLVED FIXED in Firefox 47

Status

Testing
Mochitest
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Gijs, Assigned: ahal)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox47 fixed, firefox48 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments, 1 obsolete attachment)

2.52 KB, patch
ahal
: review+
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
jmaher
: review+
Details | Review
58 bytes, text/x-review-board-request
jmaher
: review+
Details | Review
58 bytes, text/x-review-board-request
jmaher
: review+
Details | Review
58 bytes, text/x-review-board-request
ato
: review+
Details | Review
58 bytes, text/x-review-board-request
jgraham
: review+
Details | Review
58 bytes, text/x-review-board-request
jgraham
: review+
Details | Review
(Reporter)

Description

2 years ago
Nobody should really be adding tests that are disabled in e10s these days, so to avoid surprises when things hit infra and do get run in e10s, I would prefer to have locally run tests default to --e10s instead of having to opt in to it manually.

I believe at this stage this is already manageable when running individual tests. I'm less sure about the behaviour for directory filtering (ie when we're running more than 1 test) because right now you will get more tests to run when running without --e10s for many directories, and so you might miss failures when running with --e10s. Perhaps we could print a warning?

Updated

2 years ago
Blocks: 984139
tracking-e10s: --- → +
Hey ahal, how hard would it be to default to running our tests with e10s enabled?
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 2

2 years ago
I'd prefer to make this a global default (i.e both mach and automation), rather than a mach-only default. That way people can still copy/paste the commandline used in automation directly into their mach commands without getting unexpected results.

But even with that stipulation, it's not terribly difficult. Anywhere there is a "--e10s" flag, we'd need to change it to "--disable-e10s". Then we'd need to remove --e10s from the e10s jobs, and add --disable-e10s to the non-e10s jobs. For buildbot based jobs, that's still passed in via buildbot-configs:
https://dxr.mozilla.org/build-central/search?q=%22--e10s%22

For taskcluster, that's in-tree:
https://dxr.mozilla.org/mozilla-central/search?q=%22--e10s%22

As for non-e10s tests getting skipped, my preference is to not have different defaults depending on whether you're running a single test or a directory. I think that will be more confusing than helpful. I'd just print a warning and call it a day.
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 3

2 years ago
As an unfortunate aside of needing to update buildbot-configs, the order would need to be:
1. Add --disable-e10s
2. Update buildbot-configs
3. Remove --e10s

If we wanted a seamless bustage-free transition.
Created attachment 8724266 [details] [diff] [review]
Added --disable-e10s to mochitest_options.py and reftestcommandline.py
Attachment #8724266 - Flags: review?(ahalberstadt)

Updated

2 years ago
Assignee: nobody → stanleym2711
(Assignee)

Comment 5

2 years ago
Comment on attachment 8724266 [details] [diff] [review]
Added --disable-e10s to mochitest_options.py and reftestcommandline.py

Review of attachment 8724266 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, perfect!

You'll just need to make two changes to your patch metadata:
1. Append ", r=ahal" to the end of your commit message. This shows at a glance that I reviewed it.
2. Use your full name for the author. E.g "First Last <email>"

(see https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F for more info)

Please make the above changes, and re-upload the patch. After this point I'll get it landed and we can take a look at step 2 of this bug :).
Attachment #8724266 - Flags: review?(ahalberstadt) → review+
Created attachment 8725461 [details] [diff] [review]
Added --disable-e10s to mochitest_options.py and reftestcommandline.py

This should have the 2 suggested changes you advised me to make.
Attachment #8725461 - Flags: review?(ahalberstadt)
(Assignee)

Comment 7

2 years ago
Comment on attachment 8725461 [details] [diff] [review]
Added --disable-e10s to mochitest_options.py and reftestcommandline.py

Review of attachment 8725461 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent, thanks! I'll get this landed in a bit and we can start tackling the next step.

For the next part you'll need to clone http://hg.mozilla.org/build/buildbot-configs/ . And specifically you'll be working in this file:
http://hg.mozilla.org/build/buildbot-configs/file/tip/mozilla-tests/config.py

For every mochitest and reftest e10s job, we need to remove --e10s. For every mochitest and reftest non-e10s job, we need to add --disable-e10s. Feel free to give it a shot while I land this patch.
Attachment #8725461 - Flags: review?(ahalberstadt) → review+
(Assignee)

Updated

2 years ago
Attachment #8724266 - Attachment is obsolete: true

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae5567d429b0

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ae5567d429b0
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Comment 10

2 years ago
Forgot to set [leave-open].
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

2 years ago
Hey Stanley, were you still working on this? I'd like to get this landed soon. No worries if you don't have time, if I don't hear back from you soon I'll take it from here.
Flags: needinfo?(stanleym2711)
Hi Andrew, yes I am on this. I'll have a patch ready by end of tomorrow latest.
Flags: needinfo?(stanleym2711)
(Assignee)

Updated

2 years ago
Assignee: stanleym2711 → ahalberstadt

Updated

2 years ago
Depends on: 1259804
(Assignee)

Comment 13

2 years ago
Created attachment 8737251 [details]
MozReview Request: Bug 1243083 - Enable e10s by default when running mochitests, r?jmaher

Review commit: https://reviewboard.mozilla.org/r/43847/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43847/
Attachment #8737254 - Flags: review?(ato)
Attachment #8737255 - Flags: review?(james)
Attachment #8737256 - Flags: review?(james)
(Assignee)

Comment 14

2 years ago
Created attachment 8737252 [details]
MozReview Request: Bug 1243083 - Enable e10s by default when running reftests, r?jmaher

Review commit: https://reviewboard.mozilla.org/r/43849/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43849/
(Assignee)

Comment 15

2 years ago
Created attachment 8737253 [details]
MozReview Request: Bug 1243083 - Enable e10s by default when running talos, r?jmaher

Review commit: https://reviewboard.mozilla.org/r/43851/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43851/
(Assignee)

Comment 16

2 years ago
Created attachment 8737254 [details]
MozReview Request: Bug 1243083 - Enable e10s by default when running marionette tests, r?ato

Review commit: https://reviewboard.mozilla.org/r/43853/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43853/
(Assignee)

Comment 17

2 years ago
Created attachment 8737255 [details]
MozReview Request: Bug 1243083 - Enable e10s by default when running web-platform-tests, r?jgraham

Review commit: https://reviewboard.mozilla.org/r/43855/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43855/
(Assignee)

Comment 18

2 years ago
Created attachment 8737256 [details]
MozReview Request: Bug 1243083 - Enable e10s by default when using chunk-finder, r?jgraham

Review commit: https://reviewboard.mozilla.org/r/43857/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43857/
(Assignee)

Comment 19

2 years ago
Here's a try run with all the above squashed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8f2c76bc189ca7f50208a71aaa7ebc832ab1ad6&group_state=expanded
Summary: ./mach mochitest foo/bar/baz.js should default to running with --e10s, and we should support --disable-e10s or --e10s=false or similar to turn it off → Run test harnesses with e10s enabled by default and provide an option to disable it
(Assignee)

Comment 20

2 years ago
Mozreview wouldn't let me flag jmaher as he is pto.. I'll flag him Monday.
Attachment #8737254 - Flags: review?(ato) → review+
Comment on attachment 8737254 [details]
MozReview Request: Bug 1243083 - Enable e10s by default when running marionette tests, r?ato

https://reviewboard.mozilla.org/r/43853/#review40391
Comment on attachment 8737256 [details]
MozReview Request: Bug 1243083 - Enable e10s by default when using chunk-finder, r?jgraham

https://reviewboard.mozilla.org/r/43857/#review40415

::: testing/mach_commands.py:721
(Diff revision 1)
>                          dest='chunk_by_dir',
>                          help='Group tests together in the same chunk that are in the same top '
>                               'chunkByDir directories.',
>                          default=None)
>  
> -    parser.add_argument('--e10s',
> +    parser.add_argument('--disable-e10s',

This is fine, but the commit message talking about |mach test| seems confused (or I am); afaict this is for the chunk finder?
Attachment #8737256 - Flags: review?(james)
Comment on attachment 8737255 [details]
MozReview Request: Bug 1243083 - Enable e10s by default when running web-platform-tests, r?jgraham

https://reviewboard.mozilla.org/r/43855/#review40399

::: testing/web-platform/harness/wptrunner/wptcommandline.py:162
(Diff revision 1)
>                          help="Path to host certificate when using pregenerated ssl certificates")
>  
>      gecko_group = parser.add_argument_group("Gecko-specific")
>      gecko_group.add_argument("--prefs-root", dest="prefs_root", action="store", type=abs_path,
>                               help="Path to the folder containing browser prefs")
> -    gecko_group.add_argument("--e10s", dest="gecko_e10s", action="store_true",
> +    gecko_group.add_argument("--disable-e10s", dest="gecko_e10s", action="store_false", default=True,

Yes, but you need to make this change upstream and then update from upstream, or it will get lost.
Attachment #8737255 - Flags: review?(james)
(Assignee)

Updated

2 years ago
Attachment #8737251 - Flags: review?(jmaher)
Attachment #8737252 - Flags: review?(jmaher)
Attachment #8737253 - Flags: review?(jmaher)
(Assignee)

Comment 24

2 years ago
https://reviewboard.mozilla.org/r/43857/#review40415

> This is fine, but the commit message talking about |mach test| seems confused (or I am); afaict this is for the chunk finder?

Ah yes, it is me that is confused. I'll update the commit message.
(Assignee)

Updated

2 years ago
Blocks: 1261823
Comment on attachment 8737251 [details]
MozReview Request: Bug 1243083 - Enable e10s by default when running mochitests, r?jmaher

https://reviewboard.mozilla.org/r/43847/#review40667

one question, I really need some clarification on store_false vs store_true and how this is intended to work.

::: testing/mochitest/mochitest_options.py:393
(Diff revision 1)
> -          "default": False,
> -          "help": "Run tests with electrolysis preferences and test filtering enabled.",
> -          }],
>          [["--disable-e10s"],
>           {"action": "store_false",
> -          "default": False,
> +          "default": True,

I am slightly confused by this, does --disable-e10s, store_false, default=True mean that we will always disable-e10s?  if we pass in the cli flag, will it be false?
Attachment #8737251 - Flags: review?(jmaher)
(Assignee)

Comment 26

2 years ago
https://reviewboard.mozilla.org/r/43847/#review40667

> I am slightly confused by this, does --disable-e10s, store_false, default=True mean that we will always disable-e10s?  if we pass in the cli flag, will it be false?

The key thing to notice is the "dest": "e10s". It's set up this way so the rest of the code doesn't need to change, e.g stuff that uses options.e10s. So by default, options.e10s will be True (e10s is enabled). And if we pass in --disable-e10s (which stores False), options.e10s will be False (e10s disabled).
Comment on attachment 8737253 [details]
MozReview Request: Bug 1243083 - Enable e10s by default when running talos, r?jmaher

https://reviewboard.mozilla.org/r/43851/#review40685
Attachment #8737253 - Flags: review?(jmaher) → review+
Comment on attachment 8737252 [details]
MozReview Request: Bug 1243083 - Enable e10s by default when running reftests, r?jmaher

https://reviewboard.mozilla.org/r/43849/#review40687
Attachment #8737252 - Flags: review?(jmaher) → review+
Comment on attachment 8737251 [details]
MozReview Request: Bug 1243083 - Enable e10s by default when running mochitests, r?jmaher

https://reviewboard.mozilla.org/r/43847/#review40691
Attachment #8737251 - Flags: review+
(Assignee)

Comment 30

2 years ago
https://reviewboard.mozilla.org/r/43855/#review40399

> Yes, but you need to make this change upstream and then update from upstream, or it will get lost.

Right, I always forget. Mind if I land this change both here and upstream rather than doing a sync?
https://reviewboard.mozilla.org/r/43855/#review40399

> Right, I always forget. Mind if I land this change both here and upstream rather than doing a sync?

That's fine; I will r+ here once it is landed upstream.
(Assignee)

Comment 32

2 years ago
https://reviewboard.mozilla.org/r/43855/#review40399

> That's fine; I will r+ here once it is landed upstream.

Cool, here's the pull request:
https://github.com/w3c/wptrunner/pull/180

Just keep in mind that if this gets synced over before the mozharness changes have a chance to land, it will cause problems.
Comment on attachment 8737255 [details]
MozReview Request: Bug 1243083 - Enable e10s by default when running web-platform-tests, r?jgraham

https://reviewboard.mozilla.org/r/43855/#review40719
Attachment #8737255 - Flags: review+
(Assignee)

Comment 34

2 years ago
Comment on attachment 8737256 [details]
MozReview Request: Bug 1243083 - Enable e10s by default when using chunk-finder, r?jgraham

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43857/diff/1-2/
Attachment #8737256 - Attachment description: MozReview Request: Bug 1243083 - Enable e10s by default when using |mach test| locally, r?jgraham → MozReview Request: Bug 1243083 - Enable e10s by default when using chunk-finder, r?jgraham
Attachment #8737256 - Flags: review?(james)
Comment on attachment 8737256 [details]
MozReview Request: Bug 1243083 - Enable e10s by default when using chunk-finder, r?jgraham

https://reviewboard.mozilla.org/r/43857/#review40735
Attachment #8737256 - Flags: review?(james) → review+

Comment 36

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a470bcc2b2be
https://hg.mozilla.org/integration/mozilla-inbound/rev/108a50bd1e2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f32c09cf6b23
https://hg.mozilla.org/integration/mozilla-inbound/rev/940686f9541f
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7404a8b0bea
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cdaa1592a36

Comment 37

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a470bcc2b2be
https://hg.mozilla.org/mozilla-central/rev/108a50bd1e2a
https://hg.mozilla.org/mozilla-central/rev/f32c09cf6b23
https://hg.mozilla.org/mozilla-central/rev/940686f9541f
https://hg.mozilla.org/mozilla-central/rev/d7404a8b0bea
https://hg.mozilla.org/mozilla-central/rev/5cdaa1592a36
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Depends on: 1254029
Blocks: 1263092
Depends on: 1263646
No longer blocks: 1263092
Depends on: 1263092
(Assignee)

Updated

a year ago
Depends on: 1274002
You need to log in before you can comment on or make changes to this bug.