Add tests to tools/json_schema_compiler

Add tests to tools/json_schema_compiler

Add tests for different json cases by compiling test jsons and running C++
tests against them. Also fixed bugs where tests failed, removed a dead flag and refactored for readability.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=122781

[calamity, 2012-02-16 06:33:08]

Another day, another large patch. Hopefully this patch is relatively
straightforward; it's mostly tests.

[not at google - send to devlin, 2012-02-18 03:29:02]

This is really awesome. I've skimmed through the Python and it seems fine, maybe
worth going through in more detail.

My general comments are that the tests are *really* awesome, just perhaps a bit
hard to read at a glance, and I think that pulling as much of the generic
data-creation logic into common test helper/util functions would help with that.

And also, test as much as possible (as in, permutations on configurations...
particularly think about how when other people will need to update
json_schema_compiler, what kind of stuff would guard against mistakes).
Definitely try to get 100% coverage over the generated test files. I've only
looked at array in more than glancing detail (no time! ugh sorry, I'll do this
properly on Monday) and that looks pretty cool, but if you can apply the
comments I made in there to other files... cool.

http://codereview.chromium.org/9415001/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/9415001/diff/1/chrome/chrome_tests.gypi#newcod...
chrome/chrome_tests.gypi:2109:
'../tools/json_schema_compiler/test/simple_api_test.cc',
Can these be moved into json_schema_compiler_tests gyp?

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
File tools/json_schema_compiler/test/array.json (right):

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array.json:64: }
test arrays of other things too, like Item / {$ref: Item} ? Just a thought.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
File tools/json_schema_compiler/test/array_test.cc (right):

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array_test.cc:18:
strings_value->Append(Value::CreateStringValue("c"));
You're calling things like this a lot, and presumably in other tests too. I
reckon you should add some kind of test helper that you include, which has
things like

void AppendString(ListValue* value, const std::string& string)
void AppendDouble(ListValue* value, double d)

etc. And then maybe they should be moved into base/values.h

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array_test.cc:33: return value;
Should return a scoped ptr

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array_test.cc:37: scoped_ptr<DictionaryValue>
value(new DictionaryValue());
Heh, scoped_ptr here but not above? Do one or the other for consistency.
Personally I'd be lazy and not worry about having little utils like this use
scoped_ptr locally.

Though this should return a scoped_ptr.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array_test.cc:38:
value->SetWithoutPathExpansion("val", Value::CreateIntegerValue(val));
Can definitely do just
value->SetString("val", val);
here.

(btw, it's not an important point and I wouldn't go around and change everything
to using SetWithoutPathExpansion/GetWIthoutPathExpansion, but when the string
you're passing through is (a) know and (b) we're in a test, it's find to use
Set/Get I think).

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array_test.cc:79: {
If you're only testing 1 atomic unit, probably no need for the additional { }s

[not at google - send to devlin, 2012-02-19 23:19:59]

Did another lightning pass, mostly just so I could say that "don't worry about
it" comment.

(and remember the general nits and stuff that I point out apply wherever else
appropriate too).

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
File tools/json_schema_compiler/test/array_test.cc (right):

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array_test.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
These files should all be _unittest.cc not _test.cc

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array_test.cc:18:
strings_value->Append(Value::CreateStringValue("c"));
On 2012/02/18 03:29:02, kalman wrote:
> You're calling things like this a lot, and presumably in other tests too. I
> reckon you should add some kind of test helper that you include, which has
> things like
> 
> void AppendString(ListValue* value, const std::string& string)
> void AppendDouble(ListValue* value, double d)
> 
> etc. And then maybe they should be moved into base/values.h

Don't worry about this, I'm going to add it to base/values.h.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array_test.cc:63: EXPECT_EQ(1,
ref_array_type->refs[0]->val);
Should test size == 3 too (same elsewhere where appropriate).

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
File tools/json_schema_compiler/test/choices_test.cc (right):

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/choices_test.cc:120: int temp;
Make this scoped between { } like the other tests.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
File tools/json_schema_compiler/test/crossref_test.cc (right):

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/crossref_test.cc:20: return value.release();
.Pass()

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/crossref_test.cc:56: 
unnecessary blank lines

[not at google - send to devlin, 2012-02-20 03:44:24]

Gone over the python now.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cc_g...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:139: )
I think that the "if" and the GeneratePopulatePropertyFromValue stuff can be
pulled out of the required vs optional branches. Like maybe

(c.Append('Value* value_var = NULL')
  .Append('if (!%(src)s->GetWithoutPathExpansion(..)')
  .Append('  return %s;' % ('true' if prop.optional else 'false')
  .Concat(self._Generate..))

or something like that?

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:279: prop: the property the code is
populating..
nit: extra .

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:307: elif prop.type_ ==
PropertyType.OBJECT or prop.type_ == PropertyType.REF:
elsewhere in the code you have this as

prop.type_ in (OBJECT, REF)

I prefer that style.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:413: c.Append('return %s;' %
cpp_util.CreateValueFromSingleProperty(param_copy,
line too long

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cpp_...
File tools/json_schema_compiler/cpp_util.py (right):

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cpp_...
tools/json_schema_compiler/cpp_util.py:56: def GetValueType(prop):
needs comment

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cpp_...
tools/json_schema_compiler/cpp_util.py:87: def
GetConstParameterDeclaration(param, type_):
I know that this method was already here, but it's a bit strange that
(a) it's called GetConstParameterDeclaration (why not
GetSingleParameterDeclaration or something)
(b) only REFs get to be references, while everything else is passed by const
*value*.

It's pretty rare to see that... usually you easier pass by value, or pass by
const reference. The general rule is that you pass objects (including
std::strings) by const reference, and primitives by plain value.

[calamity, 2012-02-20 05:03:37]

Added a couple of array tests, reworked.

http://codereview.chromium.org/9415001/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/9415001/diff/1/chrome/chrome_tests.gypi#newcod...
chrome/chrome_tests.gypi:2109:
'../tools/json_schema_compiler/test/simple_api_test.cc',
On 2012/02/18 03:29:02, kalman wrote:
> Can these be moved into json_schema_compiler_tests gyp?

I moved them and got it to compile, but when running the tests, they weren't
there.... Not sure if I was doing something wrong (built these tests as a static
library).

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cc_g...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:139: )
On 2012/02/20 03:44:24, kalman wrote:
> I think that the "if" and the GeneratePopulatePropertyFromValue stuff can be
> pulled out of the required vs optional branches. Like maybe
> 
> (c.Append('Value* value_var = NULL')
>   .Append('if (!%(src)s->GetWithoutPathExpansion(..)')
>   .Append('  return %s;' % ('true' if prop.optional else 'false')
>   .Concat(self._Generate..))
> 
> or something like that?

I tried that, but it turns out you can't return on an optional property in a
type because you need to check all the other properties too.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:279: prop: the property the code is
populating..
On 2012/02/20 03:44:24, kalman wrote:
> nit: extra .

Done.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:307: elif prop.type_ ==
PropertyType.OBJECT or prop.type_ == PropertyType.REF:
On 2012/02/20 03:44:24, kalman wrote:
> elsewhere in the code you have this as
> 
> prop.type_ in (OBJECT, REF)
> 
> I prefer that style.

Done.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:413: c.Append('return %s;' %
cpp_util.CreateValueFromSingleProperty(param_copy,
On 2012/02/20 03:44:24, kalman wrote:
> line too long

Done.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cpp_...
File tools/json_schema_compiler/cpp_util.py (right):

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cpp_...
tools/json_schema_compiler/cpp_util.py:56: def GetValueType(prop):
On 2012/02/20 03:44:24, kalman wrote:
> needs comment

Done.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cpp_...
tools/json_schema_compiler/cpp_util.py:87: def
GetConstParameterDeclaration(param, type_):
On 2012/02/20 03:44:24, kalman wrote:
> I know that this method was already here, but it's a bit strange that
> (a) it's called GetConstParameterDeclaration (why not
> GetSingleParameterDeclaration or something)
> (b) only REFs get to be references, while everything else is passed by const
> *value*.
> 
> It's pretty rare to see that... usually you easier pass by value, or pass by
> const reference. The general rule is that you pass objects (including
> std::strings) by const reference, and primitives by plain value.

Done.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
File tools/json_schema_compiler/test/array.json (right):

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array.json:64: }
On 2012/02/18 03:29:02, kalman wrote:
> test arrays of other things too, like Item / {$ref: Item} ? Just a thought.

Done.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
File tools/json_schema_compiler/test/array_test.cc (right):

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array_test.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/02/19 23:19:59, kalman wrote:
> These files should all be _unittest.cc not _test.cc

Done.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array_test.cc:18:
strings_value->Append(Value::CreateStringValue("c"));
On 2012/02/19 23:19:59, kalman wrote:
> On 2012/02/18 03:29:02, kalman wrote:
> > You're calling things like this a lot, and presumably in other tests too. I
> > reckon you should add some kind of test helper that you include, which has
> > things like
> > 
> > void AppendString(ListValue* value, const std::string& string)
> > void AppendDouble(ListValue* value, double d)
> > 
> > etc. And then maybe they should be moved into base/values.h
> 
> Don't worry about this, I'm going to add it to base/values.h.

Added a TODO for when your patch goes through.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array_test.cc:33: return value;
On 2012/02/18 03:29:02, kalman wrote:
> Should return a scoped ptr

Done.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array_test.cc:37: scoped_ptr<DictionaryValue>
value(new DictionaryValue());
On 2012/02/18 03:29:02, kalman wrote:
> Heh, scoped_ptr here but not above? Do one or the other for consistency.
> Personally I'd be lazy and not worry about having little utils like this use
> scoped_ptr locally.
> 
> Though this should return a scoped_ptr.

Done.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array_test.cc:38:
value->SetWithoutPathExpansion("val", Value::CreateIntegerValue(val));
On 2012/02/18 03:29:02, kalman wrote:
> Can definitely do just
> value->SetString("val", val);
> here.
> 
> (btw, it's not an important point and I wouldn't go around and change
everything
> to using SetWithoutPathExpansion/GetWIthoutPathExpansion, but when the string
> you're passing through is (a) know and (b) we're in a test, it's find to use
> Set/Get I think).

Done.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array_test.cc:63: EXPECT_EQ(1,
ref_array_type->refs[0]->val);
On 2012/02/19 23:19:59, kalman wrote:
> Should test size == 3 too (same elsewhere where appropriate).

Done.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array_test.cc:79: {
On 2012/02/18 03:29:02, kalman wrote:
> If you're only testing 1 atomic unit, probably no need for the additional { }s

Done.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
File tools/json_schema_compiler/test/choices_test.cc (right):

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/choices_test.cc:120: int temp;
On 2012/02/19 23:19:59, kalman wrote:
> Make this scoped between { } like the other tests.

Done.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
File tools/json_schema_compiler/test/crossref_test.cc (right):

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/crossref_test.cc:20: return value.release();
On 2012/02/19 23:19:59, kalman wrote:
> .Pass()

Done.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/crossref_test.cc:56: 
On 2012/02/19 23:19:59, kalman wrote:
> unnecessary blank lines

Done.

[not at google - send to devlin, 2012-02-20 11:59:25]

lgtm

http://codereview.chromium.org/9415001/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/9415001/diff/1/chrome/chrome_tests.gypi#newcod...
chrome/chrome_tests.gypi:2109:
'../tools/json_schema_compiler/test/simple_api_test.cc',
On 2012/02/20 05:03:37, calamity wrote:
> On 2012/02/18 03:29:02, kalman wrote:
> > Can these be moved into json_schema_compiler_tests gyp?
> 
> I moved them and got it to compile, but when running the tests, they weren't
> there.... Not sure if I was doing something wrong (built these tests as a
static
> library).

Cool, this is fine.

http://codereview.chromium.org/9415001/diff/7002/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cpp_util.py (right):

http://codereview.chromium.org/9415001/diff/7002/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_util.py:89: arg = 'const %(type)s %(name)s'
... and remove the "const" from here.

[calamity, 2012-02-20 23:06:25]

Removed generated files.

http://codereview.chromium.org/9415001/diff/7002/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cpp_util.py (right):

http://codereview.chromium.org/9415001/diff/7002/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_util.py:89: arg = 'const %(type)s %(name)s'
On 2012/02/20 11:59:25, kalman wrote:
> ... and remove the "const" from here.

Done.

[commit-bot: I haz the power, 2012-02-20 23:06:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/9415001/8004

[commit-bot: I haz the power, 2012-02-20 23:06:58]

Presubmit check for 9415001-8004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/common/extensions/api/extension.json

Run the command: svn pset svn:eol-style LF \
  tools/json_schema_compiler/test/array.json \
  tools/json_schema_compiler/test/choices.json \
  tools/json_schema_compiler/test/crossref.json \
  tools/json_schema_compiler/test/simple_api.json

Presubmit checks took 1.3s to calculate.

[commit-bot: I haz the power, 2012-02-20 23:08:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/9415001/3007

[commit-bot: I haz the power, 2012-02-20 23:08:52]

Presubmit check for 9415001-3007 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Run the command: svn pset svn:eol-style LF \
  tools/json_schema_compiler/test/array.json \
  tools/json_schema_compiler/test/choices.json \
  tools/json_schema_compiler/test/crossref.json \
  tools/json_schema_compiler/test/simple_api.json

Presubmit checks took 1.6s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2012-02-20 23:55:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/9415001/4022

[commit-bot: I haz the power, 2012-02-20 23:55:56]

Presubmit check for 9415001-4022 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Run the command: svn pset svn:eol-style LF \
  tools/json_schema_compiler/test/array.json \
  tools/json_schema_compiler/test/choices.json \
  tools/json_schema_compiler/test/crossref.json \
  tools/json_schema_compiler/test/simple_api.json

Presubmit checks took 1.5s to calculate.

[commit-bot: I haz the power, 2012-02-20 23:59:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/9415001/4023

[commit-bot: I haz the power, 2012-02-20 23:59:31]

Presubmit check for 9415001-4023 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Run the command: svn pset svn:eol-style LF \
  tools/json_schema_compiler/test/array.json \
  tools/json_schema_compiler/test/choices.json \
  tools/json_schema_compiler/test/crossref.json \
  tools/json_schema_compiler/test/simple_api.json

Presubmit checks took 1.5s to calculate.

[commit-bot: I haz the power, 2012-02-21 00:03:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/9415001/6011

[commit-bot: I haz the power, 2012-02-21 01:13:08]

Change committed as 122781

[calamity, 2012-02-21 04:09:16]

On 2012/02/21 01:13:08, I haz the power (commit-bot) wrote:
> Change committed as 122781

@yoz: Sorry about submitting before you could have a look at this. My
internship's finishing soon so I wanted to get this in quickly so I could work
on other things. If you have any comments, please let me know and I'll address
them in another patch. Thanks.

[Yoyo Zhou, 2012-02-22 00:58:47]

I like these tests. The only thing that perhaps could use some coverage are
nested ref types (e.g. window, which contains tab).

I don't have any other comments of importance.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cc_g...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:54: for type_ in
self._namespace.types.values():
This could be nested in the if.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:64: for function in
self._namespace.functions.values():
Same here.

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
File tools/json_schema_compiler/test/array.json (right):

http://codereview.chromium.org/9415001/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/array.json:51: "description": "Increments the
given integer.",
?

http://codereview.chromium.org/9415001/diff/6011/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cpp_util.py (right):

http://codereview.chromium.org/9415001/diff/6011/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_util.py:82: """Gets a const parameter declaration
of a given model.Property and its C++
Comment still says const.

http://codereview.chromium.org/9415001/diff/6011/tools/json_schema_compiler/t...
File tools/json_schema_compiler/test/simple_api_unittest.cc (right):

http://codereview.chromium.org/9415001/diff/6011/tools/json_schema_compiler/t...
tools/json_schema_compiler/test/simple_api_unittest.cc:99:
value->RemoveWithoutPathExpansion("number", NULL);
You could just use Remove here (you know the key).