Bug 60466

Summary:

check-webkit-style can miss XML syntax and test_expectations.txt errors when checking patches

Product:

WebKit

Reporter:

Adam Roben (:aroben) <aroben>

Component:

Tools / Tests

Assignee:

David Levin <levin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, commit-queue, dpranke, enne, eric, hamaji, levin

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none

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	Details Formatted Diff Diff
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.