Add unit tests for JSON parsing & serialization. by sparkprime · Pull Request #92 · Stackdriver/metadata-agent

sparkprime · 2018-03-27T17:43:51Z

No description provided.

sparkprime · 2018-03-27T17:45:32Z

sparkprime · 2018-03-27T17:46:16Z

test/json_unittest.cc

+      -10.0, -10.0e+0, -10.0e-0, -10.0e0, -10.0E+0, -10.0E-0, -10.0E0,
+      -123, -123.456, -789, -1.05, -1.999e-99,
+    }));
+    // TODO(igorp): This first one looks spurious.


@igorpeshansky

Sorry, which first one? Do you mean -0 transforming to 0.0? That's because with int, there is no separate representation for -0, so it gets converted to static_cast<double>(0). Nothing to do here.

That doesn't explain json::Parser::FromString("-0")->ToString() == "0"

Hmm, true. Ok, warrants further investigation. Let's remove the username from the TODO, though, and add what you just said...

sparkprime · 2018-03-27T17:46:26Z

test/json_unittest.cc

+// String edge cases
+
+/*
+ * TODO(igorp): Seems that yajl does not support valid JSON.


@igorpeshansky

There's already an issue for this. Let's just change this to:
// TODO: yajl doesn't support newlines in strings (https://github.com/lloyd/yajl/issues/180).

igorpeshansky

Some comments.

igorpeshansky · 2018-03-27T18:02:07Z

test/Makefile

      format_unittest \
      health_checker_unittest \
      resource_unittest \
+      json_unittest \


Want to keep these in alphabetical order?

igorpeshansky · 2018-03-27T18:02:28Z

test/json_unittest.cc

+#include "../src/json.h"
+#include "gtest/gtest.h"
+
+#define EXPECT_TO_STRING(v, s) EXPECT_EQ(v->ToString(), s)


EXPECT_TOSTRING_EQ?

igorpeshansky · 2018-03-27T18:02:52Z

test/json_unittest.cc

@@ -0,0 +1,385 @@
+#include <functional>


Let's have this after the first two includes (for consistency).

Let's add a blank line before this one.

igorpeshansky · 2018-03-27T18:04:44Z

test/json_unittest.cc

+TEST(TrivialParseTest, Null) {
+  GuardJsonException([](){
+    std::unique_ptr<json::Value> v = json::Parser::FromString("null");
+    EXPECT_EQ(v->type(), json::NullType);


The convention seems to be EXPECT_EQ(expected, actual).

igorpeshansky · 2018-03-27T18:05:04Z

test/json_unittest.cc

+#include "../src/json.h"
+#include "gtest/gtest.h"
+
+#define EXPECT_TO_STRING(v, s) EXPECT_EQ(v->ToString(), s)


The convention seems to be EXPECT_EQ(expected, actual).

igorpeshansky · 2018-03-27T18:17:39Z

test/json_unittest.cc

+TEST(EdgeTest, StringWithUnicodeEscape) {
+  GuardJsonException([](){
+    std::unique_ptr<json::Value> v = json::Parser::FromString("\"foo\\u000abar\"");
+    //EXPECT_EQ(v->As<json::String>()->value(), "foo\nbar");


Leftover debugging code?

igorpeshansky · 2018-03-27T18:19:10Z

test/json_unittest.cc

+TEST(EdgeTest, StringWithUTF8) {
+  GuardJsonException([](){
+    // This is Korean for "Hello World!".
+    std::unique_ptr<json::Value> v = json::Parser::FromString("\"안녕 세상아!\"");


My heartfelt thanks for not using the poop emoji. 😄

igorpeshansky · 2018-03-27T18:23:35Z

test/json_unittest.cc

+
+// Number edge cases
+
+TEST(EdgeTest, PositiveNumbers) {


Is this a parse/ToString test? Would it make sense to also test JSON construction:

json::value v = json::array({ 0, 0e+0, 0e-0, 0e0, 0E+0, 0E-0, 0E0, 0.0, 0.0e+0, 0.0e-0, 0.0e0, 0.0E+0, 0.0E-0, 0.0E0, 10, 10e+0, 10e-0, 10e0, 10E+0, 10E-0, 10E0, 10.0, 10.0e+0, 10.0e-0, 10.0e0, 10.0E+0, 10.0E-0, 10.0E0, 123, 123.456, 789, 1.05, 1.999e-99, });

?

That's testing the C++ parser though. I could have split it into two parts, but since we do a lot of that already (and therefore ensure good coverage) I figured an end-to-end test is enough here.

This is also testing the contract provided by those functions and constructors... Let's do this in the RealisticConstruction tests.

igorpeshansky · 2018-03-27T18:25:02Z

test/json_unittest.cc

+  });
+}
+
+TEST(BigTest, LotsOfNesting) {


Nested objects, too?

igorpeshansky · 2018-03-27T18:30:48Z

test/json_unittest.cc

+
+// Big tests.
+
+TEST(BigTest, Realistic) {


I'd also check construction:

json::value v = json::object({ {"foo", json::array({json::number(1), json::number(2), json::number(3)})}, {"bar", json::object({{"x", json::number(0)}, {"y", json::null()}})}, {"baz", json::boolean(true)}, {"str", json::string("asdfasdf")}, });

igorpeshansky · 2018-03-27T19:07:52Z

@sparkprime Did you mean to push another commit?

igorpeshansky

Some more comments.

igorpeshansky · 2018-03-27T19:12:30Z

test/json_unittest.cc

@@ -0,0 +1,385 @@
+#include <functional>


Let's add a blank line before this one.

igorpeshansky · 2018-03-27T19:13:01Z

test/json_unittest.cc

+}
+
+
+// Trival Null


https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_Grammar

igorpeshansky · 2018-03-27T19:17:34Z

test/json_unittest.cc

+    json::value v = json::Parser::FromString("{\"f\":2}");
+    const auto& obj = v->As<json::Object>();
+    EXPECT_EQ(1, obj->size());
+    EXPECT_EQ(2.0, obj->Get<json::Number>("f"));


Let's add this here or in a separate test:

EXPECT_TRUE(v->As<json::Object>()->Has("f")); EXPECT_FALSE(v->As<json::Object>()->Has("g"));

igorpeshansky · 2018-03-27T19:26:19Z

test/json_unittest.cc

+
+// Number edge cases
+
+TEST(EdgeTest, PositiveNumbers) {


This is also testing the contract provided by those functions and constructors... Let's do this in the RealisticConstruction tests.

igorpeshansky · 2018-03-27T19:27:59Z

test/json_unittest.cc

+      -10.0, -10.0e+0, -10.0e-0, -10.0e0, -10.0E+0, -10.0E-0, -10.0E0,
+      -123, -123.456, -789, -1.05, -1.999e-99,
+    }));
+    // TODO(igorp): This first one looks spurious.


Hmm, true. Ok, warrants further investigation. Let's remove the username from the TODO, though, and add what you just said...

igorpeshansky · 2018-03-27T19:30:52Z

test/json_unittest.cc

+        "123.0,123.45600000000000307,789.0,"
+        "1.0500000000000000444,1.9989999999999998999e-99"
+      "]",
+      v


Might as well close the parenthesis on this line. Also in other EXPECT_TOSTRING_EQ calls below.

igorpeshansky

Minor stuff.

igorpeshansky · 2018-03-27T20:46:11Z

test/json_unittest.cc

+TEST(TrivialToStringTest, ObjectOneField) {
+  GuardJsonException([](){
+    json::value v = json::object({
+      {"f", json::number(2.0)}


Let's add a trailing comma, to indicate our preference to that style.

igorpeshansky · 2018-03-27T20:46:57Z

test/json_unittest.cc

+  ASSERT_THROW(json::Parser::FromString("{\"x\":}"), json::Exception);
+}
+
+


You can remove one of these two blank lines.

igorpeshansky

Thanks!
LGTM

Let's squash-and-merge this!

bmoyles0117

LGTM

Add unit tests for JSON parsing & serialization.

0db257e

sparkprime force-pushed the dcunnin-json-tests branch from ee4bacd to 0db257e Compare March 27, 2018 17:45

sparkprime commented Mar 27, 2018

View reviewed changes

igorpeshansky requested review from bmoyles0117 and igorpeshansky March 27, 2018 17:54

Add some parse error tests

5353071

igorpeshansky suggested changes Mar 27, 2018

View reviewed changes

Respond to comments

cee7634

igorpeshansky suggested changes Mar 27, 2018

View reviewed changes

sparkprime added 4 commits March 27, 2018 15:41

Respond to more comments

16a328a

titles -> sentences

2bd9b1e

terminating parens

101c979

Test 'Has'

3afceb8

igorpeshansky suggested changes Mar 27, 2018

View reviewed changes

minor syntax & whitespace

5ebf798

igorpeshansky approved these changes Mar 27, 2018

View reviewed changes

bmoyles0117 approved these changes Mar 27, 2018

View reviewed changes

igorpeshansky merged commit 2aecc4d into master Mar 27, 2018

igorpeshansky deleted the dcunnin-json-tests branch March 27, 2018 23:10

dhrupadb pushed a commit that referenced this pull request Mar 28, 2018

Add JSON unit tests. (#92)

dc3db77

		ASSERT_THROW(json::Parser::FromString("{\"x\":}"), json::Exception);
		}

Conversation

sparkprime commented Mar 27, 2018

Uh oh!

sparkprime commented Mar 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sparkprime Mar 27, 2018 • edited by igorpeshansky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky commented Mar 27, 2018

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sparkprime Mar 27, 2018 •

edited by igorpeshansky

Loading