Bug 35265 - new-run-webkit-tests --platform=mac-leopard diffs are backwards
Summary: new-run-webkit-tests --platform=mac-leopard diffs are backwards
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 35365 34984
  Show dependency treegraph
 
Reported: 2010-02-22 15:24 PST by Eric Seidel (no email)
Modified: 2010-03-01 18:59 PST (History)
1 user (show)

See Also:


Attachments
Patch (3.99 KB, patch)
2010-02-24 14:36 PST, Eric Seidel (no email)
levin: review+
levin: commit-queue-
Details | Formatted Diff | Diff
posting a revised patch since Eric is OOO (5.95 KB, patch)
2010-02-26 16:21 PST, Dirk Pranke
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2010-02-22 15:24:58 PST
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.
Comment 1 Eric Seidel (no email) 2010-02-24 14:36:20 PST
Created attachment 49441 [details]
Patch
Comment 2 Dirk Pranke 2010-02-24 14:47:16 PST
change looks good but we should make this consistent across the board.
Comment 3 Eric Seidel (no email) 2010-02-24 15:58:55 PST
I've filed bug 35365 about fixing this sort of issue globally.
Comment 4 David Levin 2010-02-25 09:03:00 PST
Comment on attachment 49441 [details]
Patch

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> +2010-02-24  Eric Seidel  <eric@webkit.org>
> +
> +        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).
Comment 5 Eric Seidel (no email) 2010-02-25 14:11:03 PST
(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?
Comment 6 Dirk Pranke 2010-02-25 15:53:18 PST
> 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.
Comment 7 Dirk Pranke 2010-02-26 16:21:44 PST
Created attachment 49664 [details]
posting a revised patch since Eric is OOO
Comment 8 David Levin 2010-03-01 10:34:17 PST
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.
Comment 9 Dirk Pranke 2010-03-01 11:36:32 PST
Committed r55372: <http://trac.webkit.org/changeset/55372>