Bug 215712 – [webkitcorepy] Standard Popen mocking API

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

Tests frequently need to mock subprocess calls. It's not obvious, but all subprocess calls can be mocked by mocking Popen well. We should have common way of mocking subprocess calls that is thorough enough that code being tested does not have to consider the testing harness that will be testing it. (namely, I think the "Executive" idiom of webkitpy makes testing much harder)

[Radar WebKit Bug Importer]

<rdar://problem/67501911>

[Jonathan Bedard]

Created attachment 406963 [details]
Patch

[Jonathan Bedard]

Comment on attachment 406963 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406963&action=review

> Tools/Scripts/libraries/webkitcorepy/README.md:85
> +    ls=lambda args, input=None, cwd=None: mocks.ProcessCompletion(returncode=0, stdout='file1.txt\nfile2.txt\n'),

The most interesting part of this PR, in my opinion, is actually the API for generating the mocked subprocess call. I've already used this myself for prototyping some SCM tooling, and it seems both sufficiently flexible for complicated mocking while being short enough for more trivial use cases.

[Jonathan Bedard]

Created attachment 407245 [details]
Patch

[Matt Lewis]

Talked offline, but I think the api for unit testing and integration testing could use a bit of simplification.

rather than with mocks.Subprocess(mocks.Subprocess.Router('ls', completion=mocks.ProcessCompletion(returncode=0, stdout='file1.txt\nfile2.txt\n'),)):

we should have something a bit easier a la:
with mocks.Subprocess('ls',completion=mocks.ProcessCompletion(returncode=0, stdout='file1.txt\nfile2.txt\n'), )):

and
(found by back an forth with Jonathan)

mocks.Subprocess.group_commands(
    mocks.Subprocess.Router(...),
    mocks.Subprocess.Router(...),
):

for unit and integration tests respectively.

Comment on attachment 407245 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407245&action=review

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/mocks/popen.py:43
> +    if sys.version_info > (3, 0):

I still see a lot of things in common in the implementation of __init__, communicate and etc.
Wouldn't it be much clearer if we have a base class?
```
class PopenBase(object):
      def __init__
...

if sys.version_info > (3, 0):
    class Popen(PopenBase):
        def __init__
        def __enter__
        def __exit__
        ...
else:
    class Popen(PopenBase):
        ...
```

[Jonathan Bedard]

(In reply to dewei_zhu from comment #6)
> Comment on attachment 407245 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407245&action=review
> 
> > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/mocks/popen.py:43
> > +    if sys.version_info > (3, 0):
> 
> I still see a lot of things in common in the implementation of __init__,
> communicate and etc.
> Wouldn't it be much clearer if we have a base class?
> ...

That does work much better, yes.

[Jonathan Bedard]

Created attachment 407317 [details]
Patch

[Jonathan Bedard]

(In reply to Matt Lewis from comment #5)
> Talked offline, but I think the api for unit testing and integration testing
> could use a bit of simplification.
> 
> rather than with mocks.Subprocess(mocks.Subprocess.Router('ls',
> completion=mocks.ProcessCompletion(returncode=0,
> stdout='file1.txt\nfile2.txt\n'),)):
> 
> we should have something a bit easier a la:
> with mocks.Subprocess('ls',completion=mocks.ProcessCompletion(returncode=0,
> stdout='file1.txt\nfile2.txt\n'), )):
> 
> and
> (found by back an forth with Jonathan)
> 
> mocks.Subprocess.group_commands(
>     mocks.Subprocess.Router(...),
>     mocks.Subprocess.Router(...),
> ):
> 
> for unit and integration tests respectively.

Turns out we don't even need group_commands to make this API work.

[Matt Lewis]

I like this much better. While I would prefer 'Router' be more specific, it's more of a nit in the direction of my personal preference, especially since it seems like you want to keep it short and brief due to subprocess's existing naming.

Comment on attachment 407317 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407317&action=review

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/mocks/popen.py:237
> +    @property
> +    def universal_newlines(self):
> +        return self.text_mode
> +
> +    @universal_newlines.setter
> +    def universal_newlines(self, universal_newlines):
> +        self.text_mode = bool(universal_newlines)
> +
> +    def poll(self):
> +        if not self._completion:
> +            self.stdin.seek(0)
> +            self._completion = Subprocess.completion_for(*self._args, cwd=self._cwd, input=self.stdin.read())
> +
> +            (self.stdout or sys.stdout).write(string_utils.decode(self._completion.stdout, target_type=self._stdout_type))
> +            (self.stdout or sys.stdout).flush()
> +
> +            (self.stderr or sys.stderr).write(string_utils.decode(self._completion.stderr, target_type=self._stderr_type))
> +            (self.stderr or sys.stderr).flush()
> +
> +        if self.returncode is not None and time.time() >= self._start_time + self._completion.elapsed:
> +            self.returncode = self._completion.returncode
> +            if self.stdout:
> +                self.stdout.seek(0)
> +            if self.stderr:
> +                self.stderr.seek(0)
> +
> +        return self.returncode
> +
> +    def send_signal(self, sig):
> +        if self.returncode is not None:
> +            return
> +
> +        if sig not in [signal.SIGTERM, signal.SIGKILL]:
> +            raise ValueError('Mock Popen object cannot handle signal {}'.format(sig))
> +        log.critical('Mock process {} send signal {}'.format(self.pid, sig))
> +        self.returncode = -1
> +
> +    def terminate(self):
> +        self.send_signal(signal.SIGTERM)
> +
> +    def kill(self):
> +        self.send_signal(signal.SIGKILL)

Those might fit better on the base class?

r=me

[Jonathan Bedard]

(In reply to dewei_zhu from comment #11)
> Comment on attachment 407317 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407317&action=review
> 
> ...
> 
> Those might fit better on the base class?

I can move those, that would mean we have two entirely difference Popen definitions, one for Python 2 and one for Python 3 (instead of just declaring the specific functions that need differences)

[Matt Lewis]

(In reply to Matt Lewis from comment #10)
> I like this much better. While I would prefer 'Router' be more specific,
> it's more of a nit in the direction of my personal preference, especially
> since it seems like you want to keep it short and brief due to subprocess's
> existing naming.

Offline talk determined using "CommandRoute" in place of Router, with the option of being able to use "Route" as well for brevity's sake.

[Jonathan Bedard]

Created attachment 407328 [details]
Patch

[Matt Lewis]

LGTM. r+

[EWS]

Committed r266190: <https://trac.webkit.org/changeset/266190>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407328 [details].