[webkit-reviews] review granted: [Bug 63597] nrwt: make the code be consistent about using test names instead of filenames or paths : [Attachment 99421] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 63597: nrwt: make the code be consistent about using test names instead of
filenames or paths
https://bugs.webkit.org/show_bug.cgi?id=63597

Attachment 99421: Patch
https://bugs.webkit.org/attachment.cgi?id=99421&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=99421&action=review

LGTM.  Please make some effort to standardize on test_name where possible
before landing.  Obviously don't go crazy changing all sorts of existing code,
but certainly the new stuff should be the same name everywehre.

> Tools/ChangeLog:12
> +	   It can be hard to figure out where in the code we're assuming
> +	   test names are listed as unix-style relative filenames and where
> +	   they are either absolute paths or relative paths following the
> +	   host filesystem convention.

I agree!

> Tools/ChangeLog:17
> +	   This patch changes things so that everything outside of the
> +	   Port object uses (and must assume) unix-style relative
> +	   filenames (with one exception, which is specifying host-local
> +	   filenames as a list of test arguments on the command line).

Yay!

>
Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:10
7
> +	   self.test = test

I think using test_name consistently would be more clear than "test".  Reading
the code I might think "test" was some sort of test object.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:471
> +    def normalize_test_name(self, test):

You use test_name here, but "test" and testname other places.  I think we
should standardize on test_name everywhere for clarity.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:923
> +	     test: name of the test

If this was test_name we wouldn't even need this explanation. :)