Bug 58295 – new-run-webkit-tests: suppress extraneous pretty patch warnings

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

new-run-webkit-tests: suppress extraneous pretty patch warnings

[Dirk Pranke]

Created attachment 89144 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 89144 [details]
Patch

Doesn't python logging already have built in ways to filter based on where the messages came from?

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 89144 [details])
> Doesn't python logging already have built in ways to filter based on where the messages came from?

Yeah, but the messages are being logged in the same place in both cases, so I'm not sure how that would help?

[Ojan Vafai]

Comment on attachment 89144 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/base.py:178
> +    def check_pretty_patch(self, logger=None):

It seems wrong that we're calling this method multiple times. How about making a pretty_patch_available method for pretty_patch_text and check_pretty_patch to call?

[Dirk Pranke]

Created attachment 89431 [details]
revise check_pretty_patch() to return a list of error strings

[Dirk Pranke]

So, the latest patch revises check_pretty_patch() to return a list of strings to log if prettypatch is not available, and an empty list == success. This works okay in this case, although the naming and return value is a little weird, since you have:

 self._pretty_patch_available = not bool(self._check_pretty_patch()).

There are several other check_XX() routines in the port interface. check_image_diff() takes a flag indicating whether or not to log messages, and returns a bool. check_sys_deps() and check_build() unconditionally call log() internally as they see fit.

It seems like all of the check_* methods should behave similarly. This is complicated by the fact that some of the routines log and fail, and some (like check_build()) might log warnings or informational messages but not fail.

Extending all of these routines so that they return (error_messages, warning_messages, info_messages, sucess_or_failure) gets increasingly wonky. I feel like I'm inventing monads :)

Alternatively, we can pass in a log object (like the first patch did). Or we can pass a flag (like check_image_diff() currently does, in the chromium port). Or we can provide two methods, one that logs, and one that doesn't, for both pretty_patch and image_diff.

Further thoughts?

[Dirk Pranke]

Created attachment 89446 [details]
version using a boolean flag param

[Ojan Vafai]

I was suggesting something more like:
def _pretty_patch_error_message(self):
   try:
       result = self._executive.run_command(['ruby', '--version'])
   except OSError, e:
       if e.errno in [errno.ENOENT, errno.EACCES, errno.ECHILD]:
           return "Ruby is not installed; can't generate pretty patches.")
   
   if not self.path_exists(self._pretty_patch_path):
       return 'Unable to find %s . Can't generate pretty patches.' % self._pretty_patch_path)
   
   return None
   
def check_pretty_patch(self):
   error_message = _pretty_patch_error_message(self)
   if (error_message):
       _log.error(error_message)
       return False
   return True
   
def pretty_patch_available(self):
   return not _pretty_patch_error_message(self)

But whatever, using a bool is good enough.

[Dirk Pranke]

(In reply to comment #8)
> I was suggesting something more like:
> def _pretty_patch_error_message(self):
>    try:
>        result = self._executive.run_command(['ruby', '--version'])
>    except OSError, e:
>        if e.errno in [errno.ENOENT, errno.EACCES, errno.ECHILD]:
>            return "Ruby is not installed; can't generate pretty patches.")
> 
>    if not self.path_exists(self._pretty_patch_path):
>        return 'Unable to find %s . Can't generate pretty patches.' % self._pretty_patch_path)
> 
>    return None
> 
> def check_pretty_patch(self):
>    error_message = _pretty_patch_error_message(self)
>    if (error_message):
>        _log.error(error_message)
>        return False
>    return True
> 
> def pretty_patch_available(self):
>    return not _pretty_patch_error_message(self)
> 
> But whatever, using a bool is good enough.

Ah, that's definitely more readable than my second version.

[Dirk Pranke]

(In reply to comment #7)
> Created an attachment (id=89446) [details]
> version using a boolean flag param

Note that this version should have check_pretty_patch(logging=False) on line 759 (fixed during the commit).

[Dirk Pranke]

Committed r83759: <http://trac.webkit.org/changeset/83759>