RESOLVED FIXED Bug 60466
check-webkit-style can miss XML syntax and test_expectations.txt errors when checking patches 
https://bugs.webkit.org/show_bug.cgi?id=60466
Summary check-webkit-style can miss XML syntax and test_expectations.txt errors when ...
Adam Roben (:aroben)
Reported 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.
Attachments
Patch (11.48 KB, patch)
2011-05-09 13:31 PDT, David Levin 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Adam Roben (:aroben)
Comment 1 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.
David Levin
Comment 2 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.
David Levin
Comment 3 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.
David Levin
Comment 4 2011-05-09 13:23:50 PDT
*** Bug 60373 has been marked as a duplicate of this bug. ***
David Levin
Comment 5 2011-05-09 13:31:55 PDT
Created attachment 92839 [details] Patch
Adam Roben (:aroben)
Comment 6 2011-05-09 13:33:17 PDT
Changed the title back to describing a symptom, not a cause.
Shinichiro Hamaji
Comment 7 2011-05-10 09:44:09 PDT
Comment on attachment 92839 [details] Patch Looks good
WebKit Commit Bot
Comment 8 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>
WebKit Commit Bot
Comment 9 2011-05-10 10:28:24 PDT
All reviewed patches have been landed. Closing bug.
Dirk Pranke
Comment 10 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.
Dirk Pranke
Comment 11 2012-12-11 12:46:11 PST
Argh, bug 104687, not bug 104701.
Note You need to log in before you can comment on or make changes to this bug.