Closed
Bug 1243083
Opened 8 years ago
Closed 8 years ago
Run test harnesses with e10s enabled by default and provide an option to disable it
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(e10s+, firefox47 fixed, firefox48 fixed)
RESOLVED
FIXED
mozilla47
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?
Updated•8 years ago
|
Blocks: e10s-tests
tracking-e10s:
--- → +
Comment 1•8 years ago
|
||
Hey ahal, how hard would it be to default to running our tests with e10s enabled?
Flags: needinfo?(ahalberstadt)
Assignee | ||
Comment 2•8 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•8 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.
Comment 4•8 years ago
|
||
Attachment #8724266 -
Flags: review?(ahalberstadt)
Updated•8 years ago
|
Assignee: nobody → stanleym2711
Assignee | ||
Comment 5•8 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+
Comment 6•8 years ago
|
||
This should have the 2 suggested changes you advised me to make.
Attachment #8725461 -
Flags: review?(ahalberstadt)
Assignee | ||
Comment 7•8 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•8 years ago
|
Attachment #8724266 -
Attachment is obsolete: true
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ae5567d429b0
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Assignee | ||
Comment 10•8 years ago
|
||
Forgot to set [leave-open].
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 11•8 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)
Comment 12•8 years ago
|
||
Hi Andrew, yes I am on this. I'll have a patch ready by end of tomorrow latest.
Flags: needinfo?(stanleym2711)
Assignee | ||
Updated•8 years ago
|
Assignee: stanleym2711 → ahalberstadt
Assignee | ||
Comment 13•8 years ago
|
||
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•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43849/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43849/
Assignee | ||
Comment 15•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43851/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43851/
Assignee | ||
Comment 16•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43853/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43853/
Assignee | ||
Comment 17•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43855/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43855/
Assignee | ||
Comment 18•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43857/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43857/
Assignee | ||
Comment 19•8 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•8 years ago
|
||
Mozreview wouldn't let me flag jmaher as he is pto.. I'll flag him Monday.
Updated•8 years ago
|
Attachment #8737254 -
Flags: review?(ato) → review+
Comment 21•8 years ago
|
||
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•8 years ago
|
||
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 23•8 years ago
|
||
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•8 years ago
|
Attachment #8737251 -
Flags: review?(jmaher)
Attachment #8737252 -
Flags: review?(jmaher)
Attachment #8737253 -
Flags: review?(jmaher)
Assignee | ||
Comment 24•8 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.
Comment 25•8 years ago
|
||
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•8 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 27•8 years ago
|
||
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 28•8 years ago
|
||
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 29•8 years ago
|
||
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•8 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?
Comment 31•8 years ago
|
||
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•8 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 33•8 years ago
|
||
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•8 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 35•8 years ago
|
||
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•8 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•8 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
Closed: 8 years ago → 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Depends on: 1263646
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•