Add optional schema compiler error messages

Add optional schema compiler error messages and unit tests.
Provides the ability to generate error messages within schema-compiled code for ease of debugging. 
Error messages may be disabled by adding a 'generate_error_messages' property to json schema, e.g. "generate_error_messages": false

Error generation mostly written by Aaron Jacobs (https://codereview.chromium.org/16462004/).

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

[dhnishi (use Chromium), 2013-08-08 21:31:05]

@kalman

Got the unit tests built! It's currently passing all of the JSON Schema Compiler
unit tests and I'm currently running the rest of the test suite to check the
others right now.

A couple of notes about this patch:
1. One of the errors, the one about expecting a list, is literally impossible to
fail, so I didn't test it. To fail, it'd need this function to return false
(https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&sq=...).
Should this error be removed or should we keep it just in case?

2. I currently set the default behavior as to generate error checking for all
files unless specified otherwise by the compiler options. Aaron's patch had it
set to not generate error checking, unless specified by the compiler_options.

3. Besides the error checking noted in #1, all of the other error generating
blocks are tested with both a passing and failing test case, separated by {}. 

I know you're busy right now with being a sheriff and other work, so just PTAL
when you get a free moment! I don't want to load too much onto you when you have
higher priority things to get around to.

Thanks,
Daniel Nishi
Cal Poly Extensions Team

[not at google - send to devlin, 2013-08-08 23:12:07]

On 2013/08/08 21:31:05, Daniel Nishi wrote:
> @kalman
> 
> Got the unit tests built! It's currently passing all of the JSON Schema
Compiler
> unit tests and I'm currently running the rest of the test suite to check the
> others right now.
> 
> A couple of notes about this patch:
> 1. One of the errors, the one about expecting a list, is literally impossible
to
> fail, so I didn't test it. To fail, it'd need this function to return false
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&sq=...).
> Should this error be removed or should we keep it just in case?

Hah ok. No point generating a string for it then, though, generating some kind
of assertion would be good.

> 
> 2. I currently set the default behavior as to generate error checking for all
> files unless specified otherwise by the compiler options. Aaron's patch had it
> set to not generate error checking, unless specified by the compiler_options.

I'd prefer this were off by default so that we don't generate a lot of code that
we don't end up using. I think that generating error strings for 90 APIs, some
of which are huge, may add up (though I suppose you could easily measure this).

In any case, error strings aren't very useful for APIs, only for those
structures which we actually expose to developers (and the JS errors are
implemented in JS).

> 
> 3. Besides the error checking noted in #1, all of the other error generating
> blocks are tested with both a passing and failing test case, separated by {}. 
> 
> I know you're busy right now with being a sheriff and other work, so just PTAL
> when you get a free moment! I don't want to load too much onto you when you
have
> higher priority things to get around to.

I can do it now :)

> 
> Thanks,
> Daniel Nishi
> Cal Poly Extensions Team

[dhnishi (use Chromium), 2013-08-08 23:30:34]

Removed the error generation for the expected list test. Replaced with a DCHECK.

Flipped the booleans back to make it off by default.

[not at google - send to devlin, 2013-08-09 00:08:49]

awesome. just reviewing the tests mostly here because it finally lets me see
these error messages in the wild.

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
File tools/json_schema_compiler/test/error_generation_unittest.cc (right):

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:17:
value->SetWithoutPathExpansion("string", Value::CreateStringValue("bling"));
there is a utility in test_util.h for this sort of stuff:

scoped_ptr<DictionaryValue> value = test_util::Dictionary(
    "string", new StringValue("bling"));

there are a few other places in this file where you could do a similar cleanup.

and yeah, new StringValue not CreateStringValue, the latter is deprecated I
believe. at the very least, it's longer.

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:18:
scoped_ptr<TestType> testType(new TestType());
test_type, and also, just declare on stack.

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:27:
EXPECT_FALSE(error.compare("expected dictionary, got binary"));
EXPECT_EQ is better, gives you nice test messages.

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:47:
EXPECT_FALSE(error.compare("unexpected type, got binary"));
this does look a bit weird, and we should be able to generate a string which
enumerates the choice types here. "expected string or array of strings, got
binary" - or whatever it is.

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:82:
params_value->Append(Value::CreateIntegerValue(5));
and here (and above, etc):

scoped_ptr<ListValue> params_value = test_util::Array(
    new FundamentalValue(5), new FundamentalValue(5));

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:167:
params_value->Append(Value::CreateBooleanValue(6));
6? that's just going to get coerced into true. Doesn't seem much different to
the next test.

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:173:
EXPECT_FALSE(error.compare("unable to populate array 'integers'"));
how hard would it be for this to say like "'integers': unsupported type at index
1, expected integer, got boolean'.

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:205:
EXPECT_FALSE(error.compare("'data': expected BinaryValue, got number"));
s/BinaryValue/binary/

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:214:
scoped_ptr<HasEnumeration> hasEnumeration(new HasEnumeration());
has_enumeration

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:231:
EXPECT_FALSE(error.compare("got bad enum value from 'enumeration'"));
how about

'enumeration': expected "one" or "two" or "three", got "bad sauce".

[dhnishi (use Chromium), 2013-08-09 17:59:50]

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
File tools/json_schema_compiler/test/error_generation_unittest.cc (right):

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:17:
value->SetWithoutPathExpansion("string", Value::CreateStringValue("bling"));
On 2013/08/09 00:08:49, kalman wrote:
> there is a utility in test_util.h for this sort of stuff:
> 
> scoped_ptr<DictionaryValue> value = test_util::Dictionary(
>     "string", new StringValue("bling"));
> 
> there are a few other places in this file where you could do a similar
cleanup.
> 
> and yeah, new StringValue not CreateStringValue, the latter is deprecated I
> believe. at the very least, it's longer. I'll open another CL for in places
other than the tests I'm adding.

You are correct. Just found bug 160596 which is all about eradicating the use of
the deprecated one. I'll remove all uses of it that I can find in there.

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:18:
scoped_ptr<TestType> testType(new TestType());
On 2013/08/09 00:08:49, kalman wrote:
> test_type, and also, just declare on stack.

Done.

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:27:
EXPECT_FALSE(error.compare("expected dictionary, got binary"));
On 2013/08/09 00:08:49, kalman wrote:
> EXPECT_EQ is better, gives you nice test messages.

Done.

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:47:
EXPECT_FALSE(error.compare("unexpected type, got binary"));
On 2013/08/09 00:08:49, kalman wrote:
> this does look a bit weird, and we should be able to generate a string which
> enumerates the choice types here. "expected string or array of strings, got
> binary" - or whatever it is.

It formats arrays by just pluralizing it, but I've got it listing the types now.

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:82:
params_value->Append(Value::CreateIntegerValue(5));
On 2013/08/09 00:08:49, kalman wrote:
> and here (and above, etc):
> 
> scoped_ptr<ListValue> params_value = test_util::Array(
>     new FundamentalValue(5), new FundamentalValue(5));

Done.

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:167:
params_value->Append(Value::CreateBooleanValue(6));
On 2013/08/09 00:08:49, kalman wrote:
> 6? that's just going to get coerced into true. Doesn't seem much different to
> the next test.

Whoops. I originally was adding an Integer value there, but later changed to
Boolean. Looks like the 6 never got changed to a Boolean, though.

Done.

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:173:
EXPECT_FALSE(error.compare("unable to populate array 'integers'"));
On 2013/08/09 00:08:49, kalman wrote:
> how hard would it be for this to say like "'integers': unsupported type at
index
> 1, expected integer, got boolean'.

The generated code itself doesn't have enough info to know where the population
broke (PopulateOptionalArrayFromList only returns a bool), so I don't think we
can get more info unless we moved the error string setting into
json_schema_compiler::util.

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:205:
EXPECT_FALSE(error.compare("'data': expected BinaryValue, got number"));
On 2013/08/09 00:08:49, kalman wrote:
> s/BinaryValue/binary/

Done.

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:214:
scoped_ptr<HasEnumeration> hasEnumeration(new HasEnumeration());
On 2013/08/09 00:08:49, kalman wrote:
> has_enumeration 

In-lined.

https://codereview.chromium.org/22228002/diff/20001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:231:
EXPECT_FALSE(error.compare("got bad enum value from 'enumeration'"));
On 2013/08/09 00:08:49, kalman wrote:
> how about
> 
> 'enumeration': expected "one" or "two" or "three", got "bad sauce".

Done.

[not at google - send to devlin, 2013-08-09 18:20:58]

lgtm

https://codereview.chromium.org/22228002/diff/49001/tools/json_schema_compile...
File tools/json_schema_compiler/cc_generator.py (right):

https://codereview.chromium.org/22228002/diff/49001/tools/json_schema_compile...
tools/json_schema_compiler/cc_generator.py:205: ', got " + ' +
use %s substitutions for anything more than a single + in python.

https://codereview.chromium.org/22228002/diff/49001/tools/json_schema_compile...
File tools/json_schema_compiler/test/error_generation_unittest.cc (right):

https://codereview.chromium.org/22228002/diff/49001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:27:
EXPECT_FALSE(TestType::Populate(*value, new TestType(), &error));
(new TestType()) looks leaky... other tests like this too.

my imagination is going wild thinking about how to write a templatised version
of this. it might be totally bad style, but something like:

template <typename T>
std::string GetPopulateError(const base::Value& value) {
  std::string error;
  T test_type;
  T::Populate(value, &test_type, &error);
  return error;
}

then here

scoped_pre<base::BinaryValue> value(new base::BinaryValue());
EXPECT_EQ("expected dictionary, got binary",
          GetPopulateError<TestType>(*value));

and below

scoped_ptr<base::BinaryValue> bad_value(new base::BinaryValue());
EXPECT_EQ("expected integers or integer, got binary",
          GetPopulateError<ChoiceType::Integers>(*bad_value));

etc.

https://codereview.chromium.org/22228002/diff/49001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:112:
EXPECT_EQ(error, "'string': expected std::string, got number");
s/std::string/string/

[dhnishi (use Chromium), 2013-08-09 20:50:48]

https://codereview.chromium.org/22228002/diff/49001/tools/json_schema_compile...
File tools/json_schema_compiler/test/error_generation_unittest.cc (right):

https://codereview.chromium.org/22228002/diff/49001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:27:
EXPECT_FALSE(TestType::Populate(*value, new TestType(), &error));
On 2013/08/09 18:20:58, kalman wrote:
> (new TestType()) looks leaky... other tests like this too.
> 
> my imagination is going wild thinking about how to write a templatised version
> of this. it might be totally bad style, but something like:
> 
> template <typename T>
> std::string GetPopulateError(const base::Value& value) {
>   std::string error;
>   T test_type;
>   T::Populate(value, &test_type, &error);
>   return error;
> }
> 
> then here
> 
> scoped_pre<base::BinaryValue> value(new base::BinaryValue());
> EXPECT_EQ("expected dictionary, got binary",
>           GetPopulateError<TestType>(*value));
> 
> and below
> 
> scoped_ptr<base::BinaryValue> bad_value(new base::BinaryValue());
> EXPECT_EQ("expected integers or integer, got binary",
>           GetPopulateError<ChoiceType::Integers>(*bad_value));
> 
> etc.

Valgrind suggests an 4-16 byte leak for all of those instances. Acceptable or
should I restructure to avoid any leaks?

https://codereview.chromium.org/22228002/diff/49001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:112:
EXPECT_EQ(error, "'string': expected std::string, got number");
On 2013/08/09 18:20:58, kalman wrote:
> s/std::string/string/

Done. Using the underlying_type now, rather than the cpp_type.

[dhnishi (use Chromium), 2013-08-09 21:12:17]

Templates are live. Valgrind isn't complaining about any leakde memory anymore.
Woo!

[not at google - send to devlin, 2013-08-09 21:13:01]

Meant to send this out before.

https://codereview.chromium.org/22228002/diff/49001/tools/json_schema_compile...
File tools/json_schema_compiler/test/error_generation_unittest.cc (right):

https://codereview.chromium.org/22228002/diff/49001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:112:
EXPECT_EQ(error, "'string': expected std::string, got number");
On 2013/08/09 20:50:48, Daniel Nishi wrote:
> On 2013/08/09 18:20:58, kalman wrote:
> > s/std::string/string/
> 
> Done. Using the underlying_type now, rather than the cpp_type.

Yeah these errors are for displaying to extension developers, the cpp types
don't mean much to them. We want the JSON types.

[not at google - send to devlin, 2013-08-09 21:14:02]

https://codereview.chromium.org/22228002/diff/58001/tools/json_schema_compile...
File tools/json_schema_compiler/test/error_generation_unittest.cc (right):

https://codereview.chromium.org/22228002/diff/58001/tools/json_schema_compile...
tools/json_schema_compiler/test/error_generation_unittest.cc:35: "expected
dictionary, got binary");
the usual idiom is to put the expected value first in test assertions. i.e.

EXPECT_EQ("expected dictionary, got binary",
          GetPopulateError(..))

[not at google - send to devlin, 2013-08-09 21:14:11]

lgtm

[commit-bot: I haz the power, 2013-08-09 21:23:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/DHNishi@gmail.com/22228002/65001

[commit-bot: I haz the power, 2013-08-09 21:23:44]

Failed to trigger a try job on chromium_presubmit
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-08-09 21:27:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/DHNishi@gmail.com/22228002/61002

[commit-bot: I haz the power, 2013-08-09 22:56:52]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-08-09 23:14:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/DHNishi@gmail.com/22228002/40002

[commit-bot: I haz the power, 2013-08-09 23:38:12]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-08-09 23:43:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/DHNishi@gmail.com/22228002/87001

[commit-bot: I haz the power, 2013-08-10 00:10:43]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-10 00:35:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/DHNishi@gmail.com/22228002/87001

[commit-bot: I haz the power, 2013-08-10 08:04:36]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-10 17:11:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/DHNishi@gmail.com/22228002/87001

[commit-bot: I haz the power, 2013-08-10 21:26:00]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-12 14:13:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/DHNishi@gmail.com/22228002/87001

[commit-bot: I haz the power, 2013-08-12 19:25:14]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-12 22:02:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/DHNishi@gmail.com/22228002/87001

[commit-bot: I haz the power, 2013-08-12 23:45:43]

Change committed as 217118