Bug 992911 (run-by-dir)

add the ability to run mochitests per directory in a loop

RESOLVED FIXED in mozilla32

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jmaher, Assigned: jmaher)

Tracking

(Blocks 1 bug, {ateam-unittests-task})

Trunk
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

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:
    runtests(dir)

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.
Posted 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
Posted patch mochitest_chunks.patch (obsolete) — Splinter Review
This patch includes the code for total number of passed/failed/todo tests.
Posted 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
Posted 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
Posted 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
Status: NEW → ASSIGNED
Attachment #8427165 - Flags: feedback?(ted)
Posted 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/runtests.py
@@ +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 @@
>  
>      log.info("runtests.py | 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 https://tbpl.mozilla.org/php/getParsedLog.php?id=40855268&tree=Mozilla-Inbound
added support for the android harness (automation.py.in) 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]:
-----------------------------------------------------------------

Lgtm!
Attachment #8433297 - Flags: review?(ahalberstadt) → review+
https://hg.mozilla.org/mozilla-central/rev/272c37660666
Status: ASSIGNED → RESOLVED
Closed: 5 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.