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

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[:Gijs (he/him)]

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?

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey ahal, how hard would it be to default to running our tests with e10s enabled?

Comment 2

[Andrew Halberstadt [:ahal]]

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.

Comment 3

[Andrew Halberstadt [:ahal]]

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.

Comment 4

[Stanley Mwangi [:smwangi]]

Attachment: Added --disable-e10s to mochitest_options.py and reftestcommandline.py

Comment 5

[Andrew Halberstadt [:ahal]]

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

Comment 6

[Stanley Mwangi [:smwangi]]

Attachment: Added --disable-e10s to mochitest_options.py and reftestcommandline.py

This should have the 2 suggested changes you advised me to make.

Comment 7

[Andrew Halberstadt [:ahal]]

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.

Comment 8

[Pulsebot]

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

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Andrew Halberstadt [:ahal]]

Forgot to set [leave-open].

Comment 11

[Andrew Halberstadt [:ahal]]

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.

Comment 12

[Stanley Mwangi [:smwangi]]

Hi Andrew, yes I am on this. I'll have a patch ready by end of tomorrow latest.

Comment 13

[Andrew Halberstadt [:ahal]]

Attachment: 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/

Comment 14

[Andrew Halberstadt [:ahal]]

Attachment: 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/

Comment 15

[Andrew Halberstadt [:ahal]]

Attachment: 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/

Comment 16

[Andrew Halberstadt [:ahal]]

Attachment: 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/

Comment 17

[Andrew Halberstadt [:ahal]]

Attachment: 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/

Comment 18

[Andrew Halberstadt [:ahal]]

Attachment: 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/

Comment 19

[Andrew Halberstadt [:ahal]]

Here's a try run with all the above squashed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8f2c76bc189ca7f50208a71aaa7ebc832ab1ad6&group_state=expanded

Comment 20

[Andrew Halberstadt [:ahal]]

Mozreview wouldn't let me flag jmaher as he is pto.. I'll flag him Monday.

Comment 21

[Andreas Tolfsen ❲:ato❳]

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 22

[James Graham [:jgraham]]

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?

Comment 23

[James Graham [:jgraham]]

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.

Comment 24

[Andrew Halberstadt [:ahal]]

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.

Comment 25

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

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?

Comment 26

[Andrew Halberstadt [:ahal]]

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 27

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

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

Comment 28

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

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

Comment 29

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

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

Comment 30

[Andrew Halberstadt [:ahal]]

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?

Comment 31

[James Graham [:jgraham]]

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.

Comment 32

[Andrew Halberstadt [:ahal]]

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 33

[James Graham [:jgraham]]

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

Comment 34

[Andrew Halberstadt [:ahal]]

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/

Comment 35

[James Graham [:jgraham]]

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

Comment 36

[Pulsebot]

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

[Carsten Book [:Tomcat]]

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