883314 - Adding chunking support to mochitest browser chrome tests

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Comment 1

[Gregory Szorc [:gps]]

testing/mochitest/runtests.py already supports --total-chunks --this-chunk and --chunk-by-dir. Is this bug valid?

Comment 2

[Chris AtLee [:catlee]]

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.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch part 1: cleanup

getTestList's complicated return value wasn't actually being used by any callers, so have it only return a list of tests.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch part 2: refactor chunking code

Splits the test chunking code into a separate file for re-use by mochitest-browser-chrome.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch part 3: adjust chunking code to handle bc/mochitest-chrome

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

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch part 4: make use of chunkifyTests.js for bc

Comment 7

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

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.

Comment 8

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

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?

Comment 9

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

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

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

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.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 12

[Chris AtLee [:catlee]]

Attachment: mach support for chunking

Comment 13

[:Gavin Sharp [email: [e-mail]]]

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

[Gregory Szorc [:gps]]

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.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] 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

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

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.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

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

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 763605 [details] [diff] [review]
mach support for chunking

Can you put this in a new bug?

Comment 19

[Ed Morley [:emorley]]

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

Comment 20

[:Gijs (he/him)]

(In reply to :Gavin Sharp (use [e-mail] 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).

Comment 21

[Chris AtLee [:catlee]]

It did happen - see bug 885577 and 860839.