Issue #1234 - Remove 'CString' From 'ToLowerString()'

test/plugin/UtilTest.cpp

Index: test/plugin/UtilTest.cpp
===================================================================
--- a/test/plugin/UtilTest.cpp
+++ b/test/plugin/UtilTest.cpp
@@ -71,3 +71,23 @@
 {
   ASSERT_FALSE(BeginsWithForTesting(L"foo", L"barfoo"));
 }
+
+TEST(ToLowerString, Empty)
+{
+  ASSERT_EQ(std::wstring(), ToLowerString(L""));

[sergei, 2015/12/16 10:32:45]

It's still not clear why we are using ASSERT in a new code.

[Eric, 2015/12/16 13:53:27]

Because I prefer ASSERT.

And as I've said before, there's no difference at all between ASSERT and EXPECT
for one line tests. And in most cases you want to terminate early if you fail an
assertion. It's an unusual case where a second assertion failure in a unit test
provides actual value.

+}
+
+TEST(ToLowerString, NotEmpty)
+{
+  ASSERT_EQ(std::wstring(L"foo"), ToLowerString(L"Foo"));

[sergei, 2015/12/16 10:32:45]

Is it necessary to explicitly construct std::wstring here?

[Eric, 2015/12/16 13:53:27]

I checked. No.

They were in there because I've had trouble with the ASSERT macros not properly
inferring type conversions before.

I took them out.

+}
+
+TEST(ToLowerString, NullWithExtra)
+{
+  ASSERT_EQ(std::wstring(L"\0BAR"), ToLowerString(L"\0BAR"));

[sergei, 2015/12/16 10:32:45]

I think this test and below are incorrect. Mainly because the rest of the
C-string after zero value character is dropped while `std::wstring` is being
constructed. One should use constructor which takes into account the length of
the string, thus `std::wstring(L"\0BAR", 4)`. One also should explicitly
construct `std::wstring` with the constructor mentioned above to pass it in
`ToLowerString`.

On the other hand if you wanted to indicate that we are ignoring whether the
rest of the string after zero value character is converted or not then it would
be better to compare only the beginning of the string, e.g something like
EXPECT_TRUE(BeginsWith(ToLowerString(std::wstring(L"\0BAR", 4)), L"");
EXPECT_TRUE(BeginsWith(ToLowerString(std::wstring(L"foo\0BAR", 7)), L"Foo");

If we proceed with the version which drops the tail after zero value character
then it should be like
EXPECT_EQ(std::wstring(), ToLowerString(std::wstring(L"\0BAR", 4)));
EXPECT_EQ(L"Foo", ToLowerString(std::wstring(L"foo\0BAR", 7)));

[Eric, 2015/12/16 13:53:27]

Yep. Fixed that. I keep forgetting that constructing from 'char[]' doesn't infer
the length of the array; it's a legacy behavior.

I've rewritten the tests to be more clear about what actually matters in this
situation.

+}
+
+TEST(ToLowerString, InternalNull)
+{
+  ASSERT_EQ(std::wstring(L"foo\0BAR"), ToLowerString(L"Foo\0BAR"));
+}