Bug 81986 - [check-webkit-style] Alphabetical sorting errors in headers are reported for the line after the first out of order header
Summary: [check-webkit-style] Alphabetical sorting errors in headers are reported for ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-22 16:10 PDT by Martin Robinson
Modified: 2012-03-23 08:44 PDT (History)
6 users (show)

See Also:

Attachments
Patch (13.86 KB, patch)
2012-03-22 16:25 PDT, Martin Robinson 		no flags Details | Formatted Diff | Diff
Patch with a more thorough test (14.44 KB, patch)
2012-03-23 08:32 PDT, Martin Robinson 		no flags 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 Martin Robinson 2012-03-22 16:10:35 PDT 
When check-webkit-style reports an out-of-order header, it reports it for the line of the second header. This is a problem for the style bot, because check-webkit-style filter out errors for all lines, except ones directly modified. An example of this is https://bugs.webkit.org/show_bug.cgi?id=81602 where the style bot did not detect new style errors.
Comment 1 Martin Robinson 2012-03-22 16:25:34 PDT 
Created attachment 133382 [details]
Patch
Comment 2 David Levin 2012-03-22 16:31:34 PDT 
Comment on attachment 133382 [details]
Patch

Where is the test that would have failed before but now passes?
Comment 3 Martin Robinson 2012-03-22 16:36:07 PDT 
(In reply to comment #2)
> (From update of attachment 133382 [details])
> Where is the test that would have failed before but now passes?

There are no end-to-end tests for style checking diffs. Instead the test I've added verifies that the error is first reported for the first of both possible lines. Before this patch the line number would be one greater.
Comment 4 Martin Robinson 2012-03-22 16:38:48 PDT 
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 133382 [details] [details])
> > Where is the test that would have failed before but now passes?
> 
> There are no end-to-end tests for style checking diffs. Instead the test I've added verifies that the error is first reported for the first of both possible lines. Before this patch the line number would be one greater.

Perhaps I could add a patchreader_unittest.py. I'll look into this.
Comment 5 Martin Robinson 2012-03-22 16:39:29 PDT 
Comment on attachment 133382 [details]
Patch

Ack. It exists already. :/
Comment 6 Martin Robinson 2012-03-23 08:32:12 PDT 
Created attachment 133490 [details]
Patch with a more thorough test
Comment 7 David Levin 2012-03-23 08:34:05 PDT 
Comment on attachment 133490 [details]
Patch with a more thorough test

Thanks!
Comment 8 Martin Robinson 2012-03-23 08:36:51 PDT 
(In reply to comment #5)
> (From update of attachment 133382 [details])
> Ack. It exists already. :/

I wasn't able to add a new unit test to patchreader_unittest.py for the cpp.py changes without pulling in the entire style checker stack. Instead I added a more thorough test to cpp_unittest.py which, I think, tests the change completely. The new test verifies that when one line of an ordering error is filtered (a single ordering error theoretically spanning two lines), the error is reported for the other one.

Before my change,  one of the first pair of assertions would fail, so I hope this test addresses your question more directly.
Comment 9 Martin Robinson 2012-03-23 08:37:02 PDT 
Thanks for the review!
Comment 10 Martin Robinson 2012-03-23 08:43:47 PDT 
Committed r111864: <http://trac.webkit.org/changeset/111864>