Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
184233009:
Extend SplitStringIntoKeyValuePairs unit tests. (Closed)
|
Created:
6 years, 9 months ago by pneubeck (no reviews) Modified:
6 years, 9 months ago Reviewers:
Mark Mentovai CC:
chromium-reviews, erikwright+watch_chromium.org, jshin+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionExtend SplitStringIntoKeyValuePairs unit tests.
Extends the unit tests about corner cases to ensure that changes to the implementation don't break these.
Test cases of the internal helper function SplitStringIntoKeyValues are merged into tests of the public function SplitStringIntoKeyValuesPairs. This removes redundancy in the tests and improves coverage of SplitStringIntoKeyValuesPairs.
The helper function is removed from the public header.
BUG=NONE
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255758
Patch Set 1 #
Total comments: 9
Patch Set 2 : Remove the helper function in this CL. #Patch Set 3 : Test handling of newline char. #
Messages
Total messages: 21 (0 generated)
please review, thanks! This prepares for the next CL https://codereview.chromium.org/184233010/ .
I'm at IETF in London and then go on vacation next week. Can you send this to another reviewer? Cheers. On Wed, Mar 5, 2014 at 2:30 PM, <pneubeck@chromium.org> wrote: > Reviewers: willchan, > > Message: > please review, thanks! > This prepares for the next CL https://codereview.chromium.org/184233010/ . > > Description: > Extend SplitStringIntoKeyValuePairs unit tests. > > Extends the unit tests about corner cases to ensure that changes to the > implementation don't break these. > > BUG=NONE > > Please review this at https://codereview.chromium.org/184233009/ > > SVN Base: svn://svn.chromium.org/chrome/trunk/src > > Affected files (+94, -86 lines): > M base/strings/string_split_unittest.cc > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Please take a look, thanks!
https://codereview.chromium.org/184233009/diff/1/base/strings/string_split_un... File base/strings/string_split_unittest.cc (left): https://codereview.chromium.org/184233009/diff/1/base/strings/string_split_un... base/strings/string_split_unittest.cc:32: class SplitStringIntoKeyValuesTest : public testing::Test { 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. https://codereview.chromium.org/184233009/diff/1/base/strings/string_split_un... File base/strings/string_split_unittest.cc (right): https://codereview.chromium.org/184233009/diff/1/base/strings/string_split_un... base/strings/string_split_unittest.cc:50: ASSERT_EQ(2U, kv_pairs.size()); 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". https://codereview.chromium.org/184233009/diff/1/base/strings/string_split_un... base/strings/string_split_unittest.cc:70: EXPECT_TRUE(SplitStringIntoKeyValuePairs(",key1:value1,key2:value2,", 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. https://codereview.chromium.org/184233009/diff/1/base/strings/string_split_un... base/strings/string_split_unittest.cc:116: EXPECT_TRUE(SplitStringIntoKeyValuePairs("key1:value1 , key2:value2", Again, your trimming behaviors are fine, but you have to define them somewhere. https://codereview.chromium.org/184233009/diff/1/base/strings/string_split_un... base/strings/string_split_unittest.cc:140: std::string a("a ?!@#$%^&*()_+:/{}\\\tb"); Maybe you should have a \n in there too.
https://codereview.chromium.org/184233009/diff/1/base/strings/string_split_un... File base/strings/string_split_unittest.cc (left): https://codereview.chromium.org/184233009/diff/1/base/strings/string_split_un... base/strings/string_split_unittest.cc:32: class SplitStringIntoKeyValuesTest : public testing::Test { On 2014/03/06 17:44:31, Mark Mentovai wrote: > 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. Done. https://codereview.chromium.org/184233009/diff/1/base/strings/string_split_un... File base/strings/string_split_unittest.cc (right): https://codereview.chromium.org/184233009/diff/1/base/strings/string_split_un... base/strings/string_split_unittest.cc:50: ASSERT_EQ(2U, kv_pairs.size()); 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_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". https://codereview.chromium.org/184233009/diff/1/base/strings/string_split_un... base/strings/string_split_unittest.cc:116: EXPECT_TRUE(SplitStringIntoKeyValuePairs("key1:value1 , key2:value2", On 2014/03/06 17:44:31, Mark Mentovai wrote: > Again, your trimming behaviors are fine, but you have to define them somewhere. Please see the function comment in the next CL. https://codereview.chromium.org/184233009/diff/1/base/strings/string_split_un... base/strings/string_split_unittest.cc:140: std::string a("a ?!@#$%^&*()_+:/{}\\\tb"); On 2014/03/06 17:44:31, Mark Mentovai wrote: > Maybe you should have a \n in there too. Done.
LGTM, then. It would be nice if at some point after you document and clean up the current state, you could also improve the brain-dead interface. No pressure, these cleanups you’re doing here certainly improve things relative to how they were before you found them.
On 2014/03/06 19:19:04, Mark Mentovai wrote: > LGTM, then. It would be nice if at some point after you document and clean up > the current state, you could also improve the brain-dead interface. No pressure, > these cleanups you’re doing here certainly improve things relative to how they > were before you found them. Thanks for the thorough review, you made very good points (e.g. dropping the negative tests would be awesome).
The CQ bit was checked by pneubeck@chromium.org
Yeah, never woulda let that fly on initial review, but since as you point out there are already a bunch of callers, we’ve got to be a little more reactive here. (So that the commit queue doesn’t do the wrong thing: still LGTM.)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pneubeck@chromium.org/184233009/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg
The CQ bit was checked by pneubeck@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pneubeck@chromium.org/184233009/40001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pneubeck@chromium.org/184233009/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg
The CQ bit was checked by pneubeck@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pneubeck@chromium.org/184233009/40001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pneubeck@chromium.org/184233009/40001
Message was sent while issue was closed.
Change committed as 255758 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
