Joel Maher ( :jmaher ) (UTC -8) Assignee
	Description • 10 years ago

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.

	Joel Maher ( :jmaher ) (UTC -8) Assignee
	Comment 1 • 10 years ago

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)

	Gregory Szorc [:gps]
	Comment 2 • 10 years ago

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?

	Joel Maher ( :jmaher ) (UTC -8) Assignee
	Comment 3 • 10 years ago

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.

	Vaibhav (:vaibhav1994)
	Comment 5 • 10 years ago

This patch includes the code for total number of passed/failed/todo tests.

	Vaibhav (:vaibhav1994)
	Comment 6 • 10 years ago

This patch adds mach support too.

	Vaibhav (:vaibhav1994)
	Comment 7 • 10 years ago

This patch contains proper mach support and also tested from chrome and mochitest-plain tests.

	Joel Maher ( :jmaher ) (UTC -8) Assignee
	Comment 8 • 10 years ago

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.

	Joel Maher ( :jmaher ) (UTC -8) Assignee
	Comment 9 • 10 years ago

some updates and cleanup- this is ready for review!

	Andrew Halberstadt [:ahal]
	Comment 10 • 10 years ago

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.

	Joel Maher ( :jmaher ) (UTC -8) Assignee
	Comment 11 • 10 years ago

resolved all the nit's and comments.  Thanks for the review, verified on try server.

	Joel Maher ( :jmaher ) (UTC -8) Assignee
	Comment 12 • 10 years ago

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86d0bd2a0233

	Carsten Book [:Tomcat]
	Comment 13 • 10 years ago

sorry had to backout this change for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=40855268&tree=Mozilla-Inbound

	Joel Maher ( :jmaher ) (UTC -8) Assignee
	Comment 14 • 10 years ago

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.

	Andrew Halberstadt [:ahal]
	Comment 15 • 10 years ago

Comment on attachment 8433297 [details] [diff] [review]
--run-by-dir support for mochitests (2.0)

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

Lgtm!

	Joel Maher ( :jmaher ) (UTC -8) Assignee
	Comment 16 • 10 years ago

thanks for the quick review:
https://hg.mozilla.org/integration/mozilla-inbound/rev/272c37660666

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/272c37660666

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 10 years ago

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.