Add a base::ParseJson() function to help tests construct Values.

Add a base::ParseJson() function to help tests construct Values.

This is very similar to base::JSONReader::Read() except that it allows
trailing commas and EXPECT-fails with a good message when the json is
malformed.

I migrated two tests to use this as a demonstration that it's widely
useful and makes things more readable.


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

[Jeffrey Yasskin, 2012-12-13 23:20:29]

Dominic and James, feel free to tell me to find other example files.

https://codereview.chromium.org/11567027/diff/1/base/test/values_test_util.cc
File base/test/values_test_util.cc (right):

https://codereview.chromium.org/11567027/diff/1/base/test/values_test_util.cc...
base/test/values_test_util.cc:63: scoped_ptr<Value> ParseJson(base::StringPiece
json) {
It might make sense to put this into a base::test namespace. Which do you
prefer?

[jamesr, 2012-12-13 23:23:32]

On 2012/12/13 23:20:29, Jeffrey Yasskin wrote:
> Dominic and James, feel free to tell me to find other example files.
> 

cc/layer_tree_host_perftest.cc has this:

    ASSERT_TRUE(file_util::ReadFileToString(json_file, &json));
    tree_.reset(base::JSONReader::Read(json));
    ASSERT_TRUE(tree_);


having a nicer failure message when the data is bad would be great!

[jamesr, 2012-12-13 23:24:41]

https://codereview.chromium.org/11567027/diff/1/base/test/values_test_util.h
File base/test/values_test_util.h (right):

https://codereview.chromium.org/11567027/diff/1/base/test/values_test_util.h#...
base/test/values_test_util.h:46: // Parses |json| as JSON, allowing trailing
commas, and returns the
I'm not sure I understand the trailing commas bit.  Is that just the most common
mode in our test code?

[Jeffrey Yasskin, 2012-12-13 23:29:27]

https://codereview.chromium.org/11567027/diff/1/base/test/values_test_util.h
File base/test/values_test_util.h (right):

https://codereview.chromium.org/11567027/diff/1/base/test/values_test_util.h#...
base/test/values_test_util.h:46: // Parses |json| as JSON, allowing trailing
commas, and returns the
On 2012/12/13 23:24:41, jamesr wrote:
> I'm not sure I understand the trailing commas bit.  Is that just the most
common
> mode in our test code?

No, I tend to find that allowing trailing commas makes it easier to maintain
code, and when we're constructing a Value in tests, we'd prefer maintainable
code over portable input data. If we had an 'accept-single-quotes' mode, I'd
want to turn that on too.

Your example from layer_tree_host_perftest.cc of reading a file and passing it
through ParseJson may argue the other direction, though. I haven't counted
possible uses to figure out which is more common.

[jamesr, 2012-12-13 23:31:37]

On Thu, Dec 13, 2012 at 3:29 PM, <jyasskin@chromium.org> wrote:

>
> https://codereview.chromium.**org/11567027/diff/1/base/test/**
>
values_test_util.h<https://codereview.chromium.org/11567027/diff/1/base/test/values_test_util.h>
> File base/test/values_test_util.h (right):
>
> https://codereview.chromium.**org/11567027/diff/1/base/test/**
>
values_test_util.h#newcode46<https://codereview.chromium.org/11567027/diff/1/base/test/values_test_util.h#newcode46>
> base/test/values_test_util.h:**46: // Parses |json| as JSON, allowing
> trailing commas, and returns the
> On 2012/12/13 23:24:41, jamesr wrote:
>
>> I'm not sure I understand the trailing commas bit.  Is that just the
>>
> most common
>
>> mode in our test code?
>>
>
> No, I tend to find that allowing trailing commas makes it easier to
> maintain code, and when we're constructing a Value in tests, we'd prefer
> maintainable code over portable input data. If we had an
> 'accept-single-quotes' mode, I'd want to turn that on too.
>
> Your example from layer_tree_host_perftest.cc of reading a file and
> passing it through ParseJson may argue the other direction, though. I
> haven't counted possible uses to figure out which is more common.
>

I feel the exact opposite of strongly about this.  I like the general
principle of being lax when accepting data in tests.

>
>
https://codereview.chromium.**org/11567027/<https://codereview.chromium.org/1...
>

[willchan no longer on Chromium, 2012-12-14 20:35:04]

lgtm, mod this naming concern.

https://chromiumcodereview.appspot.com/11567027/diff/1/base/test/values_test_...
File base/test/values_test_util.cc (right):

https://chromiumcodereview.appspot.com/11567027/diff/1/base/test/values_test_...
base/test/values_test_util.cc:63: scoped_ptr<Value> ParseJson(base::StringPiece
json) {
On 2012/12/13 23:20:29, Jeffrey Yasskin wrote:
> It might make sense to put this into a base::test namespace. Which do you
> prefer?

It would make sense if I foresaw someone taking the time to move all the rest.
Realistically speaking, I don't see that happening.

Actually, now that I look at it, I may lean a bit more towards putting it in the
base::test namespace. My reasoning is that ParseJson() is too generic a name to
put just in base::. Either we should rename to base::ParseJsonForTests() or put
it in base::test::ParseJson().

[Jeffrey Yasskin, 2012-12-14 23:12:41]

https://chromiumcodereview.appspot.com/11567027/diff/1/base/test/values_test_...
File base/test/values_test_util.cc (right):

https://chromiumcodereview.appspot.com/11567027/diff/1/base/test/values_test_...
base/test/values_test_util.cc:63: scoped_ptr<Value> ParseJson(base::StringPiece
json) {
On 2012/12/14 20:35:04, willchan wrote:
> On 2012/12/13 23:20:29, Jeffrey Yasskin wrote:
> > It might make sense to put this into a base::test namespace. Which do you
> > prefer?
> 
> It would make sense if I foresaw someone taking the time to move all the rest.
> Realistically speaking, I don't see that happening.
> 
> Actually, now that I look at it, I may lean a bit more towards putting it in
the
> base::test namespace. My reasoning is that ParseJson() is too generic a name
to
> put just in base::. Either we should rename to base::ParseJsonForTests() or
put
> it in base::test::ParseJson().

I've moved it into base::test so that tests that want the shorter name can use
"using base::test::ParseJson". I'll wait until Monday to submit so Dominic can
weigh in if he wants.

[battre, 2012-12-17 09:36:48]

LGTM if the two comments don't need to be addressed.

https://chromiumcodereview.appspot.com/11567027/diff/9001/content/renderer/v8...
File content/renderer/v8_value_converter_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/11567027/diff/9001/content/renderer/v8...
content/renderer/v8_value_converter_impl_unittest.cc:537:
EXPECT_TRUE(Value::Equals(base::test::ParseJson("{ \"bar\": null }").get(),
Is this one guaranteed to be correct? I can see that the Value returned by
ParseJson can be discarded after calling get().

https://chromiumcodereview.appspot.com/11567027/diff/9001/content/renderer/v8...
content/renderer/v8_value_converter_impl_unittest.cc:542:
EXPECT_TRUE(Value::Equals(base::test::ParseJson("[ null, null, null ]").get(),
same here.

[Jeffrey Yasskin, 2012-12-17 18:09:27]

Thanks for the reviews. :)

https://chromiumcodereview.appspot.com/11567027/diff/9001/content/renderer/v8...
File content/renderer/v8_value_converter_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/11567027/diff/9001/content/renderer/v8...
content/renderer/v8_value_converter_impl_unittest.cc:537:
EXPECT_TRUE(Value::Equals(base::test::ParseJson("{ \"bar\": null }").get(),
On 2012/12/17 09:36:49, battre wrote:
> Is this one guaranteed to be correct? I can see that the Value returned by
> ParseJson can be discarded after calling get().

The scoped_ptr is destroyed, destroying the Value, at the end of the
"full-expression", roughly at the semicolon. Inside EXPECT(), it'll definitely
get destroyed after all the function calls it appears in, although likely before
(for example) other arguments are streamed into the EXPECT's failure message.
Since it's only used inside the Value::Equals() call, this is safe.

https://chromiumcodereview.appspot.com/11567027/diff/9001/content/renderer/v8...
content/renderer/v8_value_converter_impl_unittest.cc:542:
EXPECT_TRUE(Value::Equals(base::test::ParseJson("[ null, null, null ]").get(),
On 2012/12/17 09:36:49, battre wrote:
> same here.

same here. :)

[commit-bot: I haz the power, 2012-12-17 18:09:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/11567027/9001

[commit-bot: I haz the power, 2012-12-17 18:09:52]

Presubmit check for 11567027-9001 failed and returned exit status 1.


Running presubmit commit checks ...

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

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/renderer

[jamesr, 2012-12-17 18:21:51]

content/renderer lgtm

[commit-bot: I haz the power, 2012-12-17 18:25:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/11567027/9001

[commit-bot: I haz the power, 2012-12-17 21:45:42]

Change committed as 173540