Extend SplitStringIntoKeyValuePairs unit tests.

base/strings/string_split_unittest.cc

Index: base/strings/string_split_unittest.cc
diff --git a/base/strings/string_split_unittest.cc b/base/strings/string_split_unittest.cc
index eb69b68fcbd67f7aa9e4b92013c139fce5925eb2..58a5aac86770b0f59f1f9be8b67a2cc271511608 100644
--- a/base/strings/string_split_unittest.cc
+++ b/base/strings/string_split_unittest.cc
@@ -29,122 +29,131 @@ void SplitString(const std::wstring& str,
 
 }  // anonymous namespace
 
-class SplitStringIntoKeyValuesTest : public testing::Test {

[Mark Mentovai, 2014/03/06 17:44:31]

This change gets rid of the SplitStringIntoKeyValues tests, and
https://codereview.chromium.org/184233010/ gets rid of the
SplitStringIntoKeyValues interface.

That’s fine, I probably would have preferred to see them in the same change, but
it’s not a huge deal. But the CL description should mention that you’re getting
rid of this.

[pneubeck (no reviews), 2014/03/06 18:33:21]

Done.

+class SplitStringIntoKeyValuePairsTest : public testing::Test {
  protected:
-  std::string key;
-  std::vector<std::string> values;
+  std::vector<std::pair<std::string, std::string> > kv_pairs;
 };
 
-TEST_F(SplitStringIntoKeyValuesTest, EmptyInputMultipleValues) {
-  EXPECT_FALSE(SplitStringIntoKeyValues(std::string(),  // Empty input
-                                        '\t',           // Key separators
-                                        &key,
-                                        &values));
-  EXPECT_TRUE(key.empty());
-  EXPECT_TRUE(values.empty());
-}
-
-TEST_F(SplitStringIntoKeyValuesTest, EmptyValueInputMultipleValues) {
-  EXPECT_FALSE(SplitStringIntoKeyValues("key_with_no_value\t",
-                                        '\t',  // Key separators
-                                        &key, &values));
-  EXPECT_EQ("key_with_no_value", key);
-  EXPECT_TRUE(values.empty());
-}
-
-TEST_F(SplitStringIntoKeyValuesTest, EmptyKeyInputMultipleValues) {
-  EXPECT_TRUE(SplitStringIntoKeyValues("\tvalue for empty key",
-                                       '\t',  // Key separators
-                                       &key, &values));
-  EXPECT_TRUE(key.empty());
-  ASSERT_EQ(1U, values.size());
+TEST_F(SplitStringIntoKeyValuePairsTest, EmptyString) {
+  EXPECT_TRUE(SplitStringIntoKeyValuePairs(std::string(),
+                                           ':',  // Key-value delimiter
+                                           ',',  // Key-value pair delimiter
+                                           &kv_pairs));
+  EXPECT_TRUE(kv_pairs.empty());
 }
 
-TEST_F(SplitStringIntoKeyValuesTest, KeyWithMultipleValues) {
-  EXPECT_TRUE(SplitStringIntoKeyValues("key1\tvalue1,   value2   value3",
-                                       '\t',  // Key separators
-                                       &key, &values));
-  EXPECT_EQ("key1", key);
-  ASSERT_EQ(1U, values.size());
-  EXPECT_EQ("value1,   value2   value3", values[0]);
+TEST_F(SplitStringIntoKeyValuePairsTest, MissingKeyValueDelimiter) {
+  EXPECT_FALSE(SplitStringIntoKeyValuePairs("key1,key2:value2",
+                                            ':',  // Key-value delimiter
+                                            ',',  // Key-value pair delimiter
+                                            &kv_pairs));
+  ASSERT_EQ(2U, kv_pairs.size());

[Mark Mentovai, 2014/03/06 17:44:31]

EXPECT_FALSE for the overall SplitStringIntoKeyValuePairs is correct in this
case.

You’ve documented the behavior of SplitStringIntoKeyValuePairs in the other
change as returning an empty pair in kv_pairs for a missing k-v delimiter when
this happens. My question is: is this actually useful? For a malformed k-v where
SplitStringIntoKeyValuePairs returns false, are there any callers that really
care what goes into kv_pairs?

I assume that you want to allow a value to be empty, but what is the expected
behavior when a key is empty? Based on the other tests in this file, it seems
that you want to allow that too. If keys and values are both permitted to be
empty, and assuming that you do want to return something in kv_pairs even when
the overall call returns false, you won’t be able to pluck out which k-v was
malformed and which k-v was just an empty pair in a string like
"not_kv4,k2:v2,:,k4:v4".

[pneubeck (no reviews), 2014/03/06 18:33:21]

I think the behavior of this function is terrible. There are so many special
cases that nobody would expect. IMO this function's behavior should be
simplified much.

Changing the behavior is however a bit tricky as it's used in about 20 places
and we
wouldn't want to break any. And the function indicates that the parsed result
should also be useful if it returns false. If that wouldn't be the case, we test
for "false" only in the negative cases and ignore the list or ensure that it's
empty. Many calls to this function don't even look at the bool result!

These two CLs were not about changing the behavior rather than making it's
behavior better documented and tested and sanely implemented.

I'm however inclined to fix the behavior in a separate CL (if I find enough
time).

On 2014/03/06 17:44:31, Mark Mentovai wrote:

+  EXPECT_TRUE(kv_pairs[0].first.empty());
+  EXPECT_TRUE(kv_pairs[0].second.empty());
+  EXPECT_EQ("key2", kv_pairs[1].first);
+  EXPECT_EQ("value2", kv_pairs[1].second);
 }
 
-TEST_F(SplitStringIntoKeyValuesTest, EmptyInputSingleValue) {
-  EXPECT_FALSE(SplitStringIntoKeyValues(std::string(),  // Empty input
-                                        '\t',           // Key separators
-                                        &key,
-                                        &values));
-  EXPECT_TRUE(key.empty());
-  EXPECT_TRUE(values.empty());
+TEST_F(SplitStringIntoKeyValuePairsTest, EmptyKeyWithKeyValueDelimiter) {
+  EXPECT_TRUE(SplitStringIntoKeyValuePairs(":value1,key2:value2",
+                                           ':',  // Key-value delimiter
+                                           ',',  // Key-value pair delimiter
+                                           &kv_pairs));
+  ASSERT_EQ(2U, kv_pairs.size());
+  EXPECT_TRUE(kv_pairs[0].first.empty());
+  EXPECT_EQ("value1", kv_pairs[0].second);
+  EXPECT_EQ("key2", kv_pairs[1].first);
+  EXPECT_EQ("value2", kv_pairs[1].second);
 }
 
-TEST_F(SplitStringIntoKeyValuesTest, EmptyValueInputSingleValue) {
-  EXPECT_FALSE(SplitStringIntoKeyValues("key_with_no_value\t",
-                                        '\t',  // Key separators
-                                        &key, &values));
-  EXPECT_EQ("key_with_no_value", key);
-  EXPECT_TRUE(values.empty());
+TEST_F(SplitStringIntoKeyValuePairsTest, TrailingAndLeadingPairDelimiter) {
+  EXPECT_TRUE(SplitStringIntoKeyValuePairs(",key1:value1,key2:value2,",

[Mark Mentovai, 2014/03/06 17:44:31]

The behavior chosen for this and for the next case, EmptyPair, are fine, but
it’s pointless to test them if you haven’t documented them in the header.

+                                           ':',   // Key-value delimiter
+                                           ',',   // Key-value pair delimiter
+                                           &kv_pairs));
+  ASSERT_EQ(2U, kv_pairs.size());
+  EXPECT_EQ("key1", kv_pairs[0].first);
+  EXPECT_EQ("value1", kv_pairs[0].second);
+  EXPECT_EQ("key2", kv_pairs[1].first);
+  EXPECT_EQ("value2", kv_pairs[1].second);
 }
 
-TEST_F(SplitStringIntoKeyValuesTest, EmptyKeyInputSingleValue) {
-  EXPECT_TRUE(SplitStringIntoKeyValues("\tvalue for empty key",
-                                       '\t',  // Key separators
-                                       &key, &values));
-  EXPECT_TRUE(key.empty());
-  ASSERT_EQ(1U, values.size());
-  EXPECT_EQ("value for empty key", values[0]);
+TEST_F(SplitStringIntoKeyValuePairsTest, EmptyPair) {
+  EXPECT_TRUE(SplitStringIntoKeyValuePairs("key1:value1,,key3:value3",
+                                           ':',   // Key-value delimiter
+                                           ',',   // Key-value pair delimiter
+                                           &kv_pairs));
+  ASSERT_EQ(2U, kv_pairs.size());
+  EXPECT_EQ("key1", kv_pairs[0].first);
+  EXPECT_EQ("value1", kv_pairs[0].second);
+  EXPECT_EQ("key3", kv_pairs[1].first);
+  EXPECT_EQ("value3", kv_pairs[1].second);
 }
 
-TEST_F(SplitStringIntoKeyValuesTest, KeyWithSingleValue) {
-  EXPECT_TRUE(SplitStringIntoKeyValues("key1\tvalue1,   value2   value3",
-                                       '\t',  // Key separators
-                                       &key, &values));
-  EXPECT_EQ("key1", key);
-  ASSERT_EQ(1U, values.size());
-  EXPECT_EQ("value1,   value2   value3", values[0]);
+TEST_F(SplitStringIntoKeyValuePairsTest, EmptyValue) {
+  EXPECT_FALSE(SplitStringIntoKeyValuePairs("key1:,key2:value2",
+                                            ':',   // Key-value delimiter
+                                            ',',   // Key-value pair delimiter
+                                            &kv_pairs));
+  ASSERT_EQ(2U, kv_pairs.size());
+  EXPECT_EQ("key1", kv_pairs[0].first);
+  EXPECT_EQ("", kv_pairs[0].second);
+  EXPECT_EQ("key2", kv_pairs[1].first);
+  EXPECT_EQ("value2", kv_pairs[1].second);
 }
 
-class SplitStringIntoKeyValuePairsTest : public testing::Test {
- protected:
-  std::vector<std::pair<std::string, std::string> > kv_pairs;
-};
-
-TEST_F(SplitStringIntoKeyValuePairsTest, EmptyString) {
-  EXPECT_TRUE(SplitStringIntoKeyValuePairs(std::string(),
-                                           ':',  // Key-value delimiters
-                                           ',',  // Key-value pair delims
+TEST_F(SplitStringIntoKeyValuePairsTest, UntrimmedWhitespace) {
+  EXPECT_TRUE(SplitStringIntoKeyValuePairs("key1 : value1",
+                                           ':',  // Key-value delimiter
+                                           ',',  // Key-value pair delimiter
                                            &kv_pairs));
-  EXPECT_TRUE(kv_pairs.empty());
+  ASSERT_EQ(1U, kv_pairs.size());
+  EXPECT_EQ("key1 ", kv_pairs[0].first);
+  EXPECT_EQ(" value1", kv_pairs[0].second);
 }
 
-TEST_F(SplitStringIntoKeyValuePairsTest, EmptySecondPair) {
-  EXPECT_TRUE(SplitStringIntoKeyValuePairs("key1:value1,,key3:value3",
-                                           ':',   // Key-value delimiters
-                                           ',',   // Key-value pair delims
+TEST_F(SplitStringIntoKeyValuePairsTest, TrimmedWhitespace) {
+  EXPECT_TRUE(SplitStringIntoKeyValuePairs("key1:value1 , key2:value2",

[Mark Mentovai, 2014/03/06 17:44:31]

Again, your trimming behaviors are fine, but you have to define them somewhere.

[pneubeck (no reviews), 2014/03/06 18:33:21]

Please see the function comment in the next CL.

+                                           ':',   // Key-value delimiter
+                                           ',',   // Key-value pair delimiter
                                            &kv_pairs));
   ASSERT_EQ(2U, kv_pairs.size());
   EXPECT_EQ("key1", kv_pairs[0].first);
   EXPECT_EQ("value1", kv_pairs[0].second);
-  EXPECT_EQ("key3", kv_pairs[1].first);
-  EXPECT_EQ("value3", kv_pairs[1].second);
+  EXPECT_EQ("key2", kv_pairs[1].first);
+  EXPECT_EQ("value2", kv_pairs[1].second);
 }
 
-TEST_F(SplitStringIntoKeyValuePairsTest, EmptySecondValue) {
-  EXPECT_FALSE(SplitStringIntoKeyValuePairs("key1:value1 , key2:",
-                                            ':',   // Key-value delimiters
-                                            ',',   // Key-value pair delims
-                                            &kv_pairs));
+TEST_F(SplitStringIntoKeyValuePairsTest, MultipleKeyValueDelimiters) {
+  EXPECT_TRUE(SplitStringIntoKeyValuePairs("key1:::value1,key2:value2",
+                                           ':',   // Key-value delimiter
+                                           ',',   // Key-value pair delimiter
+                                           &kv_pairs));
   ASSERT_EQ(2U, kv_pairs.size());
   EXPECT_EQ("key1", kv_pairs[0].first);
   EXPECT_EQ("value1", kv_pairs[0].second);
   EXPECT_EQ("key2", kv_pairs[1].first);
-  EXPECT_EQ("", kv_pairs[1].second);
+  EXPECT_EQ("value2", kv_pairs[1].second);
 }
 
+TEST_F(SplitStringIntoKeyValuePairsTest, OnlySplitAtGivenSeparator) {
+  std::string a("a ?!@#$%^&*()_+:/{}\\\tb");

[Mark Mentovai, 2014/03/06 17:44:31]

Maybe you should have a \n in there too.

[pneubeck (no reviews), 2014/03/06 18:33:21]

Done.

+  EXPECT_TRUE(SplitStringIntoKeyValuePairs(a + "X" + a + "Y" + a + "X" + a,
+                                           'X',  // Key-value delimiter
+                                           'Y',  // Key-value pair delimiter
+                                           &kv_pairs));
+  ASSERT_EQ(2U, kv_pairs.size());
+  EXPECT_EQ(a, kv_pairs[0].first);
+  EXPECT_EQ(a, kv_pairs[0].second);
+  EXPECT_EQ(a, kv_pairs[1].first);
+  EXPECT_EQ(a, kv_pairs[1].second);
+}
+
+
 TEST_F(SplitStringIntoKeyValuePairsTest, DelimiterInValue) {
-  EXPECT_TRUE(SplitStringIntoKeyValuePairs("key1:va:ue1 , key2:value2",
-                                           ':',   // Key-value delimiters
-                                           ',',   // Key-value pair delims
+  EXPECT_TRUE(SplitStringIntoKeyValuePairs("key1:va:ue1,key2:value2",
+                                           ':',   // Key-value delimiter
+                                           ',',   // Key-value pair delimiter
                                            &kv_pairs));
   ASSERT_EQ(2U, kv_pairs.size());
   EXPECT_EQ("key1", kv_pairs[0].first);
@@ -160,7 +169,6 @@ TEST(SplitStringUsingSubstrTest, EmptyString) {
   EXPECT_THAT(results, ElementsAre(""));
 }
 
-// Test for SplitString
 TEST(StringUtilTest, SplitString) {
   std::vector<std::wstring> r;