Closed Bug 1243083 Opened 4 years ago Closed 4 years ago

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

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(e10s+, firefox47 fixed, firefox48 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
e10s + ---
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: Gijs, Assigned: ahal)

References

(Blocks 2 open bugs)

Details

Attachments

(7 files, 1 obsolete file)

2.52 KB, patch
ahal
: review+
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
jmaher
: review+
Details
58 bytes, text/x-review-board-request
jmaher
: review+
Details
58 bytes, text/x-review-board-request
jmaher
: review+
Details
58 bytes, text/x-review-board-request
ato
: review+
Details
58 bytes, text/x-review-board-request
jgraham
: review+
Details
58 bytes, text/x-review-board-request
jgraham
: review+
Details
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?
Blocks: e10s-tests
tracking-e10s: --- → +
Hey ahal, how hard would it be to default to running our tests with e10s enabled?
Flags: needinfo?(ahalberstadt)
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)
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.
Assignee: nobody → stanleym2711
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+
This should have the 2 suggested changes you advised me to make.
Attachment #8725461 - Flags: review?(ahalberstadt)
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+
Attachment #8724266 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ae5567d429b0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Forgot to set [leave-open].
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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: stanleym2711 → ahalberstadt
Depends on: 1259804
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
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)
Attachment #8737251 - Flags: review?(jmaher)
Attachment #8737252 - Flags: review?(jmaher)
Attachment #8737253 - Flags: review?(jmaher)
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.
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)
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+
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.
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+
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+
Depends on: 1254029
No longer blocks: 1263092
Depends on: 1263092
Depends on: 1274002
You need to log in before you can comment on or make changes to this bug.