Bug 76745

Summary: check-webkit-style of the chromium test_expectations.txt file takes too long
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: New BugsAssignee: Ojan Vafai <ojan>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, dpranke, levin, rniwa, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 76749    
Attachments:
Description Flags
Patch
none
Patch dglazkov: review+

Ojan Vafai
Reported 2012-01-20 15:03:05 PST
check-webkit-style of the chromium test_expectations.txt file takes too long
Attachments
Patch (12.10 KB, patch)
2012-01-20 15:07 PST, Ojan Vafai 		no flags
DetailsFormatted DiffDiff
Patch (11.56 KB, patch)
2012-01-20 19:41 PST, Ojan Vafai 		dglazkov: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Ojan Vafai
Comment 1 2012-01-20 15:07:00 PST
Created attachment 123386 [details] Patch
Dimitri Glazkov (Google)
Comment 2 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.
Dimitri Glazkov (Google)
Comment 3 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.
Ojan Vafai
Comment 4 2012-01-20 19:41:49 PST
Created attachment 123428 [details] Patch
Tony Chang
Comment 5 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)?
Dimitri Glazkov (Google)
Comment 6 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.
Ojan Vafai
Comment 7 2012-01-24 12:10:47 PST
Committed r105770: <http://trac.webkit.org/changeset/105770>
Vincent Scheib
Comment 8 2012-01-24 13:09:39 PST
Committed r105789: <http://trac.webkit.org/changeset/105789>
Note You need to log in before you can comment on or make changes to this bug.