Bug 172958
| Summary: | check-webkit-style: validate 'specification' values in features.json | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> |
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED WORKSFORME | ||
| Severity: | Normal | CC: | ap, ddkilzer, jond, lforschler, saam, simon.fraser |
| Priority: | P2 | ||
| Version: | WebKit Nightly Build | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=172942 | ||
Simon Fraser (smfr)
We should validate features.json changes so that we don't break webkit.org/status on Keynote Day in the future.
Breaking change:
http://trac.webkit.org/r217805
Fix:
https://trac.webkit.org/r217819
See the more exhaustive checks that JSONCSSPropertiesChecker() does in jsonchecker.py.
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Simon Fraser (smfr)
All assumptions that the JS makes on the web page about the data structure should be validated by check-webkit-style.
David Kilzer (:ddkilzer)
The assumption that was missed in Bug 172942 was that there is a top-level 'specification' object for each 'specification' value in every 'feature'.
I think we should start with that instead of trying list them all exhaustively.
David Kilzer (:ddkilzer)
(In reply to David Kilzer (:ddkilzer) from comment #2)
> The assumption that was missed in Bug 172942 was that there is a top-level
> 'specification' object for each 'specification' value in every 'feature'.
So jsonchecker.py has a check for this exact thing. I wonder why it didn't run?
<https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/style/checkers/jsonchecker.py#L107>
if 'specification' in feature:
if feature['specification'] not in specification_name_set:
self._handle_style_error(0, 'json/syntax', 5, 'The feature "%s" has a specification field but no specification of that name exists.' % feature_name)
David Kilzer (:ddkilzer)
(In reply to David Kilzer (:ddkilzer) from comment #3)
> So jsonchecker.py has a check for this exact thing. I wonder why it didn't
> run?
Runs locally when I specify the file:
$ ./Tools/Scripts/check-webkit-style Source/JavaScriptCore/features.json
ERROR: Source/JavaScriptCore/features.json:0: The features should be sorted alphabetically by name, "Block Scoping" appears after "async/await". [json/syntax] [5]
ERROR: Source/JavaScriptCore/features.json:0: The feature "Object rest/spread" has a specification field but no specification of that name exists. [json/syntax] [5]
Total errors found: 2 in 1 files
And if I back up to the revision before and then reapply the patch locally:
$ git checkout b678b94bdcb57fd1d1ef831097147147910c6b60^
Note: checking out 'b678b94bdcb57fd1d1ef831097147147910c6b60^'.
[...]
$ git diff b678b94bdcb57fd1d1ef831097147147910c6b60^..b678b94bdcb57fd1d1ef831097147147910c6b60 | patch -p1
patching file Source/JavaScriptCore/ChangeLog
patching file Source/JavaScriptCore/features.json
$ ./Tools/Scripts/check-webkit-style
ERROR: Source/JavaScriptCore/features.json:0: The features should be sorted alphabetically by name, "Block Scoping" appears after "async/await". [json/syntax] [5]
ERROR: Source/JavaScriptCore/features.json:0: The feature "Object rest/spread" has a specification field but no specification of that name exists. [json/syntax] [5]
Total errors found: 2 in 2 files
So it seems like check-webkit-patch wasn't run because the patch was landed directly instead of going through the usual process of being posted to Bug 172942 and having check-webkit-style process it.