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

test/plugin/UtilTest.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 56 matching lines @@
   ASSERT_FALSE(BeginsWithForTesting(L"foo", L"foobar"));
 }
 
 TEST(BeginsWith, LongerBad)
 {
   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.

+  ASSERT_EQ(L"", ToLowerString(L""));
 }
 
 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.

+  ASSERT_EQ(L"foobar", ToLowerString(L"FooBAR"));
 }
 
 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.

+  std::wstring input(L"\0BAR", 4);
+  ASSERT_EQ(4, input.length());
+
+  std::wstring actual;
+  ASSERT_NO_THROW({ actual = ToLowerString(input); });
+  // White-box tests for further verification of no-crash behavior
+  ASSERT_EQ(4, actual.length());
+  ASSERT_EQ(L"", std::wstring(actual.c_str()));
 }
 
 TEST(ToLowerString, InternalNull)
 {
-  ASSERT_EQ(std::wstring(L"foo\0BAR"), ToLowerString(L"Foo\0BAR"));
+  std::wstring input(L"Foo\0BAR", 7);
+  ASSERT_EQ(7, input.length());
+
+  std::wstring actual;
+  ASSERT_NO_THROW({ actual = ToLowerString(input); });
+  // White-box tests for further verification of no-crash behavior
+  ASSERT_EQ(7, actual.length());
+  ASSERT_EQ(L"foo", std::wstring(actual.c_str()));
 }