Skip to content

Commit

Permalink
Launch py.test subprocess with shell=False.
Browse files Browse the repository at this point in the history 
This fixes the edge case when py.test arguments contained quoted strings
with spaces, e.g.:

    ptw -- -m 'not slow'
  • Loading branch information
@aldanor
aldanor committed May 27, 2015
1 parent d1d1910 commit dc066e2
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion pytest_watch/watcher.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def run(self, filename=None):
print(Fore.CYAN + msg.format(Fore.LIGHTCYAN_EX + arg + Fore.CYAN) + Fore.RESET)
if self.auto_clear:
print()
exit_code = subprocess.call(command, shell=True)
exit_code = subprocess.call(['py.test'] + self.args, shell=False)

This comment has been minimized.

Sign in to view

Copy link
@joeyespo

joeyespo May 28, 2015

Hmm. Does this have any other side effects? I used shell for a reason, but can't think of it off the top of my head right now.

Also, shell=False is the default, so can probably just remove that arg altogether.

This comment has been minimized.

Sign in to view

Copy link
@aldanor

aldanor May 28, 2015

Author Owner

Docopt runs something like shlex.split and returns us a ready-to-use list of positional arguments to pass to pytest. If we use shell=True, we now have to join all those strings back together and pass them as a string to subprocess.call so the shell can parse it again which is a bit of a waste... Now imagine what happens when one of the arguments contains spaces (and gets dequoted for you by docopt)?

Also, if you do something like

ptw -- test*.py

Your shell will expand it anyway before it even reaches docopt, so I don't see any point in using shell=True.

This comment has been minimized.

Sign in to view

Copy link
@joeyespo

joeyespo May 28, 2015

I'll have to test it locally and see if I can hit the reason why I went with shell=True.

This comment has been minimized.

Sign in to view

Copy link
@aldanor

aldanor May 28, 2015

Author Owner

@joeyespo I think one example is when you would like to expand an environment variable under the subprocess shell and not in the parent shell, but I can't possibly see why you would want to do that in this particular case...

This comment has been minimized.

Sign in to view

Copy link
@joeyespo

joeyespo Jun 14, 2015

Ok, so I remember why I had this now. ptw flat-out doesn't on Windows without it. The reason is that py.test.exe is the qualified file name so the file to run cannot be found. The shell understands how to run programs this way though. The CLEAR_COMMAND won't work with this trick outside of a shell though.

Another way to remove shell=True would be to run python -m pytest, going to revert this change instead until it becomes an actual problem for someone.

To fix quoted arguments, we can manually quote them using quote from shlex (falling back to pipes on Python 2):

' '.join(['py.test'] + [quote(arg) for arg in self.args])

Going to revert to shell=True for now so it runs on Windows.

This comment has been minimized.

Sign in to view

Copy link
@joeyespo

joeyespo Jun 14, 2015

Actually, maybe we can avoid the shell quirks and security risks altogether by calling pytest.main directly.

Can you confirm that this works for you too? This should fix the quoted arg problem. Updated in abfd402.

This comment has been minimized.

Sign in to view

Copy link
@blueyed

blueyed Jun 26, 2015

I think shell=True should only used for Windows then.

At least it needs to be fixed for POSIX, where shell=True implies that you should use a string and not a sequence. Otherwise py.test does not see the args (and cannot be limited to a subdir etc)!

From the docs:

On POSIX with shell=True, the shell defaults to /bin/sh. If args is a string, the string specifies the command to execute through the shell. This means that the string must be formatted exactly as it would be when typed at the shell prompt. This includes, for example, quoting or backslash escaping filenames with spaces in them. If args is a sequence, the first item specifies the command string, and any additional items will be treated as additional arguments to the shell itself. That is to say, Popen does the equivalent of:

Popen(['/bin/sh', '-c', args[0], args[1], ...])

This comment has been minimized.

Sign in to view

Copy link
@aldanor

aldanor Jun 26, 2015

Author Owner

Agreed, this completely breaks it on non-Windows platforms.

This comment has been minimized.

Sign in to view

Copy link
@blueyed

blueyed Jun 26, 2015

Fixed in joeyespo#18.

(Wondered if I was missing something, because it has been broken since a week then already..)

This comment has been minimized.

Sign in to view

Copy link
@joeyespo

joeyespo Jun 27, 2015

I pulled in your change to at least make this workable. From what you're saying though, it looks like this will still be broken on Windows since passing in a sequence isn't correct. I'll take a look at that case.

This comment has been minimized.

Sign in to view

Copy link
@blueyed

blueyed Jun 28, 2015

@joeyespo
Cool, thanks!
From what I remember it should work like it's now: for Windows it's OK to use a sequence (and that's why shell=True was used in the first place - to fix Windows).

This comment has been minimized.

Sign in to view

Copy link
@joeyespo

joeyespo Jun 28, 2015

Just tested it. Everything seems good 👍
passed = exit_code == 0

# Beep if failed
Expand Down

3 comments on commit dc066e2

@aldanor
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joeyespo Just checked it out, seems to work fine so far. Calling pytest.main directly seems like the way to go.

@joeyespo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@joeyespo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Please sign in to comment.