Bug 96136 – Regression: check-webkit-style no longer checks TestExpectations for syntax errors

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

Regression: check-webkit-style no longer checks TestExpectations for syntax errors

[Tony Chang]

Created attachment 162838 [details]
Patch

[Tony Chang]

Dirk, what was the overrides param for? Should I put the FIXME back?

[Ojan Vafai]

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.

[Tony Chang]

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.

[WebKit Review Bot]

Comment on attachment 162838 [details]
Patch

Clearing flags on attachment: 162838

Committed r127910: <http://trac.webkit.org/changeset/127910>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

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.

[Ojan Vafai]

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.