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.
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.
This patch includes the code for total number of passed/failed/todo tests.
This patch adds mach support too.
This patch contains proper mach support and also tested from chrome and mochitest-plain tests.
Attachment #8424971 - Attachment is obsolete: true
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.
some updates and cleanup- this is ready for review!
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.
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.
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+
thanks for the quick review: https://hg.mozilla.org/integration/mozilla-inbound/rev/272c37660666
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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.
You need to log in before you can comment on or make changes to this bug.