Bug 37406 – Add PrettyPatch links to new-run-webkit-tests output

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

[Eric Seidel (no email)]

Add PrettyPatch links to new-run-webkit-tests output

[Eric Seidel (no email)]

Created attachment 53079 [details]
Patch

[Adam Barth]

Comment on attachment 53079 [details]
Patch

Ok.  I miss the pretty diffs.  Glad it's coming back.

[WebKit Commit Bot]

Comment on attachment 53079 [details]
Patch

Clearing flags on attachment: 53079

Committed r57446: <http://trac.webkit.org/changeset/57446>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

Comment on attachment 53079 [details]
Patch

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index b1e0733..33893f6 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,27 @@
> [phone]  Eric Seidel  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add PrettyPatch links to new-run-webkit-tests output
> +        https://bugs.webkit.org/show_bug.cgi?id=37406
> +

Does this patch introduce a dependency on Ruby into the test system? I don't think we have
Ruby on Chromium windows (and am not sure about Linux, although it's probably there).

Also, do we really need three different kinds of diffs?

> @@ -519,6 +519,7 @@ class Port(object):
>                 '--end-insert=##WDIFF_END##',
>                 actual_filename,
>                 expected_filename]
> +        # FIXME: Why not just check os.exists(executable) once?
>          global _wdiff_available

See the comments in base.py's Port.wdiff_text()

[Eric Seidel (no email)]

I need to add a check for Ruby, as it looks like the Linux canary doesn't have ruby.

Personally I find PrettyPatch diffs prettier than wdiff. :)  wdiff doesn't exist on Mac systems, and more importantly is the same prettydiff that we use on bugs.webkti.org (the "Formatted Diff" link next to any patch is a PrettyPatch diff).

[Dirk Pranke]

(In reply to comment #6)
> I need to add a check for Ruby, as it looks like the Linux canary doesn't have
> ruby.
> 
> Personally I find PrettyPatch diffs prettier than wdiff. :)  wdiff doesn't
> exist on Mac systems, and more importantly is the same prettydiff that we use
> on bugs.webkti.org (the "Formatted Diff" link next to any patch is a
> PrettyPatch diff).

Sorry, I should have been clearer; I'm not wedded to wdiff either, except that it shows you intraline diffs, and I don't know that anyone else is, either. Perhaps we should replace wdiff with PrettyPatch, instead of having three diffs?

[Adam Barth]

> Perhaps we should replace wdiff with PrettyPatch, instead of having three
> diffs?

That sounds like a good plan to me.  That's consistent with how the tool has worked historically and with how bugs.webkit.org works.

[Eric Seidel (no email)]

I wont be able to fix the script for pllatforms missing Ruby until tomorrow morning. If that's blocking the Gardner from getting work done, feel free to roll this out.

[Eric Seidel (no email)]

Fixed systems missing ruby in bug 37441.