[webkit-reviews] review denied: [Bug 173070] webkitpy: By default, OutputCapture should capture all output which would be printed : [Attachment 312306] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 173070: webkitpy: By default, OutputCapture should capture all output which
would be printed
https://bugs.webkit.org/show_bug.cgi?id=173070

Attachment 312306: Patch

https://bugs.webkit.org/attachment.cgi?id=312306&action=review




--- Comment #11 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 312306
  --> https://bugs.webkit.org/attachment.cgi?id=312306
Patch

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

> Tools/Scripts/webkitpy/common/system/outputcapture.py:47
> -	   self._log_level = logging.INFO
> +	   self._log_level = log_level if log_level is not None else
logging.getLogger().level

r-, this will cause test failures when you run the test using "test-webkitpy
-vv".

OutputCapture is used almost exclusively in unit testing code. It seems error
prone to have OutputCatpure use the current logger level instead of hardcoded
log level as an unbalanced logger.getLogger().setLevel() can cause later code
that uses OutputCapture to be affected. Although we would notice such an
unbalanced logger.getLogger().setLevel() that affects current tests (as they
would fail) it would be hard to notice such an unbalanced call to
logger.getLogger().setLevel() added to new code tested with newly added tests
(or future tests added long after the code that added the unbalanced
logger.getLogger().setLevel() was committed) unless both patch writer and
reviewer are extra vigilant. For patch writers that explicitly want to capture
a logging level different from logging.INFO they can use
OutputCapture.set_log_level() to explicitly opt-into capturing such log
messages.