Add global presubmit that JSON and IDL files can be parsed.

Add global presubmit that JSON and IDL files can be parsed.

Sometimes, changes to JSON/IDL files make them invalid. It's easier to check these at presubmit time than discovering they can't be parsed at runtime.

This just moves the check from chrome/common/extensions/api to top-level.
This presubmit check excludes files in test/data directories.

BUG=366395
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274432

[Paweł Hajdan Jr., 2014-04-17 09:04:50]

LGTM

Also adding Brett for additions to base/ - he may prefer these files to be
somewhere else. In that case, please recommend a different location that would
work for you.

[Yoyo Zhou, 2014-04-24 16:10:23]

brettw: ping. Can you please review whether these new additions to
base/test/data are okay?

[brettw, 2014-04-24 18:18:19]

Sorry, I don't understand the reason for this change. Can you cover this in the
CL description?

[Yoyo Zhou, 2014-04-24 18:44:25]

Added some commentary to the CL description.

[Yoyo Zhou, 2014-04-24 19:41:04]

cc: iannucci
Brett, there's a bug associated with this, as it turns out.

[Yoyo Zhou, 2014-04-25 20:18:29]

I added another exception to the presubmit check, per discussion on
chromium-dev.

[iannucci1, 2014-04-25 20:31:41]

On 2014/04/25 20:18:29, Yoyo Zhou wrote:
> I added another exception to the presubmit check, per discussion on
> chromium-dev.

I guess I should have poked around more before adding the toplevel check... Do
we have a 'standard' json parsing library which knows how to deal with the
extensions (I suppose the 'comment eater' tool)? My original change was
specifically using the python 'json' library, because the json files in
testing/buildbot are assumed to be straight-up json. I'm a bit worried that this
more-permissive check will enable people to add comments to those files (which
will then break the buildbot side).

Should I add a more-restrictive check for this subdirectory, or should we plan
to change the upstream parsers to allow json-with-comments?

[Paweł Hajdan Jr., 2014-04-28 12:18:05]

On 2014/04/25 20:31:41, iannucci1 wrote:
> Should I add a more-restrictive check for this subdirectory, or should we plan
> to change the upstream parsers to allow json-with-comments?

I'm leaning towards a restrictive check for testing/buildbot .

It seems safest for interoperability to always work with standard json that can
be parsed using most common libraries.

[brettw, 2014-04-28 22:54:41]

I just noticed you moved the IDL one as well. I'm worried about doing this. I
think there are a number of flavors of IDL. Microsoft has one. We have several.
I don't think we can apply a presubmit filter to all idl files. At least it's
well-defined what JSON means so I'm less concerned about that.

[Yoyo Zhou, 2014-04-30 00:48:45]

On 2014/04/28 22:54:41, brettw wrote:
> I just noticed you moved the IDL one as well. I'm worried about doing this. I
> think there are a number of flavors of IDL. Microsoft has one. We have
several.
> I don't think we can apply a presubmit filter to all idl files. At least it's
> well-defined what JSON means so I'm less concerned about that.

I moved the IDL check so that it can also apply to extensions/common/api. But I
understand what you mean.

The following has changed:
- JSON files in ^testing/ are checked without removing comments.
- IDL files are checked only if they're in extensions/common/api or
chrome/common/extensions/api.

[brettw, 2014-04-30 18:18:26]

I think Pawel should check the Python again since it's changed a lot. I'm not so
experienced in this stuff.

I'm super excited about adding files to base since this has nothing to do with
base. base/test is really for the bease tests.

It looks like the other presubmit tests have a way to do mock files with
hardcoded input, and it seems like your data is only 2-4 lines per file so would
also work well with this scheme.

Why did you need to exclude the policy_templates json file?

[Yoyo Zhou, 2014-05-06 01:49:55]

Pawel, can you take another look?

I made the test data inline in PRESUBMIT_test.py. Along the way, I had to modify
idl_schema.py to support stdin, so the IDL error messages now say filename:
"<stdin>": ... instead of filename: filename: ... but I think that's okay.

Antony, feel free to review idl_schema.py changes if you wish.

On 2014/04/30 18:18:26, brettw wrote:
> Why did you need to exclude the policy_templates json file?

It came up in the thread on chromium-dev. Basically, it's not actually json (as
is obvious from the single-quotes and Python comments); it's loaded as Python
source.

[asargent_no_longer_on_chrome, 2014-05-06 04:14:16]

idl_schema.py lgtm

[Yoyo Zhou, 2014-05-14 16:59:32]

Pawel, ping

[Yoyo Zhou, 2014-05-19 22:38:55]

Pawel, ping!

[Paweł Hajdan Jr., 2014-05-20 14:24:31]

LGTM

Feel free to use IM or irc to ping - for long reviews it's harder to follow
them, especially when other reviewers are discussing something.

[Yoyo Zhou, 2014-05-20 18:57:48]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-05-20 18:59:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoz@chromium.org/239283008/50001

[commit-bot: I haz the power, 2014-05-20 19:49:39]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-05-20 19:54:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-20 19:54:04]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[Yoyo Zhou, 2014-06-02 21:30:11]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-06-02 21:31:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoz@chromium.org/239283008/70001

[commit-bot: I haz the power, 2014-06-03 01:37:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-03 08:44:51]

Change committed as 274432