| Summary: | new-run-webkit-tests: handle missing ruby/prettypatch better | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||
| Component: | New Bugs | Assignee: | Dirk Pranke <dpranke> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, eric, ojan, tony | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Other | ||||||||
| OS: | OS X 10.5 | ||||||||
| Attachments: |
|
||||||||
|
Description
Dirk Pranke
2010-10-09 23:45:33 PDT
Created attachment 70389 [details]
Patch
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? (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. (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. Created attachment 70582 [details]
Patch
Comment on attachment 70582 [details]
Patch
Seems we want to do the same magic for wdiff.
Generally looks fine to me.
fixed in http://trac.webkit.org/changeset/69820 . |