Summary: | [webkitcorepy] Standard Popen mocking API | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Bedard <jbedard> | ||||||||||
Component: | Tools / Tests | Assignee: | Jonathan Bedard <jbedard> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aakash_jain, dean_johnson, dewei_zhu, jlewis3, slewis, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=214378 | ||||||||||||
Attachments: |
|
Description
Jonathan Bedard
2020-08-20 13:22:17 PDT
Created attachment 406963 [details]
Patch
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. Created attachment 407245 [details]
Patch
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): ... ``` (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. Created attachment 407317 [details]
Patch
(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. 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 (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) (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. Created attachment 407328 [details]
Patch
LGTM. r+ Committed r266190: <https://trac.webkit.org/changeset/266190> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407328 [details]. |