Closed
Bug 932186
Opened 11 years ago
Closed 11 years ago
Allow mach to specify a manifest file as a test path
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla28
People
(Reporter: markh, Assigned: markh)
References
Details
Attachments
(1 file)
(deleted),
patch
|
ted
:
review+
gps
:
feedback+
|
Details | Diff | Splinter Review |
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.
Attachment #823863 -
Flags: feedback?(gps)
Updated•11 years ago
|
Component: mach → Mochitest
Product: Core → Testing
Comment 1•11 years ago
|
||
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.
Attachment #823863 -
Flags: feedback?(gps) → feedback+
Assignee | ||
Comment 2•11 years ago
|
||
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.
Attachment #823863 -
Flags: review?(ted)
Assignee | ||
Comment 3•11 years ago
|
||
(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•11 years ago
|
||
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.
Attachment #823863 -
Flags: review?(ted) → review+
Comment 5•11 years ago
|
||
Sorry for the extreme review delay, I've been derelict in my reviewing.
Updated•11 years ago
|
Assignee: nobody → mhammond
Assignee | ||
Comment 6•11 years ago
|
||
Thanks! Pushed with comments addressed - https://hg.mozilla.org/integration/mozilla-inbound/rev/9df8c9aa8ddc
Status: NEW → ASSIGNED
Comment 7•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in
before you can comment on or make changes to this bug.
Description
•