Bug 63597 - nrwt: make the code be consistent about using test names instead of filenames or paths
Summary: nrwt: make the code be consistent about using test names instead of filenames...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-28 19:42 PDT by Dirk Pranke
Modified: 2011-09-06 15:36 PDT (History)
6 users (show)

See Also:


Attachments
Patch (70.85 KB, patch)
2011-06-28 19:46 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.29 MB, application/zip)
2011-06-28 19:58 PDT, WebKit Review Bot
no flags Details
Patch (82.76 KB, patch)
2011-06-30 20:01 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
rebase to HEAD after driver split-out, rename test to test_name where possible (94.43 KB, patch)
2011-07-06 16:49 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-06-28 19:42:37 PDT
nrwt: make the code be consistent about using test names instead of filenames or paths
Comment 1 Dirk Pranke 2011-06-28 19:46:18 PDT
Created attachment 99027 [details]
Patch
Comment 2 Dirk Pranke 2011-06-28 19:47:26 PDT
For your consideration ... I think this change makes most of the NRWT code cleaner and clearer, but I'm not wedded to it.

WDYT?
Comment 3 Dirk Pranke 2011-06-28 19:52:46 PDT
Oh, I should not that while this patch will work as written, it probably still needs some integration tests to make sure we're not breaking anything on windows ports.
Comment 4 WebKit Review Bot 2011-06-28 19:58:00 PDT
Comment on attachment 99027 [details]
Patch

Attachment 99027 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8958084
Comment 5 WebKit Review Bot 2011-06-28 19:58:05 PDT
Created attachment 99028 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 6 Dirk Pranke 2011-06-30 20:01:49 PDT
Created attachment 99421 [details]
Patch
Comment 7 Dirk Pranke 2011-06-30 20:06:51 PDT
it appears that everything works on all the ports, so either the existing tests were good enough for windows or I got lucky. Still need to verify which :)
Comment 8 Eric Seidel (no email) 2011-07-06 14:29:00 PDT
Comment on attachment 99421 [details]
Patch

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:107
> +        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. :)
Comment 9 Dirk Pranke 2011-07-06 14:30:27 PDT
(In reply to comment #8)
> (From update of attachment 99421 [details])
> 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.

Easily done. Thanks!
Comment 10 Dirk Pranke 2011-07-06 16:49:34 PDT
Created attachment 99900 [details]
rebase to HEAD after driver split-out, rename test to test_name where possible
Comment 11 Dirk Pranke 2011-07-06 18:23:10 PDT
Committed r90520: <http://trac.webkit.org/changeset/90520>
Comment 12 Eric Seidel (no email) 2011-09-06 15:36:34 PDT
Comment on attachment 99900 [details]
rebase to HEAD after driver split-out, rename test to test_name where possible

Cleared review? from attachment 99900 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).