Bug 215380 – [webkitcorepy] Add OutputCapture to webkitcorepy

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

Webkitpy has an OutputCapture class (Tools/Scripts/webkitpy/common/system/outputcapture.py), but it uses an older set of idioms that modern Python developers usually find confusing. This has resulted in the OutputCapture not being used much in modern webkitpy code, which is unfortunate, because the idea behind the OutputCapture class is a fantastic one. It's also an idea that's useful for more than just testing (for example, sending a log to both stdout and a file).

This bug attempts to create an OutputCapture object inspired by the one in webkitpy, but using more modern idioms and more explicit access of stdout/stderr vs logging channels.

[Radar WebKit Bug Importer]

<rdar://problem/66846384>

[Jonathan Bedard]

Created attachment 406388 [details]
Patch

[Jonathan Bedard]

Created attachment 406399 [details]
Patch

[Jonathan Bedard]

Comment on attachment 406399 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/bot/ircbot_unittest.py:73
> +        self.assertEqual(captured.root.log.getvalue(), 'MOCK: irc.post: Exception executing command: mock_exception\n')

This is a taste of what the refactor adopting the new OutputCapture would look like. It's not particularly difficult, but I would like to make sure we have general agreement that this is the right direction before I change everything.

Comment on attachment 406388 [details]
Patch

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

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/output_capture.py:51
> +        self.log = StringIO()

Can we really we reset `self.log` every time?

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/tests/output_capture_unittest.py:54
> +            with capturer:
> +                with capturer:
> +                    pass
> +

What happens when I do
```
with capturer:
    log.info('level 1')
    with capturer:
        log.info('level 2')
self.assertEqual(capturer.log.getvalue(), 'level 1\nlevel 2\n')
```

[Jonathan Bedard]

Comment on attachment 406388 [details]
Patch

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

>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/output_capture.py:51
>> +        self.log = StringIO()
> 
> Can we really we reset `self.log` every time?

We could, but I think your point about re-entry and what should happen is a really good one, we probably shouldn't.

Will update and get a better answer to your later question :)

[Jonathan Bedard]

Comment on attachment 406388 [details]
Patch

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

>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/tests/output_capture_unittest.py:54
>> +
> 
> What happens when I do
> ```
> with capturer:
>     log.info('level 1')
>     with capturer:
>         log.info('level 2')
> self.assertEqual(capturer.log.getvalue(), 'level 1\nlevel 2\n')
> ```

Needed to read my own code.

Entering level 2 will raise a ValueError exception with the current patch.

Comment on attachment 406388 [details]
Patch

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

>>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/tests/output_capture_unittest.py:54
>>> +
>> 
>> What happens when I do
>> ```
>> with capturer:
>>     log.info('level 1')
>>     with capturer:
>>         log.info('level 2')
>> self.assertEqual(capturer.log.getvalue(), 'level 1\nlevel 2\n')
>> ```
> 
> Needed to read my own code.
> 
> Entering level 2 will raise a ValueError exception with the current patch.

I see, so we don't want nested use or even re-use.

Comment on attachment 406388 [details]
Patch

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

>>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/output_capture.py:51
>>> +        self.log = StringIO()
>> 
>> Can we really we reset `self.log` every time?
> 
> We could, but I think your point about re-entry and what should happen is a really good one, we probably shouldn't.
> 
> Will update and get a better answer to your later question :)

OK, depends on whether we allow/want to re-use / nested using this context object, this might be subject to change.
If we don't want this to be re-used, then we should set self.log only once, either in `__init__` or '__enter__'. Setting at both place seems not necessary.

[Jonathan Bedard]

Created attachment 406654 [details]
Patch

[Jonathan Bedard]

(In reply to dewei_zhu from comment #9)
> Comment on attachment 406388 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406388&action=review
> 
> >>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/output_capture.py:51
> >>> +        self.log = StringIO()
> >> 
> >> Can we really we reset `self.log` every time?
> > 
> > We could, but I think your point about re-entry and what should happen is a really good one, we probably shouldn't.
> > 
> > Will update and get a better answer to your later question :)
> 
> OK, depends on whether we allow/want to re-use / nested using this context
> object, this might be subject to change.
> If we don't want this to be re-used, then we should set self.log only once,
> either in `__init__` or '__enter__'. Setting at both place seems not
> necessary.

Yes!

I think allowing both re-use and nested usage makes sense for the OutputCapture and LoggerCapture (I can see a weird use case where you might want to capture, the start a second capture, but then stop the second capture before restarting it again, which would require either re-using a capture or nesting the first capture)

It's the OutputDuplicate that I'm a bit more skeptical about, because nesting OutputDuplicates results in triplicating the output (which is correct in the pedantic sense, but feels weird)

[Jonathan Bedard]

Created attachment 406655 [details]
Patch

[Jonathan Bedard]

Comment on attachment 406655 [details]
Patch

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

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/tests/output_capture_unittest.py:103
> +            self.assertEqual(duplicator.output.getvalue(), 'Log 1\nLevel 1\nLog 2\nLog 2\nLevel 2\nLevel 2\n')

This test-case shows the weirdness of nesting the OutputDuplicate class.

[Jonathan Bedard]

Created attachment 406734 [details]
Patch for landing

[EWS]

Committed r265769: <https://trac.webkit.org/changeset/265769>

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