Bug 47466 – new-run-webkit-tests: handle missing ruby/prettypatch better

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

[Dirk Pranke]

new-run-webkit-tests: handle missing ruby/prettypatch better

[Dirk Pranke]

Created attachment 70389 [details]
Patch

[Tony Chang]

Comment on attachment 70389 [details]
Patch

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

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:131
> +    def check_pretty_patch(self, override_step=None, logging=True):

What are the parameters for? override_step doesn't seem to be used and logging is not set by the single caller of this function.

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:673
> +        command = ["ruby", "-I", os.path.dirname(self._pretty_patch_path),
> +                   self._pretty_patch_path, diff_path]

Nit: Use a tuple instead of a list since we don't plan on mutating this.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:122
> +        if not self._pretty_patch_available:
> +            _log.error('')

Why an empty error?  It looks like check_pretty_patch already logs a bunch of stuff, so do we really need this?

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 70389 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70389&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:131
> > +    def check_pretty_patch(self, override_step=None, logging=True):
> 
> What are the parameters for? override_step doesn't seem to be used and logging is not set by the single caller of this function.
> 

This was from chromium.py:_check_file_exists() as a template, but you're right that neither argument is needed. Good catch.

> > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:673
> > +        command = ["ruby", "-I", os.path.dirname(self._pretty_patch_path),
> > +                   self._pretty_patch_path, diff_path]
> 
> Nit: Use a tuple instead of a list since we don't plan on mutating this.
> 

Done.

> > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:122
> > +        if not self._pretty_patch_available:
> > +            _log.error('')
> 
> Why an empty error?  It looks like check_pretty_patch already logs a bunch of stuff, so do we really need this?

It makes the output slightly more legible by putting whitespace in between the up-front checks and the output of the next phase of the run.

[Dirk Pranke]

(In reply to comment #3)
> > > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:122
> > > +        if not self._pretty_patch_available:
> > > +            _log.error('')
> > 
> > Why an empty error?  It looks like check_pretty_patch already logs a bunch of stuff, so do we really need this?
> 
> It makes the output slightly more legible by putting whitespace in between the up-front checks and the output of the next phase of the run.

Hm. I was thinking of a different code path. It would be better if the blank line was next to the other output, so I've moved it there.

[Dirk Pranke]

Created attachment 70582 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 70582 [details]
Patch

Seems we want to do the same magic for wdiff.

Generally looks fine to me.

[Dirk Pranke]

fixed in http://trac.webkit.org/changeset/69820 .