Bug 96136 - Regression: check-webkit-style no longer checks TestExpectations for syntax errors
Summary: Regression: check-webkit-style no longer checks TestExpectations for syntax e...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-07 12:00 PDT by Tony Chang
Modified: 2012-09-07 15:23 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.86 KB, patch)
2012-09-07 12:02 PDT, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.