Closed Bug 839340 Opened 9 years ago Closed 9 years ago

"mach mochitest-* $DIR" doesn't work on Windows (os.path.sep problem)

Categories

(Firefox Build System :: Mach Core, enhancement)

All
Windows 8
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla21

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Details

Attachments

(2 files, 2 obsolete files)

This works:

  pymake -C $OBJDIR mochitest-plain toolkit/components/passwordmgr/test

This fails:

  ./mach mochitest-plain toolkit/components/passwordmgr/test

Part of the problem is this line, which adds a backslash (\) to the end of the path on Windows:
http://hg.mozilla.org/mozilla-central/file/a46bc920998d/testing/moztesting/util.py#l17

...so the test runner ends up with this URL (note the %20):

  http://mochi.test:8888/tests/toolkit/components/passwordmgr/test%20

If I pass backslashes (with two layers of escaping):

  ./mach mochitest-plain toolkit\\\\\\\\components\\\\\\\\passwordmgr\\\\\\\\test\\\\\\\\

...then the correct path makes it through but the backslashes end up in the URL, making it invalid:

  http://mochi.test:8888/tests/toolkit%5Ccomponents%5Cpasswordmgr%5Ctest%5C
Attached patch patch (obsolete) — Splinter Review
I think it's better not to use parse_test_path here at all.  Unlike the mach reftest command, the mochitest commands don't actually need or use this parsing.
Assignee: nobody → mbrubeck
Status: NEW → ASSIGNED
Attachment #711635 - Flags: review?(gps)
"mach reftest $DIR" is broken in a similar way but will require a different fix.  I'm working on that now.
Attached patch fix mochitests (obsolete) — Splinter Review
Uploaded the wrong patch by accident.
Attachment #711635 - Attachment is obsolete: true
Attachment #711635 - Flags: review?(gps)
Attachment #711676 - Flags: review?(gps)
Attached patch fix reftestsSplinter Review
mach on Windows sets a TEST_PATH with backslashes, so TEST_PATH needs to be quoted in command lines.

Also fixed some bugs in parse_test_path:
* Call normpath on both test_path and topsrcdir before comparing them, so the comparison works whether you use forward- or backslashes on Windows.
* Don't append a path separator; this is unnecessary since we use os.path.join to concatenate paths.
Attachment #711677 - Flags: review?(gps)
Whiteboard: [has patch]
(In reply to Matt Brubeck (:mbrubeck) from comment #1)
> Created attachment 711635 [details] [diff] [review]
> patch
> 
> I think it's better not to use parse_test_path here at all.  Unlike the mach
> reftest command, the mochitest commands don't actually need or use this
> parsing.

Remove the

from moztesting.util import parse_test_path

line, then, please.
(In reply to :Ms2ger from comment #5)
> Remove the
> 
> from moztesting.util import parse_test_path

Done, thanks.
Attachment #711676 - Attachment is obsolete: true
Attachment #711676 - Flags: review?(gps)
Attachment #711767 - Flags: review?(gps)
Comment on attachment 711767 [details] [diff] [review]
fix mochitests (v2)

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

LGTM.
Attachment #711767 - Flags: review?(gps) → review+
Comment on attachment 711677 [details] [diff] [review]
fix reftests

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

Why do we need to add quotes to the paths in testsuite-targets.mk? Is mach passing a different path than "native" invocation? I think adding quotes is OK. I'm just curious.

Will r+ once I have answer.
(In reply to Gregory Szorc [:gps] from comment #8)
> Why do we need to add quotes to the paths in testsuite-targets.mk? Is mach
> passing a different path than "native" invocation? I think adding quotes is
> OK. I'm just curious.

Manually invoking "make" is also broken if you use backslashes in TEST_PATH:

  make TEST_PATH=layout/reftests/scrolling reftest # this works

  make TEST_PATH=layout\\reftests\\scrolling reftest # broken

mach always passes backslashes, so it is always broken.

This patch fixes both "mach" and "make" with backslashes.
Comment on attachment 711677 [details] [diff] [review]
fix reftests

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

LGTM.
Attachment #711677 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/1f052742266d
https://hg.mozilla.org/mozilla-central/rev/d01bf199fcba
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.