Bug 76745 - check-webkit-style of the chromium test_expectations.txt file takes too long
Summary: check-webkit-style of the chromium test_expectations.txt file takes too long
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: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks: 76749
  Show dependency tree / graph
 
Reported: 2012-01-20 15:03 PST by Ojan Vafai
Modified: 2012-01-24 13:09 PST (History)
7 users (show)

See Also:

Attachments
Patch (12.10 KB, patch)
2012-01-20 15:07 PST, Ojan Vafai 		no flags Details | Formatted Diff | Diff
Patch (11.56 KB, patch)
2012-01-20 19:41 PST, Ojan Vafai 		dglazkov: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2012-01-20 15:03:05 PST 
check-webkit-style of the chromium test_expectations.txt file takes too long
Comment 1 Ojan Vafai 2012-01-20 15:07:00 PST 
Created attachment 123386 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2012-01-20 15:19:04 PST 
I think the right approach would be to just reuse the model, instead of caching. The model no longer depends on the port/platform against which you check, so it should just be able to take multiple passes. That's how I envisioned it working, anyway.
Comment 3 Dimitri Glazkov (Google) 2012-01-20 15:19:47 PST 
That's why the errors are collected for each line -- so that you could review them all once you ran them against all ports.
Comment 4 Ojan Vafai 2012-01-20 19:41:49 PST 
Created attachment 123428 [details]
Patch
Comment 5 Tony Chang 2012-01-20 23:18:22 PST 
Comment on attachment 123428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123428&action=review

I'll let dglazkov do the actual review.  Just some style nits.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:795
>          if len(errors) or len(warnings):

Nit: I think you can remove the len() calls.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:799
>              if len(errors):

Ditto.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:220
> +    def test_error_on_different_platform(self):

Do we have a test that checks different configurations (e.g., DEBUG vs RELEASE)?
Comment 6 Dimitri Glazkov (Google) 2012-01-24 11:33:39 PST 
Comment on attachment 123428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123428&action=review

>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:220
>> +    def test_error_on_different_platform(self):
> 
> Do we have a test that checks different configurations (e.g., DEBUG vs RELEASE)?

I think it might be good to capture just a few more expectations here, just in case the machinery in layout_tests/models is refactored away or something.
Comment 7 Ojan Vafai 2012-01-24 12:10:47 PST 
Committed r105770: <http://trac.webkit.org/changeset/105770>
Comment 8 Vincent Scheib 2012-01-24 13:09:39 PST 
Committed r105789: <http://trac.webkit.org/changeset/105789>