Issue #1234 - Remove 'CString' from PluginFilter.*

test/UtilTest.cpp

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
- * Copyright (C) 2006-2015 Eyeo GmbH
+ * Copyright (C) 2006-2016 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 #include <gtest/gtest.h>
 #include "../src/shared/Utils.h"
 
 namespace
 {
-  void TrimTestBody(std::wstring input, std::wstring expected)

[sergei, 2015/11/30 12:37:54]

It's not introduces in this change set but the comments are the same, const
references, EXPECT_EQ instead of ASSERT_EQ and let's rename it.

[Eric, 2015/11/30 15:54:07]

Done.

+  void TrimTestBody(const std::wstring& input, const std::wstring& expected)
   {
     std::wstring trimmed = TrimString(input);
-    ASSERT_EQ(expected, trimmed);
+    EXPECT_EQ(expected, trimmed);
   }
 
-  void TrimLeftTestBody(std::wstring input, std::wstring expected)

[sergei, 2015/11/30 12:37:54]

Why not to pass arguments as constant references?

[sergei, 2015/11/30 12:37:54]

It would be better to call it like
ExpectEqualToLeftTrimmed(expected, input);

[Eric, 2015/11/30 15:54:07]

I'm not going to bother with the renaming. I don't see any good reason to to
make it look like a Google Test primitive.

[Eric, 2015/11/30 15:54:08]

Done.

+  void TrimLeftTestBody(const std::wstring& input, const std::wstring& expected)
   {
     std::wstring trimmed = TrimStringLeft(input);
-    ASSERT_EQ(expected, trimmed);

[sergei, 2015/11/30 12:37:54]

It should not stop the tests, could you please use EXPECT_EQ?

[Eric, 2015/11/30 15:54:07]

Done.

+    EXPECT_EQ(expected, trimmed);
   }
 
-  void TrimRightTestBody(std::wstring input, std::wstring expected)

[sergei, 2015/11/30 12:37:54]

ExpectEqualToRightTrimmed(expected, input);

[sergei, 2015/11/30 12:37:54]

Same here, constant references

[Eric, 2015/11/30 15:54:07]

Done.

+  void TrimRightTestBody(const std::wstring& input, const std::wstring& expected)
   {
     std::wstring trimmed = TrimStringRight(input);
-    ASSERT_EQ(expected, trimmed);

[sergei, 2015/11/30 12:37:55]

EXPECT_EQ

[Eric, 2015/11/30 15:54:07]

Done.

+    EXPECT_EQ(expected, trimmed);
   }
 }
 
 TEST(TrimTest, Trim00)
 {
   const std::wstring x = L"";
   TrimTestBody(x, L"");
   TrimLeftTestBody(x, L"");
   TrimRightTestBody(x, L"");
 }
@@ skipping 103 matching lines @@
 }
 
 TEST(TrimTest, Trim14)
 {
   const std::wstring x = L"  foo bar \r\n";
   TrimTestBody(x, L"foo bar");
   TrimLeftTestBody(x, L"foo bar \r\n");
   TrimRightTestBody(x, L"  foo bar");
 }
 
+/*
+ * First of two tests verifying that TrimString() does not alter its argument.
+ * This one tests an ordinary, non-const variable as an argument.
+ * It differs from its companion only in the one line that declares the variable used as an argument.
+ */
 TEST(TrimTest, TrimPassesByValue)
 {
-  std::wstring x = L"foo bar  "; // not declared 'const', coould alter
+  std::wstring x = L"foo bar  "; // not declared 'const', could alter
   const std::wstring y = x;
   ASSERT_EQ(y, x);

[sergei, 2015/11/30 12:37:54]

EXPECT_EQ here and below

[Eric, 2015/11/30 15:54:08]

This statement is largely documentation. It's here to match the ASSERT at the
end. I did change the middle one.

   std::wstring trimmed = TrimString(x); // expect here pass by value

[sergei, 2015/11/30 12:37:54]

What if one calls it as `TrimString<std::wstring&>(x)`?
It's actually a bug in the test, we should explicitly specify the argument type
as a non-constant reference.

[Eric, 2015/11/30 15:54:08]

If you do that, you get in-place modification. I searched the code; there's no
occurrence of that syntax anywhere in our code. If this were a general purpose
library, I'd worry about it right now.
It's not a defect in the test. It's arguably a defect in 'TrimString', but if it
is, it's not one that has any significant consequences.

This test simply ensures that when the argument is a variable that it's being
passed by value so that it doesn't break existing code. I changed the
implementation significantly enough that I wanted verification on this point.

Incidentally, most, but not all, of the uses of TrimString are in-place
modifications. It would be good, in any case, to change them to use the explicit
'...InPlace...' variants in the new code. I didn't do it in this change set
because it's too much out of scope.

If we want library-quality code, we'd want all of the following variant
arguments
- T
- T&
- const T&
- T&&
It would take longer to write the unit tests for all of those than to write all
the variants. I don't see it being worth the time at present to do that. And if
we want to do it, it's a separate change and a new ticket #.

[sergei, 2015/11/30 16:36:03]

Acknowledged. It's another issue, let's leave it.

-  ASSERT_EQ(L"foo bar", trimmed);
+  EXPECT_EQ(L"foo bar", trimmed);
   ASSERT_EQ(y, x); // argument variable not altered
 }
 
+/*
+ * Second of two tests verifying that TrimString() does not alter its argument.
+ * This one tests a const variable as an argument.
+ */
 TEST(TrimTest, TrimBindsOnConstArg)

[sergei, 2015/11/30 12:37:55]

If it's an attempt to test whether there is some bug which modifies the string
passed into TrimString then I would say we likely don't need such test.

Even more, here is a bug in the test because the parameter of TrimString will a
copy of an argument and `TrimString<const std::wstring&>(x)` is not compilable.

[Eric, 2015/11/30 15:54:07]

It verifies the modification, yes. The real purpose of the test is to ensure
that the present function works with const arguments. That's why the test has
the name it does.
There's no defect in this test. It verifies exactly what it's supposed to do,
that T instantiates as 'std::wstring', that the const argument is used to
initialize a temporary argument object, and that the return value is correct.
The verification that the const variable 'x' hasn't changed is icing on the
cake.

`TrimString<const std::wstring&>(x)` doesn't need to be compilable. As I said
above, 'TrimString' isn't meant a completely general purpose template function. 

 {
   const std::wstring x = L"foo bar  "; // declared 'const'
   const std::wstring y = x;
   ASSERT_EQ(y, x);
   std::wstring trimmed = TrimString(x);
-  ASSERT_EQ(L"foo bar", trimmed);
+  EXPECT_EQ(L"foo bar", trimmed);
   ASSERT_EQ(y, x);
 }