Bug 58295 - new-run-webkit-tests: suppress extraneous pretty patch warnings
Summary: new-run-webkit-tests: suppress extraneous pretty patch warnings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-11 18:53 PDT by Dirk Pranke
Modified: 2011-04-13 13:58 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.22 KB, patch)
2011-04-11 18:55 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
revise check_pretty_patch() to return a list of error strings (6.16 KB, patch)
2011-04-13 12:17 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
version using a boolean flag param (5.21 KB, patch)
2011-04-13 13:46 PDT, Dirk Pranke
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-04-11 18:53:16 PDT
new-run-webkit-tests: suppress extraneous pretty patch warnings
Comment 1 Dirk Pranke 2011-04-11 18:55:04 PDT
Created attachment 89144 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-04-12 11:00:43 PDT
Comment on attachment 89144 [details]
Patch

Doesn't python logging already have built in ways to filter based on where the messages came from?
Comment 3 Dirk Pranke 2011-04-12 11:28:27 PDT
(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?
Comment 4 Ojan Vafai 2011-04-13 09:56:57 PDT
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?
Comment 5 Dirk Pranke 2011-04-13 12:17:03 PDT
Created attachment 89431 [details]
revise check_pretty_patch() to return a list of error strings
Comment 6 Dirk Pranke 2011-04-13 12:40:25 PDT
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?
Comment 7 Dirk Pranke 2011-04-13 13:46:18 PDT
Created attachment 89446 [details]
version using a boolean flag param
Comment 8 Ojan Vafai 2011-04-13 13:50:00 PDT
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.
Comment 9 Dirk Pranke 2011-04-13 13:53:47 PDT
(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.
Comment 10 Dirk Pranke 2011-04-13 13:57:46 PDT
(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).
Comment 11 Dirk Pranke 2011-04-13 13:58:23 PDT
Committed r83759: <http://trac.webkit.org/changeset/83759>