Issue 1115 - Some yahoo page not correctly shown on IE8 when ABP enabled

test/UtilForEachQueryStringParameterTest.cpp

Index: test/UtilForEachQueryStringParameterTest.cpp
diff --git a/test/UtilForEachQueryStringParameterTest.cpp b/test/UtilForEachQueryStringParameterTest.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..f36b553903dc5e5f571d3508a500550a17dea646
--- /dev/null
+++ b/test/UtilForEachQueryStringParameterTest.cpp
@@ -0,0 +1,192 @@
+/*
+ * 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
+ * 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 <cstdint>
+#include <gtest/gtest.h>
+#include "../src/shared/Utils.h"
+
+namespace
+{
+  struct ParameterHandler

[Eric, 2015/02/12 17:24:41]

The code reads much better with this class present.

Now that I've seen the new version, I'll just observe that there's lot of
duplication in verifying the resulting vectors. For whatever reason, it bothers
me less.

That's partly because our current compiler doesn't support C++11 uniform
initializer lists, so writing a single expression for expected results isn't
possible.

[sergei, 2015/02/13 15:21:56]

Yes, it's not possible due to compiler limitation, in addition, having
verification statements in the body of test (not in the common function) can be
useful because in that case we immediately know the exact location of the
failure.

[Eric, 2015/02/13 16:33:43]

I've written such things, and you can get the location by how you write the
common verification function. On the other hand, I've used lots of unit test
frameworks but I've not needed to do this in google-test, so I'm not sure what
the details and limitations are for the package we're using...

+  {
+    bool operator()(const std::wstring& parameter, const std::wstring& value)
+    {
+      parameters.emplace_back(parameter, value);
+      return true;
+    }
+    struct Parameter
+    {
+      Parameter(const std::wstring& name, const std::wstring& value)
+        : name(name), value(value)
+      {
+      }
+      std::wstring name;
+      std::wstring value;
+    };
+    std::vector<Parameter> parameters;
+  };
+
+  struct LimitedParameterHandler : ParameterHandler

[Eric, 2015/02/12 17:24:41]

I think I'd prefer than ParameterHandler take the moral equivalent of
shouldContinue() here as a constructor argument. My reason for this is that the
code here only applies to a single test case and so the code for that test is
split between two, separate, non-contiguous types.

I'd be satisfied, though, if you declared this call immediately before the unit
test in which it's used.

[sergei, 2015/02/13 15:21:56]

I thought about it when I was creating of it and decided that it would be better
to have it as separate class. 
It's moved before the usage.

[Eric, 2015/02/13 16:33:43]

If we had more variation in the tests, subclassing each time would become
problematic. With just one variation, it's fine.

+  {
+    LimitedParameterHandler(uint32_t limit)
+      : callCounter(0), limit(limit)
+    {
+    }
+    bool operator()(const std::wstring& parameter, const std::wstring& value)
+    {
+      ++callCounter;
+      return shouldContinue() && ParameterHandler::operator()(parameter, value) && shouldContinue();

[Eric, 2015/02/12 17:24:41]

No need for the first shouldContinue() call here. The only time it could matter
in the present case is when 'limit' is zero.

[sergei, 2015/02/13 15:21:56]

I've added it with another intention but it's fine without it. Removed.

+    }
+    bool shouldContinue() const
+    {
+      return parameters.size() < limit;
+    }
+    uint32_t callCounter;
+    uint32_t limit;
+  };
+}
+
+TEST(ForEachQueryStringParameterTest, EmptyParameterHandler)
+{
+  ASSERT_THROW(ForEachQueryStringParameter(L"some=query&string",
+    std::function<bool(const std::wstring&, const std::wstring&)>()), std::bad_function_call);
+}
+
+TEST(ForEachQueryStringParameterTest, EmptyQueryString)
+{
+  ParameterHandler parameterHandler;
+  ForEachQueryStringParameter(L"", std::ref(parameterHandler));

[Eric, 2015/02/12 17:24:41]

Do we need reference wrappers? It seems like would should be able to declare the
ForEach... function arguments as rvalue references rather than regular
references. 

If we do, we should likely use 'std::cref' here.

[sergei, 2015/02/13 15:21:56]

The tested function accepts `std::function` but we are passing an object with
the `operator()`. When `std::function` is implicitly constructed from it, it
copies the passed object, in that case the tests fail because the tested
function will populate the temporary object held by `std::function`. To avoid
such copying one can pass std::reference_wrapper or use some another technique,
like put `ParameterHandler` members into shared object or use lambda which
captures it by reference. I guess, `std::reference_wrapper` is easier here.

We cannot use std::cref because the object is not constant, we need call
ParameterHandler::operator()(...) which has no `const` modifier. An attempt to
use `std::cref` results into compilation error.

[Eric, 2015/02/13 16:33:43]

You may have misunderstood. I was suggesting declaring

    void ForEachQueryStringParameter(..., const std::function<...>&&
parameterHandler)

instead of 

    void ForEachQueryStringParameter(..., const std::function<...>&&
parameterHandler)
My suggestion boils down to avoiding copy by replacing it with a move.
I'm misunderstanding std::cref, I guess. I would have called it std::pure_ref if
that's its actual behavior.

+  EXPECT_EQ(0, parameterHandler.parameters.size());
+}
+
+TEST(ForEachQueryStringParameterTest, QueryStringWithOneNormalParameter)
+{
+  ParameterHandler parameterHandler;
+  ForEachQueryStringParameter(L"param=value", std::ref(parameterHandler));
+  ASSERT_EQ(1, parameterHandler.parameters.size());
+  EXPECT_EQ(L"param", parameterHandler.parameters[0].name);
+  EXPECT_EQ(L"value", parameterHandler.parameters[0].value);
+}
+
+TEST(ForEachQueryStringParameterTest, QueryStringWithSeveralNormalParameters)
+{
+  ParameterHandler parameterHandler;
+  ForEachQueryStringParameter(L"param1=value1&param2=value2&param3=value3", std::ref(parameterHandler));
+  ASSERT_EQ(3, parameterHandler.parameters.size());
+  EXPECT_EQ(L"param1", parameterHandler.parameters[0].name);
+  EXPECT_EQ(L"value1", parameterHandler.parameters[0].value);
+  EXPECT_EQ(L"param2", parameterHandler.parameters[1].name);
+  EXPECT_EQ(L"value2", parameterHandler.parameters[1].value);
+  EXPECT_EQ(L"param3", parameterHandler.parameters[2].name);
+  EXPECT_EQ(L"value3", parameterHandler.parameters[2].value);
+}
+
+TEST(ForEachQueryStringParameterTest, QueryStringWithEmptyParameterAtTheEnd)
+{
+  ParameterHandler parameterHandler;
+  ForEachQueryStringParameter(L"param=value&", std::ref(parameterHandler));
+  ASSERT_EQ(2, parameterHandler.parameters.size());
+  EXPECT_EQ(L"param", parameterHandler.parameters[0].name);
+  EXPECT_EQ(L"value", parameterHandler.parameters[0].value);
+  EXPECT_EQ(L"", parameterHandler.parameters[1].name);
+  EXPECT_EQ(L"", parameterHandler.parameters[1].value);
+}
+
+TEST(ForEachQueryStringParameterTest, QueryStringWithEmptyParameterAtTheBeginning)
+{
+  ParameterHandler parameterHandler;
+  ForEachQueryStringParameter(L"&param=value", std::ref(parameterHandler));
+  ASSERT_EQ(2, parameterHandler.parameters.size());
+  EXPECT_EQ(L"", parameterHandler.parameters[0].name);
+  EXPECT_EQ(L"", parameterHandler.parameters[0].value);
+  EXPECT_EQ(L"param", parameterHandler.parameters[1].name);
+  EXPECT_EQ(L"value", parameterHandler.parameters[1].value);
+}
+
+TEST(ForEachQueryStringParameterTest, ParameterWithoutAssignSign)
+{
+  ParameterHandler parameterHandler;
+  ForEachQueryStringParameter(L"param", std::ref(parameterHandler));
+  ASSERT_EQ(1, parameterHandler.parameters.size());
+  EXPECT_EQ(L"param", parameterHandler.parameters[0].name);
+  EXPECT_EQ(L"", parameterHandler.parameters[0].value);
+}
+
+TEST(ForEachQueryStringParameterTest, ParametersWithoutAssignSign)
+{
+  ParameterHandler parameterHandler;
+  ForEachQueryStringParameter(L"param1&param2&param3", std::ref(parameterHandler));
+  ASSERT_EQ(3, parameterHandler.parameters.size());
+  EXPECT_EQ(L"param1", parameterHandler.parameters[0].name);
+  EXPECT_EQ(L"", parameterHandler.parameters[0].value);
+  EXPECT_EQ(L"param2", parameterHandler.parameters[1].name);
+  EXPECT_EQ(L"", parameterHandler.parameters[1].value);
+  EXPECT_EQ(L"param3", parameterHandler.parameters[2].name);
+  EXPECT_EQ(L"", parameterHandler.parameters[2].value);
+}
+
+TEST(ForEachQueryStringParameterTest, ParameterWithSeveralAssignSigns)
+{
+  ParameterHandler parameterHandler;
+  ForEachQueryStringParameter(L"param1=value1.1=value1.2", std::ref(parameterHandler));
+  ASSERT_EQ(1, parameterHandler.parameters.size());
+  EXPECT_EQ(L"param1", parameterHandler.parameters[0].name);
+  EXPECT_EQ(L"value1.1=value1.2", parameterHandler.parameters[0].value);
+}
+
+TEST(ForEachQueryStringParameterTest, ParameterWithEmptyValue)
+{
+  ParameterHandler parameterHandler;
+  ForEachQueryStringParameter(L"param1=", std::ref(parameterHandler));
+  ASSERT_EQ(1, parameterHandler.parameters.size());
+  EXPECT_EQ(L"param1", parameterHandler.parameters[0].name);
+  EXPECT_EQ(L"", parameterHandler.parameters[0].value);
+}
+
+TEST(ForEachQueryStringParameterTest, StopProcessingWhenHandlerReturnsFalse)
+{
+  LimitedParameterHandler parameterHandler(2);
+  ForEachQueryStringParameter(L"param1=value1&param2=value2&param3=value3", std::ref(parameterHandler));
+  EXPECT_EQ(2, parameterHandler.callCounter);
+  ASSERT_EQ(2, parameterHandler.parameters.size());
+  EXPECT_EQ(L"param1", parameterHandler.parameters[0].name);
+  EXPECT_EQ(L"value1", parameterHandler.parameters[0].value);
+  EXPECT_EQ(L"param2", parameterHandler.parameters[1].name);
+  EXPECT_EQ(L"value2", parameterHandler.parameters[1].value);
+}
+
+TEST(ForEachQueryStringParameterTest, ParameterWithoutName)
+{
+  ParameterHandler parameterHandler;
+  ForEachQueryStringParameter(L"=param", std::ref(parameterHandler));
+  ASSERT_EQ(1, parameterHandler.parameters.size());
+  EXPECT_EQ(L"", parameterHandler.parameters[0].name);
+  EXPECT_EQ(L"param", parameterHandler.parameters[0].value);
+}
+
+TEST(ForEachQueryStringParameterTest, ParametersWithoutName)
+{
+  ParameterHandler parameterHandler;
+  ForEachQueryStringParameter(L"=param1&=param2&=param3", std::ref(parameterHandler));
+  ASSERT_EQ(3, parameterHandler.parameters.size());
+  EXPECT_EQ(L"", parameterHandler.parameters[0].name);
+  EXPECT_EQ(L"param1", parameterHandler.parameters[0].value);
+  EXPECT_EQ(L"", parameterHandler.parameters[1].name);
+  EXPECT_EQ(L"param2", parameterHandler.parameters[1].value);
+  EXPECT_EQ(L"", parameterHandler.parameters[2].name);
+  EXPECT_EQ(L"param3", parameterHandler.parameters[2].value);
+}