Bug 96136

Summary: Regression: check-webkit-style no longer checks TestExpectations for syntax errors
Product: WebKit Reporter: Tony Chang <tony>
Component: New BugsAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, levin, ojan, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Tony Chang 2012-09-07 12:00:05 PDT 
Regression: check-webkit-style no longer checks TestExpectations for syntax errors
Comment 1 Tony Chang 2012-09-07 12:02:39 PDT 
Created attachment 162838 [details]
Patch
Comment 2 Tony Chang 2012-09-07 12:03:21 PDT 
Dirk, what was the overrides param for? Should I put the FIXME back?
Comment 3 Ojan Vafai 2012-09-07 12:09:36 PDT 
Comment on attachment 162838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162838&action=review

> Tools/Scripts/webkitpy/style/checkers/test_expectations.py:-80
> -        # FIXME: we should pass in the filenames here if possible, and ensure
> -        # that this works with with cascading expectations files and remove the overrides param.

As discussed in person, I think with cascading expectations we want to lint each file in isolation and so doing nothing special is correct. I'll r+ for now to get the linter running again. Dirk can clarify if I'm wrong when he returns from vacation.
Comment 4 Tony Chang 2012-09-07 12:13:04 PDT 
Comment on attachment 162838 [details]
Patch

Removing the overrides check should just make this check more permissive, which is still better than not running it at all. If we need to do something with overrides, we can do that in a follow up.
Comment 5 WebKit Review Bot 2012-09-07 13:11:38 PDT 
Comment on attachment 162838 [details]
Patch

Clearing flags on attachment: 162838

Committed r127910: <http://trac.webkit.org/changeset/127910>
Comment 6 WebKit Review Bot 2012-09-07 13:11:41 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 7 Dirk Pranke 2012-09-07 13:39:03 PDT 
I seem to recall the overrides check in the style checker was something of a kludge; as you could see, all we did was cat the two strings together.

The TestExpectationsParser class is only set up to parse one file at a time and so has no concept of the cascade. As such, using this approach misses out on some cross-file semantic warnings, but it's probably good enough for now. 

If we wanted we could get fancier and pass in a full OrderedDict of the cascades, but I don't think there's a real need for it.

LGTM.
Comment 8 Ojan Vafai 2012-09-07 15:23:27 PDT 
This misses when tests are duplicated between TestExpectations and Skipped files. "run-webkit-tests --lint-test-files" warns for those cases. It's not a big deal. Hopefully we just won't have Skipped files soon.