Closed
Bug 883314
Opened 11 years ago
Closed 11 years ago
Adding chunking support to mochitest browser chrome tests
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla24
People
(Reporter: catlee, Assigned: Gavin)
References
Details
Attachments
(5 files)
5.68 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
8.52 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
1.50 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
2.03 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
3.20 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•11 years ago
|
||
testing/mochitest/runtests.py already supports --total-chunks --this-chunk and --chunk-by-dir. Is this bug valid?
Updated•11 years ago
|
Reporter | ||
Comment 2•11 years ago
|
||
it doesn't seem to do anything for the browser chrome tests though. For regular mochitests the chunking logic is ultimately implemented by http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/setup.js#127. Does this get used for browser chrome tests? Also, having mach options for chunking would be awesome.
Assignee | ||
Updated•11 years ago
|
Component: General → BrowserTest
Assignee | ||
Comment 3•11 years ago
|
||
getTestList's complicated return value wasn't actually being used by any callers, so have it only return a list of tests.
Assignee | ||
Comment 4•11 years ago
|
||
Splits the test chunking code into a separate file for re-use by mochitest-browser-chrome.
Attachment #763247 -
Flags: review?(jmaher)
Assignee | ||
Comment 5•11 years ago
|
||
The chunking code didn't handle being passed an array of chrome:// URIs, which is what happens for mochitest-chrome (existing issue, but we don't use chunking here atm) and mochitest-browser-chrome (an issue now that we're going to support chunking).
Attachment #763248 -
Flags: review?(jmaher)
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #763249 -
Flags: review?(jmaher)
Comment 7•11 years ago
|
||
Comment on attachment 763245 [details] [diff] [review] patch part 1: cleanup Review of attachment 763245 [details] [diff] [review]: ----------------------------------------------------------------- this is nice, I have always wanted to make the return value much simpler.
Attachment #763245 -
Flags: review?(jmaher) → review+
Comment 8•11 years ago
|
||
Comment on attachment 763247 [details] [diff] [review] patch part 2: refactor chunking code Review of attachment 763247 [details] [diff] [review]: ----------------------------------------------------------------- I really don't understand this, we pulled out 60 lines of code and created a new file out of it? This patch seems fine overall. I assume a future patch in this series will make use of this?
Attachment #763247 -
Flags: review?(jmaher) → review+
Comment 9•11 years ago
|
||
Comment on attachment 763248 [details] [diff] [review] patch part 3: adjust chunking code to handle bc/mochitest-chrome Review of attachment 763248 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/chunkifyTests.js @@ +23,5 @@ > + // URIs > + var protocolRegexp = /^[a-zA-Z]+:\/\//; > + if (protocolRegexp.test(test_path)) { > + test_path = test_path.replace(protocolRegexp, ""); > + } my concern here is we are using test_path for chunkByDir and we are splitting the name up (i.e. removing chrome://). Will this work for chunking?
Comment 10•11 years ago
|
||
Comment on attachment 763249 [details] [diff] [review] patch part 4: make use of chunkifyTests.js for bc Review of attachment 763249 [details] [diff] [review]: ----------------------------------------------------------------- nice and simple, a great benefit.
Attachment #763249 -
Flags: review?(jmaher) → review+
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #9) > my concern here is we are using test_path for chunkByDir and we are > splitting the name up (i.e. removing chrome://). Will this work for > chunking? Not sure I understand the concern - this is required for chunking to work, because the chunking code assumes it is passed a relative file path (as is the case for plain old mochitest), but in the cases of mochitest-chrome and mochitest-browser-chrome we're passing it chrome URIs. This wasn't a problem in the past because we hadn't tried using chunking with either of those until now.
Reporter | ||
Comment 12•11 years ago
|
||
Attachment #763605 -
Flags: review?
Assignee | ||
Comment 13•11 years ago
|
||
One thing to note: the existing chunk-by-dir logic doesn't work so well for mochitest-browser-chrome, I think because much of the tests are concentrated in the same directories. E.g. using --chunk-by-dir=4 --total-chunks=5 with a total of 1353 tests, you get 5 chunks with sizes 1053, 52, 7, 14, 227. Similarly, even with --chunk-by-dir=9000 --total-chunks=2, you get two buckets of size 1053 and 300. So we may need to make the chunkifying logic smarter to handle this particular case...
Comment 14•11 years ago
|
||
Comment on attachment 763605 [details] [diff] [review] mach support for chunking Review of attachment 763605 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/mach_commands.py @@ +38,5 @@ > """ > def run_mochitest_test(self, suite=None, test_file=None, debugger=None, > debugger_args=None, shuffle=False, keep_open=False, rerun_failures=False, > + no_autorun=False, repeat=0, run_until_failure=False, slow=False, > + totalChunks=None, thisChunk=None, chunkByDir=None): Consistent variable naming, please.
Assignee | ||
Comment 15•11 years ago
|
||
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #11) > (In reply to Joel Maher (:jmaher) from comment #9) > > my concern here is we are using test_path for chunkByDir and we are > > splitting the name up (i.e. removing chrome://). Will this work for > > chunking? > > Not sure I understand the concern Oh, maybe I do now. test_path is only used for the chunking logic, we use tests[i] again later on for the actual return value. So this modification doesn't affect the values output by chunkifyTests, just the internal sorting that determines their order etc.
Comment 16•11 years ago
|
||
Comment on attachment 763248 [details] [diff] [review] patch part 3: adjust chunking code to handle bc/mochitest-chrome Review of attachment 763248 [details] [diff] [review]: ----------------------------------------------------------------- thanks for clarifying that the logic doesn't affect the test links themselves, lets land this and profit.
Attachment #763248 -
Flags: review?(jmaher) → review+
Assignee | ||
Comment 17•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/afa0efd81288 https://hg.mozilla.org/integration/mozilla-inbound/rev/a13d509adbf0 https://hg.mozilla.org/integration/mozilla-inbound/rev/1b2f815f5f08 https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6ff98c7ab1
Target Milestone: --- → mozilla24
Assignee | ||
Comment 18•11 years ago
|
||
Comment on attachment 763605 [details] [diff] [review] mach support for chunking Can you put this in a new bug?
Attachment #763605 -
Flags: review?
Assignee | ||
Updated•11 years ago
|
Flags: in-testsuite-
Comment 19•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/afa0efd81288 https://hg.mozilla.org/mozilla-central/rev/a13d509adbf0 https://hg.mozilla.org/mozilla-central/rev/1b2f815f5f08 https://hg.mozilla.org/mozilla-central/rev/7e6ff98c7ab1
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 20•11 years ago
|
||
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #18) > Comment on attachment 763605 [details] [diff] [review] > mach support for chunking > > Can you put this in a new bug? Looking at ./mach help mochitest-browser and the dependencies of this bug, it looks like this never happened. Chris, is that right? Any chance of this still happening sooner-rather-than-later? (especially now that we're talking about removing non-mach ways of invoking tests, and because of test failures in bug 819963 which I (and presumably others) now don't know how to easily reproduce).
Flags: needinfo?(catlee)
Reporter | ||
Comment 21•11 years ago
|
||
It did happen - see bug 885577 and 860839.
Flags: needinfo?(catlee)
Updated•6 years ago
|
Component: BrowserTest → Mochitest
You need to log in
before you can comment on or make changes to this bug.
Description
•