Split ValueSerializer into separate Serializer and Deserializer classes.

base/json/json_value_serializer_unittest.cc

Index: base/json/json_value_serializer_unittest.cc
diff --git a/base/json/json_value_serializer_unittest.cc b/base/json/json_value_serializer_unittest.cc
index d2a84dea89622b60c5898a6f0d2c34c961ab2bf3..f24951716ddf3299c31942b071538ecac621a7d0 100644
--- a/base/json/json_value_serializer_unittest.cc
+++ b/base/json/json_value_serializer_unittest.cc
@@ -89,25 +89,7 @@ void ValidateJsonList(const std::string& json) {
 // Test proper JSON [de]serialization from string is working.

[rvargas (doing something else), 2015/02/25 03:38:24]

nit: deserialization

 TEST(JSONValueSerializerTest, ReadProperJSONFromString) {

[rvargas (doing something else), 2015/02/25 03:38:24]

nit: Deserializer. (& others)

[prashant.hiremath, 2015/02/25 06:01:37]

Hi Ricardo, 
Thanks for the review.
I'll change the test category name to JSONValueDeserializerTest, which is
explicitly testing deserialization.
But there are some tests which are testing both serialization and
deserialization like "Roundtrip". what should we name for those test categories
? 

[Matt Giuca, 2015/02/25 06:11:40]

I think the test name should probably match the filename. So if we were to
rename half of these tests to JSONValueDeserializerTest, we probably should move
them to json_value_deserializer_unittest.cc. That seems like too much churn,
especially for this already-big CL. I would just leave it as-is (which solves
the dilemma of which test class to put Roundtrip in).

[rvargas (doing something else), 2015/02/25 19:00:15]

While I agree that in general the test name matches the name of the file, I see
that as a consequence of the guideline of having a class Foo live on foo.h.

On the other hand, I see no big issue in having both classes live in the same
header (as they are very closely related)... which also means that tests live
together in the same file.

I think it is more important to match the (main) class that is under test. (see
time_unittest.cc for an example). As for Roundtrip, leaving it under
serialization sounds fine to me.

[Matt Giuca, 2015/02/25 23:20:49]

Acknowledged.

   // Try to deserialize it through the serializer.
-  JSONStringValueSerializer str_deserializer(kProperJSON);
-
-  int error_code = 0;
-  std::string error_message;
-  scoped_ptr<Value> value(
-      str_deserializer.Deserialize(&error_code, &error_message));
-  ASSERT_TRUE(value.get());
-  ASSERT_EQ(0, error_code);
-  ASSERT_TRUE(error_message.empty());
-  // Verify if the same JSON is still there.
-  CheckJSONIsStillTheSame(*value);
-}
-
-// Test proper JSON deserialization from a string pointer is working.
-TEST(JSONValueSerializerTest, ReadProperJSONFromStringPointer) {
-  // Try to deserialize a string pointer through the serializer. (This exercises
-  // a separate code path to passing a StringPiece.)
-  std::string proper_json(kProperJSON);
-  JSONStringValueSerializer str_deserializer(&proper_json);
+  JSONStringValueDeserializer str_deserializer(kProperJSON);
 
   int error_code = 0;
   std::string error_message;
@@ -126,7 +108,7 @@ TEST(JSONValueSerializerTest, ReadProperJSONFromStringPiece) {
   // kProperJSON.
   base::StringPiece proper_json(kProperJSONPadded);
   proper_json = proper_json.substr(5, proper_json.length() - 10);
-  JSONStringValueSerializer str_deserializer(proper_json);
+  JSONStringValueDeserializer str_deserializer(proper_json);
 
   int error_code = 0;
   std::string error_message;
@@ -143,7 +125,7 @@ TEST(JSONValueSerializerTest, ReadProperJSONFromStringPiece) {
 // the proper flag for that is set.
 TEST(JSONValueSerializerTest, ReadJSONWithTrailingCommasFromString) {
   // Try to deserialize it through the serializer.
-  JSONStringValueSerializer str_deserializer(kProperJSONWithCommas);
+  JSONStringValueDeserializer str_deserializer(kProperJSONWithCommas);
 
   int error_code = 0;
   std::string error_message;
@@ -171,7 +153,7 @@ TEST(JSONValueSerializerTest, ReadProperJSONFromFile) {
             WriteFile(temp_file, kProperJSON, strlen(kProperJSON)));
 
   // Try to deserialize it through the serializer.
-  JSONFileValueSerializer file_deserializer(temp_file);
+  JSONFileValueDeserializer file_deserializer(temp_file);
 
   int error_code = 0;
   std::string error_message;
@@ -196,7 +178,7 @@ TEST(JSONValueSerializerTest, ReadJSONWithCommasFromFile) {
                       strlen(kProperJSONWithCommas)));
 
   // Try to deserialize it through the serializer.
-  JSONFileValueSerializer file_deserializer(temp_file);
+  JSONFileValueDeserializer file_deserializer(temp_file);
   // This must fail without the proper flag.
   int error_code = 0;
   std::string error_message;
@@ -217,8 +199,8 @@ TEST(JSONValueSerializerTest, ReadJSONWithCommasFromFile) {
 TEST(JSONValueSerializerTest, Roundtrip) {
   static const char kOriginalSerialization[] =
     "{\"bool\":true,\"double\":3.14,\"int\":42,\"list\":[1,2],\"null\":null}";
-  JSONStringValueSerializer serializer(kOriginalSerialization);
-  scoped_ptr<Value> root(serializer.Deserialize(NULL, NULL));
+  JSONStringValueDeserializer deserializer(kOriginalSerialization);
+  scoped_ptr<Value> root(deserializer.Deserialize(NULL, NULL));
   ASSERT_TRUE(root.get());
   ASSERT_TRUE(root->IsType(Value::TYPE_DICTIONARY));
 
@@ -241,10 +223,6 @@ TEST(JSONValueSerializerTest, Roundtrip) {
   ASSERT_TRUE(root_dict->GetDouble("double", &double_value));
   ASSERT_DOUBLE_EQ(3.14, double_value);
 
-  // We shouldn't be able to write using this serializer, since it was
-  // initialized with a const string.
-  ASSERT_FALSE(serializer.Serialize(*root_dict));
-
   std::string test_serialization;
   JSONStringValueSerializer mutable_serializer(&test_serialization);
   ASSERT_TRUE(mutable_serializer.Serialize(*root_dict));
@@ -331,7 +309,7 @@ TEST(JSONValueSerializerTest, UnicodeStrings) {
   ASSERT_EQ(kExpected, actual);
 
   // escaped ascii text -> json
-  JSONStringValueSerializer deserializer(kExpected);
+  JSONStringValueDeserializer deserializer(kExpected);
   scoped_ptr<Value> deserial_root(deserializer.Deserialize(NULL, NULL));
   ASSERT_TRUE(deserial_root.get());
   DictionaryValue* dict_root =
@@ -355,7 +333,7 @@ TEST(JSONValueSerializerTest, HexStrings) {
   ASSERT_EQ(kExpected, actual);
 
   // escaped ascii text -> json
-  JSONStringValueSerializer deserializer(kExpected);
+  JSONStringValueDeserializer deserializer(kExpected);
   scoped_ptr<Value> deserial_root(deserializer.Deserialize(NULL, NULL));
   ASSERT_TRUE(deserial_root.get());
   DictionaryValue* dict_root =
@@ -366,7 +344,7 @@ TEST(JSONValueSerializerTest, HexStrings) {
 
   // Test converting escaped regular chars
   static const char kEscapedChars[] = "{\"test\":\"\\u0067\\u006f\"}";
-  JSONStringValueSerializer deserializer2(kEscapedChars);
+  JSONStringValueDeserializer deserializer2(kEscapedChars);
   deserial_root.reset(deserializer2.Deserialize(NULL, NULL));
   ASSERT_TRUE(deserial_root.get());
   dict_root = static_cast<DictionaryValue*>(deserial_root.get());
@@ -380,12 +358,12 @@ TEST(JSONValueSerializerTest, AllowTrailingComma) {
   static const char kTestWithCommas[] = "{\"key\": [true,],}";
   static const char kTestNoCommas[] = "{\"key\": [true]}";
 
-  JSONStringValueSerializer serializer(kTestWithCommas);
-  serializer.set_allow_trailing_comma(true);
-  JSONStringValueSerializer serializer_expected(kTestNoCommas);
-  root.reset(serializer.Deserialize(NULL, NULL));
+  JSONStringValueDeserializer deserializer(kTestWithCommas);
+  deserializer.set_allow_trailing_comma(true);
+  JSONStringValueDeserializer deserializer_expected(kTestNoCommas);
+  root.reset(deserializer.Deserialize(NULL, NULL));
   ASSERT_TRUE(root.get());
-  root_expected.reset(serializer_expected.Deserialize(NULL, NULL));
+  root_expected.reset(deserializer_expected.Deserialize(NULL, NULL));
   ASSERT_TRUE(root_expected.get());
   ASSERT_TRUE(root->Equals(root_expected.get()));
 }
@@ -435,7 +413,7 @@ TEST_F(JSONFileValueSerializerTest, Roundtrip) {
 
   ASSERT_TRUE(PathExists(original_file_path));
 
-  JSONFileValueSerializer deserializer(original_file_path);
+  JSONFileValueDeserializer deserializer(original_file_path);
   scoped_ptr<Value> root;
   root.reset(deserializer.Deserialize(NULL, NULL));
 
@@ -483,7 +461,7 @@ TEST_F(JSONFileValueSerializerTest, RoundtripNested) {
 
   ASSERT_TRUE(PathExists(original_file_path));
 
-  JSONFileValueSerializer deserializer(original_file_path);
+  JSONFileValueDeserializer deserializer(original_file_path);
   scoped_ptr<Value> root;
   root.reset(deserializer.Deserialize(NULL, NULL));
   ASSERT_TRUE(root.get());
@@ -508,9 +486,9 @@ TEST_F(JSONFileValueSerializerTest, NoWhitespace) {
   source_file_path = source_file_path.Append(
       FILE_PATH_LITERAL("serializer_test_nowhitespace.json"));
   ASSERT_TRUE(PathExists(source_file_path));
-  JSONFileValueSerializer serializer(source_file_path);
+  JSONFileValueDeserializer deserializer(source_file_path);
   scoped_ptr<Value> root;
-  root.reset(serializer.Deserialize(NULL, NULL));
+  root.reset(deserializer.Deserialize(NULL, NULL));
   ASSERT_TRUE(root.get());
 }