172958 2017-06-05 22:00:08 -0700 check-webkit-style: validate 'specification' values in features.json 2017-06-06 10:23:04 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified RESOLVED WORKSFORME https://bugs.webkit.org/show_bug.cgi?id=172942 P2 Normal --- 1 simon.fraser webkit-unassigned ap ddkilzer jond lforschler saam simon.fraser oldest_to_newest 1315898 0 simon.fraser 2017-06-05 22:00:08 -0700 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. 1315899 1 simon.fraser 2017-06-05 22:00:59 -0700 All assumptions that the JS makes on the web page about the data structure should be validated by check-webkit-style. 1316032 2 ddkilzer 2017-06-06 09:42:20 -0700 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. 1316050 3 ddkilzer 2017-06-06 10:01:35 -0700 (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) 1316057 4 ddkilzer 2017-06-06 10:23:04 -0700 (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.