932186 - Allow mach to specify a manifest file as a test path

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: Handle-a-test-path-referring-to-a-test-manifest.patch

When running tests via mach, if you specify a directory name, mach runs all tests in that directory but doesn't evaluate the conditions specified in the relevant manifest in that directory.

This patch allows you to specify the path to a manifest file - it will then run all tests which are enabled by the manifest.

The attachment checks if the test_path specified is a manifest for the test type - in all cases other than mochitest-plain, the manifest is the same as |suite|.

On Windows, the paths have windows path separators - the patch here replaces them with full slashes.

For mochitest-plain tests, the split on self.TEST_PATH splits at the wrong place - there is a "_tests" portion earlier in the path - I fixed this by splitting on "/tests/".

gps: I looked over bug 920849, but don't see how that will be providing the same basic functionality - apologies if I missed something.  In fact, the number of things I could have overlooked is staggering :)  But this should help get a discussion rolling even if it's the wrong approach.

Comment 1

[Gregory Szorc [:gps]]

Comment on attachment 823863 [details] [diff] [review]
Handle-a-test-path-referring-to-a-test-manifest.patch

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

This should work as a short term solution. Long term, we can plug in the code from bug 920849.

Please request review from someone on ATeam for the final patch. Ted might be best since he's reviewing bug 920849.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 823863 [details] [diff] [review]
Handle-a-test-path-referring-to-a-test-manifest.patch

(In reply to Gregory Szorc [:gps] from comment #1)
> Please request review from someone on ATeam for the final patch. Ted might
> be best since he's reviewing bug 920849.

Sorry Ted, you lose again - but please feel free to pass this on.  The urgency is fairly low.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(In reply to Gregory Szorc [:gps] from comment #1)
> This should work as a short term solution. Long term, we can plug in the
> code from bug 920849.

Yeah, I finally got my head around that bug.  I opened bug 938019 for the longer-term solution, but given that might be some time before landing and is necessary for the e10s-testing efforts, I still think we should go ahead with this patch (plus a comment mentioning the temporary nature of the fix and referencing bug 938019)

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 823863 [details] [diff] [review]
Handle-a-test-path-referring-to-a-test-manifest.patch

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

::: testing/mochitest/mach_commands.py
@@ +299,5 @@
>  
> +            # Handle test_path pointing at a manifest file so conditions in
> +            # the manifest are processed.
> +            # The manifest basename is the same as |suite|, except for plain
> +            manifest_base = 'mochitest' if suite == "plain" else suite

Odd mix of single and double quotes here.

::: testing/mochitest/runtests.py
@@ +347,5 @@
>        # Bug 883858 - return all tests including disabled tests
> +      tests = manifest.active_tests(disabled=True, **mozinfo.info)
> +      # We need to ensure we match on a complete directory name matching the
> +      # test root, and not a substring somewhere else in the path.
> +      test_root = os.path.sep + self.getTestRoot(options) + os.path.sep

This strikes me as still sort of terrible, there has to be a nicer way to write this. It's better than what's already here though, so I'm not going to complain too much.

@@ +352,3 @@
>        paths = []
>        for test in tests:
> +        tp = test['path'].split(test_root, 1)[1].replace("\\", "/").strip('/')

Again you have an odd mix of single and double quotes.

Comment 5

[(not currently active) Ted Mielczarek]

Sorry for the extreme review delay, I've been derelict in my reviewing.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Thanks!  Pushed with comments addressed - https://hg.mozilla.org/integration/mozilla-inbound/rev/9df8c9aa8ddc

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9df8c9aa8ddc