Bug 172958 – check-webkit-style: validate 'specification' values in features.json

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: Tools / Tests

[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.

[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.