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

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.