Bug 35484 – check-webkit-style: Start using Python's logging module

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

[Chris Jerdonek]

Start using the logging module in check-webkit-style.

Also provide an example of how to unit test logging when using the logging module.

[Chris Jerdonek]

Created attachment 49687 [details]
Proposed patch

[Chris Jerdonek]

Created attachment 49688 [details]
Proposed patch 2

Removed the trailing newline from two logging calls since the logging module appends a newline automatically.

[Chris Jerdonek]

Created attachment 49691 [details]
Proposed patch 3

Refactored the patch so that the supporting unit test code is in a central location.

In a later patch, I can rename the webkitpy/init directory to something like webkitpy/infra to signify that it contains infrastructure-related code (autoinstall, logging, etc).

[Shinichiro Hamaji]

Comment on attachment 49691 [details]
Proposed patch 3

Generally looks great. r- for now, but some of my comments would be wrong.

> diff --git a/WebKitTools/Scripts/webkitpy/init/wklogging.py b/WebKitTools/Scripts/webkitpy/init/wklogging.py

Are you planning to put non-unittest code into this module? If not, I think the name isn't the best. Maybe logging_test_utils.py or something more verbose naming would be better.

If you are planning to add non-unittest code, I'd suggest splitting this file into two files.

> +"""Support for logging."""

It would be better to say this module supports testing of logging.

> +    This method returns a list of references to the logging handlers
> +    added so that the caller can later remove the handlers using
> +    logger.removeHandler.  This is primarily useful in testing
> +    situations where the caller may want to enable logging temporarily
> +    and then undo the enabling.

I'm not sure if this style is widely used, but I prefer the following style

  Returns:
    a list of references to the logging handlers...

cpp.py has bunch of docstrings with this style.

Will we return more than two handlers? If not, I think we can return tuple and explicitly say "this function returns an error-handler and a non-error-handler using tuple".

> +    def test_info_message(self):
> +        # We test the tighter boundary case rather than logging.INFO,
> +        # which equals 30.

Hmm... I'm confused. It seems logging.INFO is 20. I'm not sure, but if I understand the logging module correctly, logging.INFO should be outputted but logging.INFO-1 should not, right? If so, I think logging.INFO is the boundary. Maybe I'm misunderstanding?

[Chris Jerdonek]

(In reply to comment #4)
> (From update of attachment 49691 [details])

Thanks a lot for the helpful comments.

> Are you planning to put non-unittest code into this module? If not, I think the
> name isn't the best. Maybe logging_test_utils.py or something more verbose
> naming would be better.
> 
> If you are planning to add non-unittest code, I'd suggest splitting this file
> into two files.

I was planning to put non-unittest code in the file.  Good idea.  I will rename.

I may choose something a bit shorter though since PEP8 encourages short module names.

> Will we return more than two handlers? If not, I think we can return tuple and
> explicitly say "this function returns an error-handler and a non-error-handler
> using tuple".

For now, just two.  But I would prefer to keep it a list since the calling code only needs to loop through it.  It does not need to know its contents or use the sequence in a structured way as a tuple would imply.

> > +    def test_info_message(self):
> > +        # We test the tighter boundary case rather than logging.INFO,
> > +        # which equals 30.
> 
> Hmm... I'm confused. It seems logging.INFO is 20. I'm not sure, but if I
> understand the logging module correctly, logging.INFO should be outputted but
> logging.INFO-1 should not, right? If so, I think logging.INFO is the boundary.
> Maybe I'm misunderstanding?

You're right -- that was a typo (should be 20 rather than 30).  I'm sorry.  I knew about it and failed to correct.

In any case, the configured handlers should style messages with level 30 and above as an error message, and 29 and below as an info message.

The purpose of my comment was to explain why I was testing 29 even though in practice we would only be using 20 (i.e. _log.info).  I will make the comment clearer.

[Shinichiro Hamaji]

> I may choose something a bit shorter though since PEP8 encourages short module
> names.

Yeah, I think shorter name is OK. I tend to like verbose names maybe because I'm not good at coming up with cool short names :(

> For now, just two.  But I would prefer to keep it a list since the calling code
> only needs to loop through it.  It does not need to know its contents or use
> the sequence in a structured way as a tuple would imply.

I see. It's OK as is.

> You're right -- that was a typo (should be 20 rather than 30).  I'm sorry.  I
> knew about it and failed to correct.
> 
> In any case, the configured handlers should style messages with level 30 and
> above as an error message, and 29 and below as an info message.
> 
> The purpose of my comment was to explain why I was testing 29 even though in
> practice we would only be using 20 (i.e. _log.info).  I will make the comment
> clearer.

Ah, I was also wrong. I missed you are setting logging.WARNING for your filter in configure_logging. Thanks for clarification!

[Chris Jerdonek]

Created attachment 49787 [details]
Proposed patch

[Chris Jerdonek]

Created attachment 49788 [details]
Proposed patch 5

Fixed ChangeLog (extraneous reviewed by line).

[Chris Jerdonek]

Created attachment 49805 [details]
Proposed patch 6

Added __init__,py back to webkitpy/init directory. 

The __init__.py file was deleted from webkitpy/init when the directory was emptied in a recent rollout:

http://trac.webkit.org/changeset/55363

(Note that we will be renaming webkitpy/init to something like webkitpy/infra in a later patch.)

[Shinichiro Hamaji]

Comment on attachment 49805 [details]
Proposed patch 6

Looks good!

[Chris Jerdonek]

Committed (via "webkit-patch land-from-bug"):

http://trac.webkit.org/changeset/55409