Unit test for NULL values in preference validation delegate.

chrome/browser/safe_browsing/preference_validation_delegate_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/safe_browsing/preference_validation_delegate.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 105 matching lines @@
         value->SetInteger("forty-seven", 47);
         return value.PassAs<Value>();
       }
       case Value::TYPE_LIST: {
         scoped_ptr<base::ListValue> value(new base::ListValue());
         value->AppendInteger(22);
         value->AppendInteger(47);
         return value.PassAs<Value>();
       }
       default:
-        ADD_FAILURE() << "unsupported value type " << value_type;
+        // An unknown type means a NULL value.

[gab, 2014/06/16 15:51:41]

Specifically put -1 in a constant and handle it above; e.g. kNullValueForTest,
as this is essentially a hack for your test and it feels fairly obscure right
now.

+        return scoped_ptr<Value>();
     }
-    return scoped_ptr<Value>();
   }
 
   base::Value::Type value_type_;
   const char* expected_value_;
 };
 
 TEST_P(PreferenceValidationDelegateValues, Value) {
   instance_->OnAtomicPreferenceValidation(kPrefPath_,
                                           MakeValue(value_type_).get(),
                                           PrefHashStoreTransaction::CLEARED,
                                           TrackedPreferenceHelper::DONT_RESET);
   safe_browsing::ClientIncidentReport_IncidentData* incident =
       incidents_.back();
-  EXPECT_EQ(std::string(expected_value_),
-            incident->tracked_preference().atomic_value());
+  if (!*expected_value_) {

[gab, 2014/06/16 15:51:41]

This is a fairly obscure way to do "check that the string is empty which is a
trick in this test to indicate a NULL value"; can you make this clearer to a the
next reader who will see it as a whole (and not in a diff like I am viewing it
now which clearly labels all the pieces of the hack)

+    EXPECT_FALSE(incident->tracked_preference().has_atomic_value());
+  } else {
+    EXPECT_EQ(std::string(expected_value_),
+              incident->tracked_preference().atomic_value());
+  }
 }
 
 INSTANTIATE_TEST_CASE_P(
     Values,
     PreferenceValidationDelegateValues,
     testing::Values(
+        std::tr1::make_tuple(static_cast<base::Value::Type>(-1), ""),

[gab, 2014/06/16 15:51:41]

"" and NULL value is not the same; I guess this is fine here, but please add
extra comments to explain this case.

         std::tr1::make_tuple(base::Value::TYPE_NULL, "null"),
         std::tr1::make_tuple(base::Value::TYPE_BOOLEAN, "false"),
         std::tr1::make_tuple(base::Value::TYPE_INTEGER, "47"),
         std::tr1::make_tuple(base::Value::TYPE_DOUBLE, "0.47"),
         std::tr1::make_tuple(base::Value::TYPE_STRING, "i have a spleen"),
         std::tr1::make_tuple(base::Value::TYPE_DICTIONARY,
                              "{\"forty-seven\":47,\"twenty-two\":22}"),
         std::tr1::make_tuple(base::Value::TYPE_LIST, "[22,47]")));
 
 // Tests that no incidents are reported for relevant combinations of ValueState.
@@ skipping 89 matching lines @@
 INSTANTIATE_TEST_CASE_P(
     WithIncident,
     PreferenceValidationDelegateWithIncident,
     testing::Combine(
         testing::Values(PrefHashStoreTransaction::CLEARED,
                         PrefHashStoreTransaction::WEAK_LEGACY,
                         PrefHashStoreTransaction::CHANGED,
                         PrefHashStoreTransaction::UNTRUSTED_UNKNOWN_VALUE),
         testing::Values(TrackedPreferenceHelper::WANTED_RESET,
                         TrackedPreferenceHelper::DO_RESET)));