Bug 60466 - check-webkit-style can miss XML syntax and test_expectations.txt errors when checking patches
Summary: check-webkit-style can miss XML syntax and test_expectations.txt errors when ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
URL:
Keywords:
: 60373 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-05-09 07:06 PDT by Adam Roben (:aroben)
Modified: 2012-12-11 12:46 PST (History)
7 users (show)

See Also:


Attachments
Patch (11.48 KB, patch)
2011-05-09 13:31 PDT, David Levin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2011-05-09 07:06:49 PDT
Attachment 92746 [details] in bug 54181 contained XML syntax errors in WebCore.vcproj. check-webkit-style didn't flag those errors, so the buggy patch was committed in r86047. I had to land a fix in r86052 to fix them.

We should figure out why check-webkit-style didn't flag the errors.
Comment 1 Adam Roben (:aroben) 2011-05-09 07:23:01 PDT
Note that check-webkit-style *does* flag the errors if run on the r86047 version of WebCore.vcproj. It just doesn't flag the errors when run on the patch.
Comment 2 David Levin 2011-05-09 07:31:51 PDT
(In reply to comment #0)
> Attachment 92746 [details] in bug 54181 contained XML syntax errors in WebCore.vcproj. check-webkit-style didn't flag those errors, so the buggy patch was committed in r86047. I had to land a fix in r86052 to fix them.
> 
> We should figure out why check-webkit-style didn't flag the errors.

I suspect this has to do with the fact that it filters to only be on the lines where changes were made. In general, this is really good, but for some checkers like the xml one, it probably shouldn't happen.
Comment 3 David Levin 2011-05-09 13:08:50 PDT
(In reply to comment #2)
> (In reply to comment #0)
> > Attachment 92746 [details] [details] in bug 54181 contained XML syntax errors in WebCore.vcproj. check-webkit-style didn't flag those errors, so the buggy patch was committed in r86047. I had to land a fix in r86052 to fix them.
> > 
> > We should figure out why check-webkit-style didn't flag the errors.
> 
> I suspect this has to do with the fact that it filters to only be on the lines where changes were made. In general, this is really good, but for some checkers like the xml one, it probably shouldn't happen.

Yep, that was it. Patching coming shortly. The test expectations checker had the same issue.
Comment 4 David Levin 2011-05-09 13:23:50 PDT
*** Bug 60373 has been marked as a duplicate of this bug. ***
Comment 5 David Levin 2011-05-09 13:31:55 PDT
Created attachment 92839 [details]
Patch
Comment 6 Adam Roben (:aroben) 2011-05-09 13:33:17 PDT
Changed the title back to describing a symptom, not a cause.
Comment 7 Shinichiro Hamaji 2011-05-10 09:44:09 PDT
Comment on attachment 92839 [details]
Patch

Looks good
Comment 8 WebKit Commit Bot 2011-05-10 10:28:19 PDT
Comment on attachment 92839 [details]
Patch

Clearing flags on attachment: 92839

Committed r86165: <http://trac.webkit.org/changeset/86165>
Comment 9 WebKit Commit Bot 2011-05-10 10:28:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Dirk Pranke 2012-12-11 12:45:45 PST
Just for the record, it doesn't seem to still make sense to check the whole file for TestExpectations changes, so I've restored the line filtering in bug 104701 ... see the comments in that bug for more context.
Comment 11 Dirk Pranke 2012-12-11 12:46:11 PST
Argh, bug 104687, not bug 104701.