Fix automatic shell detection, the check was reversed

Fix automatic shell detection, the check was reversed

Improved tests and added regression test.

R=dpranke@chromium.org
BUG=
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80614

[Dirk Pranke, 2011-04-05 21:18:49]

LGTM.

A couple of questions noted below, though.

http://codereview.chromium.org/6793044/diff/1/subprocess2.py
File subprocess2.py (right):

http://codereview.chromium.org/6793044/diff/1/subprocess2.py#newcode150
subprocess2.py:150: kwargs['shell'] = bool(sys.platform=='win32')
don't think you need the case here.

http://codereview.chromium.org/6793044/diff/1/tests/subprocess2_test.py
File tests/subprocess2_test.py (right):

http://codereview.chromium.org/6793044/diff/1/tests/subprocess2_test.py#newco...
tests/subprocess2_test.py:25: subprocess2: ['Popen', 'call', 'check_call',
'capture', 'check_output'],
Don't need to change this in this patch, but, do you need to stub out things
beyond Popen()? Doesn't everything else end up calling that?

[M-A Ruel, 2011-04-06 01:27:25]

Le 5 avril 2011 17:18, <dpranke@chromium.org> a écrit :

> LGTM.
>
> A couple of questions noted below, though.
>
>
> http://codereview.chromium.org/6793044/diff/1/subprocess2.py
> File subprocess2.py (right):
>
> http://codereview.chromium.org/6793044/diff/1/subprocess2.py#newcode150
> subprocess2.py:150: kwargs['shell'] = bool(sys.platform=='win32')
> don't think you need the case here.
>

Cast? I just wanted to have this extra clear but I agree it's extraneous. I
had added it because I thought I had a bug at a moment.



> http://codereview.chromium.org/6793044/diff/1/tests/subprocess2_test.py
> File tests/subprocess2_test.py (right):
>
>
>
http://codereview.chromium.org/6793044/diff/1/tests/subprocess2_test.py#newco...
> tests/subprocess2_test.py:25: subprocess2: ['Popen', 'call',
>
> 'check_call', 'capture', 'check_output'],
> Don't need to change this in this patch, but, do you need to stub out
> things beyond Popen()? Doesn't everything else end up calling that?


Oh no, just planing to test more.

M-A