Closed Bug 992911 (run-by-dir) Opened 8 years ago Closed 7 years ago

add the ability to run mochitests per directory in a loop


(Testing :: Mochitest, defect)

Not set


(Not tracked)



(Reporter: jmaher, Assigned: jmaher)


(Blocks 1 open bug)


(Keywords: ateam-unittests-task)


(1 file, 8 obsolete files)

I propose --run-by-dir to mochitest where we run mochitest in a loop, where each loop is a directory.

This would effectively be:
dirs = getlistofalltestdirs()
for dir in dirs:

where runtests creates a profile, sets up servers, runs the tests for the given path (in this case dir), and then cleans everything up (leakfiles, crashdumps, servers, profile).

While this adds overhead to the total runtime, we get a great balance between runtime and test isolation.
Attached patch run per directory (WIP) (obsolete) — Splinter Review
a very alpha version- this takes care of building the directory list, chunking the directory list, and adding the loop into things, it doesn't:
* add a cli option
* have full support of --test-path
* integrate with mach
* have testing for various options (debugger, etc.)
* have a way to summarize tests at the end (useful for current tbpl)
Great compromise. Would it be possible to change this to "per manifest" instead of "per directory?" Or do we not have enough things using manifests yet to make this possible?
mochitests are all on manifests, what is the difference of per manifest vs per directory?  I like the idea, my only reason for asking is that I need to understand what differences there are.
Blocks: 996504
Depends on: 999602
Depends on: 999610
Depends on: 999620
Depends on: 957369
Depends on: 999771
Depends on: 999781
Depends on: 525284
Depends on: 1000171
Depends on: 1000311
Depends on: 947574
Depends on: 1001820
Depends on: 1001821
Depends on: 1002093
Depends on: 1002439
Attachment #8402642 - Attachment is obsolete: true
Depends on: 1007211
Alias: run-by-dir
Depends on: 1007687
Attached patch mochitest_chunks.patch (obsolete) — Splinter Review
This patch includes the code for total number of passed/failed/todo tests.
Attached patch mochitest_chunks.patch (obsolete) — Splinter Review
This patch adds mach support too.
Attachment #8414436 - Attachment is obsolete: true
Attachment #8424898 - Attachment is obsolete: true
Attached patch mochitest_chunks.patch (obsolete) — Splinter Review
This patch contains proper mach support and also tested from chrome and mochitest-plain tests.
Attachment #8424971 - Attachment is obsolete: true
Assignee: nobody → vaibhavmagarwal
Attached patch run tests per directory (1.0) (obsolete) — Splinter Review
This patch (with the help of :vaibhav) is much closer to ready.  Ted, if you could take a few minutes and go over the approach- let me know if anything big jumps out at you.  There is more cleanup to do, prior to review.
Assignee: vaibhavmagarwal → jmaher
Attachment #8425563 - Attachment is obsolete: true
Attachment #8427165 - Flags: feedback?(ted)
Attached patch run tests per directory (1.1) (obsolete) — Splinter Review
some updates and cleanup- this is ready for review!
Attachment #8427165 - Attachment is obsolete: true
Attachment #8427165 - Flags: feedback?(ted)
Attachment #8429451 - Flags: review?(ted)
Depends on: 963048
Depends on: 1017158
Depends on: 1017163
Depends on: 1017179
Depends on: 1017181
Depends on: 1017187
Attachment #8429451 - Flags: review?(ted) → review?(ahalberstadt)
Comment on attachment 8429451 [details] [diff] [review]
run tests per directory (1.1)

Review of attachment 8429451 [details] [diff] [review]:

r+ with some nits. This will also conflict heavily with Ahmed's mochitest structured logging patch. I'll let him know.

::: testing/mochitest/chrome-harness.js
@@ +163,4 @@
>    var uri = getResolvedURI(basePath);
>    var chromeDir = getChromeDir(uri);
>    chromeDir.appendRelativePath(dir);
> +  basePath += '/' + dir.replace(/\\\\/g, '\\').replace(/\\/g, '/');

Why not change the '\\' directly to '/'? Will result in fewer replacements.

::: testing/mochitest/
@@ +11,5 @@
>  import sys
>  SCRIPT_DIR = os.path.abspath(os.path.realpath(os.path.dirname(__file__)))
>  sys.path.insert(0, SCRIPT_DIR);
> +# used to find what directory files and directories live in
> +TEST_DIR = os.path.dirname(__file__)

This is the same thing as SCRIPT_DIR except this won't follow symlinks, please just use that.

@@ +1325,5 @@
> +    if not options.runByDir:
> +      return self.doTests(options, onLaunch)
> +
> +    dirs = self.getDirectories(options)
> +    

nit: whitespace

@@ +1480,5 @@
>" | Running tests: end.")
> +    if self.manifest is not None:
> +      self.cleanup(self.manifest, options)

No need to pass in self.manifest here. Can just modify cleanup to use it.
Attachment #8429451 - Flags: review?(ahalberstadt) → review+
resolved all the nit's and comments.  Thanks for the review, verified on try server.
Attachment #8429451 - Attachment is obsolete: true
Attachment #8432407 - Flags: review+
sorry had to backout this change for test failures like
added support for the android harness ( and for b2g when we don't have a tests.json available as we call runtests() directly and do not parse the manifest.
Attachment #8432407 - Attachment is obsolete: true
Attachment #8433297 - Flags: review?(ahalberstadt)
Comment on attachment 8433297 [details] [diff] [review]
--run-by-dir support for mochitests (2.0)

Review of attachment 8433297 [details] [diff] [review]:

Attachment #8433297 - Flags: review?(ahalberstadt) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
No longer depends on: 1000311
No longer depends on: 1001821
No longer depends on: 1002093
No longer depends on: 999602
No longer depends on: 999610
No longer depends on: 999620
No longer depends on: 999771
Depends on: 963075
Depends on: 1041527
Depends on: 1041537
Depends on: 1041544
Depends on: 1041549
Depends on: 1041569
Depends on: 1026310
Depends on: 1041583
Depends on: 1041594
No longer depends on: 1001820
No longer depends on: 1007687
No longer depends on: 1026310
No longer depends on: 999781
No longer depends on: 1041594
FYI, mochitest-e10s-browser-chrome was enabled on Linux builds on trunk today. We'll want to make sure that gets added to future Try runs as well.
No longer depends on: 963075
You need to log in before you can comment on or make changes to this bug.