Bug 172876 – webkitpy: Add contextmanager to disable logging for a block

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

We need a contextmanager which disables logging in a block and renables logging on exit.  Currently, OutputCapture only provides functions to disable and then re-enable logging.

[Radar WebKit Bug Importer]

<rdar://problem/32546391>

[Jonathan Bedard]

Created attachment 311877 [details]
Patch

[Daniel Bates]

Comment on attachment 311877 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/outputcapture.py:36
> +from contextlib import contextmanager
>  from StringIO import StringIO

These imports should be sorted based on the UNIX sort command.

> Tools/Scripts/webkitpy/common/system/outputcapture.py:112
> +class Suppressor(OutputCapture):

Maybe a better name for this would be OutputCaptureScope?

> Tools/Scripts/webkitpy/common/system/outputcapture.py:116
> +        self.last_log = None

I suggest that we use the same terminology as used in OutputCapture and call this captured_output.

> Tools/Scripts/webkitpy/common/system/outputcapture.py:123
> +    @contextmanager
> +    def suppress_logging(self):
> +        self.capture_output()
> +        yield
> +        self.last_log = self.restore_output()
> +

Instead of using @contextmanager can we have this class implement __enter__() and __exit__() itself so that we can make use of this convenience class using the following syntax:

scope = OutputCaptureScope()
with scope:
    ...
scope.captured_output

[Daniel Bates]

Comment on attachment 311877 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/system/outputcapture.py:112
>> +class Suppressor(OutputCapture):
> 
> Maybe a better name for this would be OutputCaptureScope?

We may want to have this class take the OutputCapture instance instead of extending it to make it easy to use this class correctly and hard to use it incorrectly or un-idiomatic.

[Jonathan Bedard]

Created attachment 311886 [details]
Patch

[Daniel Bates]

Comment on attachment 311886 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/outputcapture.py:114
> +        self.captured_output = None

This should return (None, None, None) to match up with how restore_output() behaves.

> Tools/Scripts/webkitpy/common/system/outputcapture_unittest.py:61
> +    def test_output_capture_scope(self):

Please add a test where OutputScope takes an OutputCapture as an argument.

[Daniel Bates]

Comment on attachment 311886 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/outputcapture.py:120
> +        self.captured_output = self._output_capture.restore_output()

exc_val => exc_value
exc_tb => traceback

(To match https://docs.python.org/2/reference/datamodel.html#context-managers)

[Jonathan Bedard]

Created attachment 311927 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 311927 [details]
Patch for landing

Clearing flags on attachment: 311927

Committed r217758: <http://trac.webkit.org/changeset/217758>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.