Add JSON Clone() Is() As() testing. by sparkprime · Pull Request #104 · Stackdriver/metadata-agent

sparkprime · 2018-03-29T23:41:05Z

No description provided.

igorpeshansky

Thanks!

igorpeshansky · 2018-03-30T01:48:03Z

test/json_unittest.cc

+    EXPECT_TRUE(v->Is<json::Null>());
    EXPECT_EQ(json::NullType, v->type());
+    // Check it does not throw exception.
+    v->As<json::Null>();


How about EXPECT_EQ(json::NullType, v->As<json::Null>())?

That doesn't work but is there an EXPECT_DYNAMIC_CAST or something?

Oops, I meant EXPECT_EQ(json::NullType, v->As<json::Null>()->type()). That's what I get for typing reviews into a tiny window on my phone.

igorpeshansky · 2018-03-30T01:49:21Z

test/json_unittest.cc

    json::value v = json::Parser::FromString("null");
+    EXPECT_TRUE(v->Is<json::Null>());
    EXPECT_EQ(json::NullType, v->type());
+    // Check it does not throw exception.


Let's also check that all of the other conversions do...

That is implicit in the ASSERT_EQ that the other tests have.

Huh? I'm confused. The contract here is: convert or throw. I was suggesting that we check:

EXPECT_THROW(v->As<json::Boolean>(), json::Exception); EXPECT_THROW(v->As<json::Number>(), json::Exception); EXPECT_THROW(v->As<json::String>(), json::Exception); EXPECT_THROW(v->As<json::Array>(), json::Exception); EXPECT_THROW(v->As<json::Object>(), json::Exception);

And the same for the other cases...

ah yes I get you now

igorpeshansky · 2018-03-30T01:50:22Z

test/json_unittest.cc

 TEST(TrivialParseTest, Null) {
  GuardJsonException([](){
    json::value v = json::Parser::FromString("null");
+    EXPECT_TRUE(v->Is<json::Null>());


Let's also check that Is<any-other-type>() is false.

I've added a test that all types are distinct instead, otherwise this gets silly.

That's separate. You can test that all of the enum values are distinct (which, BTW, we can assert statically in the source — I'll make that change later), but then there's the behavior of the Is template... It would be good to confirm that, e.g., v->Is<json::Number>() is false for a json::Boolean.

igorpeshansky · 2018-03-30T01:52:28Z

test/json_unittest.cc

+  "  \"str\": \"asdfasdf\"\n"
+  "}\n";
+
+const std::string canonical_json_text =


Can't have global non-POD types... This needs to be a constexpr const char[].

igorpeshansky · 2018-03-30T01:52:56Z

test/json_unittest.cc


 // Big tests.

+const std::string json_text =


Can't have global non-POD types... This needs to be a constexpr const char[].

Constants should be named kConstantDescription.

igorpeshansky · 2018-03-30T01:54:29Z

test/json_unittest.cc

-      "\"str\":\"asdfasdf\""
-      "}",
-      v);
+    EXPECT_TOSTRING_EQ(canonical_json_text, v);


I'm not so sure this is a good idea. The expected value now sits far away from the test. Might be better to live with a little duplication...

Done (inlined in this test only).

igorpeshansky · 2018-03-30T02:01:40Z

test/json_unittest.cc

+    EXPECT_TRUE(v->Is<json::Array>());
+    EXPECT_EQ(json::ArrayType, v->type());
    EXPECT_TRUE(v->As<json::Array>()->empty());
  });


For the nonempty case, can you also check that you can extract a json::value using operator[] (i.e., (*array)[0]) and at (i.e., array->at(0))?

(*array)[0] was already there, added at.

igorpeshansky · 2018-03-30T02:03:10Z

test/json_unittest.cc

    json::value v = json::Parser::FromString("{}");
+    EXPECT_TRUE(v->Is<json::Object>());
+    EXPECT_EQ(json::ObjectType, v->type());
    EXPECT_TRUE(v->As<json::Object>()->empty());


For the nonempty case, can you also check that you can extract a json::value using at (i.e., obj->at("f")) and that obj->at("g") throws std::out_of_bounds?

igorpeshansky · 2018-03-30T02:06:44Z

test/json_unittest.cc


 namespace {

+void ExpectSerializesToSelf(const std::string& json_text) {


This needs a comment...

igorpeshansky

A couple more.

igorpeshansky · 2018-03-30T17:25:22Z

test/json_unittest.cc

 TEST(TrivialParseTest, Null) {
  GuardJsonException([](){
    json::value v = json::Parser::FromString("null");
+    EXPECT_TRUE(v->Is<json::Null>());


That's separate. You can test that all of the enum values are distinct (which, BTW, we can assert statically in the source — I'll make that change later), but then there's the behavior of the Is template... It would be good to confirm that, e.g., v->Is<json::Number>() is false for a json::Boolean.

igorpeshansky · 2018-03-30T17:28:43Z

test/json_unittest.cc


 // Big tests.

+const char kComplexExample[] =


constexpr const char kComplexExample[]. Also below.

You need both, unfortunately. constexpr refers to the array, and const refers to the char.

igorpeshansky

Almost there.

igorpeshansky · 2018-03-30T18:45:57Z

test/json_unittest.cc


 // Big tests.

+const char kComplexExample[] =


You need both, unfortunately. constexpr refers to the array, and const refers to the char.

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

test/json_unittest.cc

 #include "gtest/gtest.h"

 #include <functional>
+#include <map>


igorpeshansky · 2018-03-30T18:47:11Z

test/json_unittest.cc

+    json::NullType, json::BooleanType, json::NumberType, json::StringType,
+    json::ArrayType, json::ObjectType
+  });
+  std::set<json::Type> all_types_set(all_types.begin(), all_types.end());


You'll want to #include <set> for this...

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

test/json_unittest.cc

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


Let's group all "f"-related checks together (and all "g"-related checks to follow).

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

test/json_unittest.cc

+    EXPECT_FALSE(obj->Has("g"));
    EXPECT_EQ(2.0, obj->Get<json::Number>("f"));
+    EXPECT_EQ(2.0, obj->at("f")->As<json::Number>()->value());
+    EXPECT_THROW(obj->Get<json::String>("f"), json::Exception);


Might as well check that all Get methods throw except for Get<json::Number>.

supriyagarg

LGTM

supriyagarg · 2018-03-30T19:19:52Z

test/json_unittest.cc

+    EXPECT_THROW(v->As<json::Boolean>(), json::Exception);
+    EXPECT_THROW(v->As<json::Number>(), json::Exception);
+    EXPECT_THROW(v->As<json::String>(), json::Exception);
+    const auto& arr = v->As<json::Array>();


move this line down - to just before the expect_true

They're in that order deliberately, as that's the order of complexity of the json types

supriyagarg · 2018-03-30T19:22:00Z

test/json_unittest.cc

+    EXPECT_THROW(v->As<json::Number>(), json::Exception);
+    EXPECT_THROW(v->As<json::String>(), json::Exception);
    const auto& arr = v->As<json::Array>();
+    EXPECT_THROW(v->As<json::Object>(), json::Exception);


ditto, move the arr declaration down

igorpeshansky · 2018-03-31T05:29:10Z

test/json_unittest.cc

+// Verifies that the given json parses, clones, and serializes to its original
+// form.
+void ExpectSerializesToSelf(const std::string& json_text) {
+    json::value v = json::Parser::FromString(json_text);


2-space indent...

Just fixed.

igorpeshansky

LGTM

igorpeshansky · 2018-03-31T05:34:13Z

test/json_unittest.cc

+// Verifies that the given json parses, clones, and serializes to its original
+// form.
+void ExpectSerializesToSelf(const std::string& json_text) {
+    json::value v = json::Parser::FromString(json_text);


Just fixed.

Add Clone() Is() As() testing

71d4b19

igorpeshansky requested review from bmoyles0117, igorpeshansky and supriyagarg March 30, 2018 01:29

igorpeshansky suggested changes Mar 30, 2018

View reviewed changes

igorpeshansky changed the title ~~Add Clone() Is() As() testing~~ Add JSON Clone() Is() As() testing. Mar 30, 2018

Respond to comments

34e423f

igorpeshansky suggested changes Mar 30, 2018

View reviewed changes

Respond to comments

0592ff3

igorpeshansky suggested changes Mar 30, 2018

View reviewed changes

Respond to comments

60c6b30

supriyagarg approved these changes Mar 30, 2018

View reviewed changes

Add const to constexpr

6f155c7

igorpeshansky suggested changes Mar 31, 2018

View reviewed changes

Whitespace fixes.

b5f0e5b

igorpeshansky approved these changes Mar 31, 2018

View reviewed changes

igorpeshansky merged commit e031c55 into master Mar 31, 2018

igorpeshansky deleted the dcunnin-more-json-unittest branch March 31, 2018 05:35


		namespace {

		void ExpectSerializesToSelf(const std::string& json_text) {

Conversation

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

Uh oh!

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!

sparkprime Mar 30, 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!

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 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!

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

sparkprime commented Mar 29, 2018 •

edited by igorpeshansky

Loading

sparkprime Mar 30, 2018 •

edited by igorpeshansky

Loading