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

test/UtilTest.cpp

Index: test/UtilTest.cpp
===================================================================
--- a/test/UtilTest.cpp
+++ b/test/UtilTest.cpp
@@ -13,10 +13,10 @@
  *
  * 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"
-
+ */
+#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.

@@ -24,79 +24,156 @@
     std::wstring trimmed = TrimString(input);
     ASSERT_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.

+  {
+    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.

+  }
+
+  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.

+  {
+    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.

+  }
 }
 
 TEST(TrimTest, Trim00)
 {
-  TrimTestBody(L"", L"");
+  const std::wstring x = L"";
+  TrimTestBody(x, L"");
+  TrimLeftTestBody(x, L"");
+  TrimRightTestBody(x, L"");
 }
 
 TEST(TrimTest, Trim01)
 {
-  TrimTestBody(L" ", L"");
+  const std::wstring x = L" ";
+  TrimTestBody(x, L"");
+  TrimLeftTestBody(x, L"");
+  TrimRightTestBody(x, L"");
 }
 
 TEST(TrimTest, Trim02)
 {
-  TrimTestBody(L"\n", L"");
+  const std::wstring x = L"\n";
+  TrimTestBody(x, L"");
+  TrimLeftTestBody(x, L"");
+  TrimRightTestBody(x, L"");
 }
 
 TEST(TrimTest, Trim03)
 {
-  TrimTestBody(L"\r", L"");
+  const std::wstring x = L"\r";
+  TrimTestBody(x, L"");
+  TrimLeftTestBody(x, L"");
+  TrimRightTestBody(x, L"");
 }
 
 TEST(TrimTest, Trim04)
 {
-  TrimTestBody(L"\t", L"");
+  const std::wstring x = L"\t";
+  TrimTestBody(x, L"");
+  TrimLeftTestBody(x, L"");
+  TrimRightTestBody(x, L"");
 }
 
 TEST(TrimTest, Trim05)
 {
-  TrimTestBody(L"foo", L"foo");
+  const std::wstring x = L"foo";
+  TrimTestBody(x, L"foo");
+  TrimLeftTestBody(x, L"foo");
+  TrimRightTestBody(x, L"foo");
 }
 
 TEST(TrimTest, Trim06)
 {
-  TrimTestBody(L" foo", L"foo");
+  const std::wstring x = L" foo";
+  TrimTestBody(x, L"foo");
+  TrimLeftTestBody(x, L"foo");
+  TrimRightTestBody(x, L" foo");
 }
 
 TEST(TrimTest, Trim07)
 {
-  TrimTestBody(L"\r\nfoo", L"foo");
+  const std::wstring x = L"\r\nfoo";
+  TrimTestBody(x, L"foo");
+  TrimLeftTestBody(x, L"foo");
+  TrimRightTestBody(x, L"\r\nfoo");
 }
 
 TEST(TrimTest, Trim08)
 {
-  TrimTestBody(L"\tfoo", L"foo");
+  const std::wstring x = L"\tfoo";
+  TrimTestBody(x, L"foo");
+  TrimLeftTestBody(x, L"foo");
+  TrimRightTestBody(x, L"\tfoo");
 }
 
 TEST(TrimTest, Trim09)
 {
-  TrimTestBody(L"foo  ", L"foo");
+  const std::wstring x = L"foo  ";
+  TrimTestBody(x, L"foo");
+  TrimLeftTestBody(x, L"foo  ");
+  TrimRightTestBody(x, L"foo");
 }
 
 TEST(TrimTest, Trim10)
 {
-  TrimTestBody(L"foo\r\n", L"foo");
+  const std::wstring x = L"foo\r\n";
+  TrimTestBody(x, L"foo");
+  TrimLeftTestBody(x, L"foo\r\n");
+  TrimRightTestBody(x, L"foo");
 }
 
 TEST(TrimTest, Trim11)
 {
-  TrimTestBody(L"foo\t", L"foo");
+  const std::wstring x = L"foo\t";
+  TrimTestBody(x, L"foo");
+  TrimLeftTestBody(x, L"foo\t");
+  TrimRightTestBody(x, L"foo");
 }
 
 TEST(TrimTest, Trim12)
 {
-  TrimTestBody(L"foo bar", L"foo bar");
+  const std::wstring x = L"foo bar";
+  TrimTestBody(x, L"foo bar");
+  TrimLeftTestBody(x, L"foo bar");
+  TrimRightTestBody(x, L"foo bar");
 }
 
 TEST(TrimTest, Trim13)
 {
-  TrimTestBody(L"foo bar \r\n", L"foo bar");
+  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");
 }
 
 TEST(TrimTest, Trim14)
 {
-  TrimTestBody(L"  foo bar \r\n", L"foo bar");
+  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");
 }
+
+TEST(TrimTest, TrimPassesByValue)
+{
+  std::wstring x = L"foo bar  "; // not declared 'const', coould 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);
+  ASSERT_EQ(y, x); // argument variable not altered
+}
+
+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);
+  ASSERT_EQ(y, x);
+}