Bug 35265 – new-run-webkit-tests --platform=mac-leopard diffs are backwards

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Eric Seidel (no email)]

run-chromium-webkit-tests --platform=mac-leopard diffs are backwards

--- /tmp/run-chromium-webkit-tests-layout-test-results/http/tests/security/local-user-CSS-from-remote-actual.txt 
+++ /tmp/run-chromium-webkit-tests-layout-test-results/http/tests/security/local-user-CSS-from-remote-expected.txt 

run-webkit-tests diffs the other way.  + lines are "actual" instead of expected.

This confuses me to no end.

[Eric Seidel (no email)]

Created attachment 49441 [details]
Patch

[Dirk Pranke]

change looks good but we should make this consistent across the board.

[Eric Seidel (no email)]

I've filed bug 35365 about fixing this sort of issue globally.

[David Levin]

Comment on attachment 49441 [details]
Patch

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> [phone]  Eric Seidel  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        run-chromium-webkit-tests --platform=mac-leopard diffs are backwards
> +        https://bugs.webkit.org/show_bug.cgi?id=35265
> +
> +        Some parts of the code passed arguments as
> +        "actual, expected" and some passed as "expected, actual".
> +        As you might imagine, this lead to great confusion and wrongness.
> +        Standardize on "expected, actual" as that't the order which is

typo: that't

> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py
> -    def diff_text(self, actual_text, expected_text,
> -                  actual_filename, expected_filename):
> +    def diff_text(self, expected_text, actual_text,
> +            expected_filename, actual_filename):
It would be nice to align to the ( of the previous line (as was done before).

[Eric Seidel (no email)]

(In reply to comment #4)
> > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py
> > -    def diff_text(self, actual_text, expected_text,
> > -                  actual_filename, expected_filename):
> > +    def diff_text(self, expected_text, actual_text,
> > +            expected_filename, actual_filename):
> It would be nice to align to the ( of the previous line (as was done before).

I thought "we" as webkit avoided that because you end up having to re-indent all the time.  I went with the "just indent one level" model.  Does PEP8 say anything on this subject?

[Dirk Pranke]

> I thought "we" as webkit avoided that because you end up having to re-indent
> all the time.  I went with the "just indent one level" model.  Does PEP8 say
> anything on this subject?

PEP 8 says "Make sure to indent the continued line appropriately", but examples show function arguments and parenthesized expressions being aligned. Google's Python style guide says to align vertically and, failing that, to align w/ a hanging indent.

[Dirk Pranke]

Created attachment 49664 [details]
posting a revised patch since Eric is OOO

[David Levin]

Comment on attachment 49664 [details]
posting a revised patch since Eric is OOO

> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py
> +    def diff_text(self, expected_text, actual_text,
> +                  expected_filename, actual_filename):

> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/test.py
> +    def diff_text(self, expected_text, actual_text,
> +            expected_filename, actual_filename):

fwiw, this indent doesn't align with the (. Note that there are many examples even within this patch of the alignment happening.

[Dirk Pranke]

Committed r55372: <http://trac.webkit.org/changeset/55372>