Adding chunking support to mochitest browser chrome tests

RESOLVED FIXED in mozilla24

Status

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: catlee, Assigned: Gavin)

Tracking

Trunk
mozilla24
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

No description provided.
testing/mochitest/runtests.py already supports --total-chunks --this-chunk and --chunk-by-dir. Is this bug valid?
Blocks: 819963
No longer blocks: 883157
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.
Component: General → BrowserTest
getTestList's complicated return value wasn't actually being used by any callers, so have it only return a list of tests.
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #763245 - Flags: review?(jmaher)
Splits the test chunking code into a separate file for re-use by mochitest-browser-chrome.
Attachment #763247 - Flags: review?(jmaher)
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)
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 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 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 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+
(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.
Attachment #763605 - Flags: review?
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 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.
(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 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+
Comment on attachment 763605 [details] [diff] [review]
mach support for chunking

Can you put this in a new bug?
Attachment #763605 - Flags: review?
Flags: in-testsuite-
(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)
It did happen - see bug 885577 and 860839.
Flags: needinfo?(catlee)
Depends on: 938700
Component: BrowserTest → Mochitest
You need to log in before you can comment on or make changes to this bug.