Bug 81986 – [check-webkit-style] Alphabetical sorting errors in headers are reported for the line after the first out of order header

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 133382 [details]
Patch

[David Levin]

Comment on attachment 133382 [details]
Patch

Where is the test that would have failed before but now passes?

[Martin Robinson]

(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.

[Martin Robinson]

(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.

[Martin Robinson]

Comment on attachment 133382 [details]
Patch

Ack. It exists already. :/

[Martin Robinson]

Created attachment 133490 [details]
Patch with a more thorough test

[David Levin]

Comment on attachment 133490 [details]
Patch with a more thorough test

Thanks!

[Martin Robinson]

(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.

[Martin Robinson]

Thanks for the review!

[Martin Robinson]

Committed r111864: <http://trac.webkit.org/changeset/111864>