Bug 215380

<rdar://problem/66846384>

Created attachment 406388 [details]
Patch

Created attachment 406399 [details]
Patch

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')
```

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 :)

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.

Created attachment 406654 [details]
Patch

(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)

Created attachment 406655 [details]
Patch

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.

Created attachment 406734 [details]
Patch for landing

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

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