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

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Matt Brubeck (:mbrubeck)]

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

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: 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.

Comment 2

[Matt Brubeck (:mbrubeck)]

"mach reftest $DIR" is broken in a similar way but will require a different fix.  I'm working on that now.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: fix mochitests

Uploaded the wrong patch by accident.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: fix reftests

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.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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.

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: fix mochitests (v2)

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

Done, thanks.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 711767 [details] [diff] [review]
fix mochitests (v2)

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

LGTM.

Comment 8

[Gregory Szorc [:gps]]

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.

Comment 9

[Matt Brubeck (:mbrubeck)]

(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 10

[Gregory Szorc [:gps]]

Comment on attachment 711677 [details] [diff] [review]
fix reftests

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

LGTM.

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f052742266d
https://hg.mozilla.org/integration/mozilla-inbound/rev/d01bf199fcba

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1f052742266d
https://hg.mozilla.org/mozilla-central/rev/d01bf199fcba