Issue 2005 - Refactor working with strings in InputBuffer and OutputBuffer

test/CommunicationTest.cpp

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 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 <thread>
+#include <chrono>
+#include <memory>
 #include "../src/shared/AutoHandle.h"
 #include "../src/shared/Communication.h"
 
 const std::wstring pipeName(L"\\\\.\\pipe\\adblockplustests");
 
+TEST(CommunicationTest, PipeConnect)
+{
+  auto serverThread = std::thread([]
+  {
+    Communication::Pipe pipe(pipeName, Communication::Pipe::MODE_CREATE);
+  });
+  std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  ASSERT_NO_THROW(Communication::Pipe pipe(pipeName, Communication::Pipe::MODE_CONNECT));
+  serverThread.join();
+}
+
+TEST(CommunicationTest, PipeSendReceive)
+{
+  std::unique_ptr<Communication::Pipe> client;
+  auto clientThread = std::thread([&client]
+  {
+    while(!client)

[Eric, 2015/02/18 18:12:47]

There's the possibility of an infinite loop if there's a persistent error in the
implementation of the Pipe constructor. There should be a maximum number of
iterations; if it reaches the limit, the test should abort.

It may be desirable to add a small delay (1-10 ms, say) in case of exception to
keep this loop from churning too rapidly.

[sergei, 2015/02/19 17:26:57]

I've added the counter and changed `yeild` by `sleep_for`. On my computer it
usually takes two cycles.

+    {
+      try
+      {
+        client.reset(new Communication::Pipe(pipeName, Communication::Pipe::MODE_CONNECT));
+      }
+      catch(...)
+      {
+        std::this_thread::yield();
+      }
+    }
+  });
+  Communication::Pipe server(pipeName, Communication::Pipe::MODE_CREATE);

[Eric, 2015/02/18 18:12:47]

Shouldn't creating the pipe come before connecting to it? If we were seriously
improving these tests, we'd have tests for each order of instantiation. That is,
isn't this test trying to exercise basic behavior, rather than edge cases?

[sergei, 2015/02/19 17:26:57]

This particular test is aimed to exercise the sending of data through the pipe
and not connecting. BTW, our bho (client) and engine (server) actually work as
in this test. The another order is used in `TEST(CommunicationTest,
PipeConnect)`, so I would say there is no reason to create another tests for the
present.

+  clientThread.join();
+
+  // send to the server
+  {
+    Communication::OutputBuffer message;
+    message << Communication::ProcType::PROC_IS_WHITELISTED_URL
+      << std::string("This is a test message") << 64ll << 32 << true << false;
+    client->WriteMessage(message);
+  }
+  // receive on the server and send back
+  {
+    Communication::OutputBuffer response;
+    Communication::InputBuffer message = server.ReadMessage();
+    Communication::ProcType proc;
+    message >> proc;
+    EXPECT_EQ(Communication::ProcType::PROC_IS_WHITELISTED_URL, proc);
+    response << Communication::ProcType::PROC_IS_FIRST_RUN_ACTION_NEEDED;
+
+    std::string text;
+    message >> text;
+    EXPECT_EQ("This is a test message", text);
+    response << std::string("Response");
+
+    int64_t integer64;
+    message >> integer64;
+    EXPECT_EQ(64ll, integer64);
+    response << 46ll;
+
+    int32_t integer32;
+    message >> integer32;
+    EXPECT_EQ(32, integer32);
+    response << 23;
+
+    bool boolean;
+    message >> boolean;
+    EXPECT_TRUE(boolean);
+    response << false;
+
+    message >> boolean;
+    EXPECT_FALSE(boolean);
+    response << true;
+
+    server.WriteMessage(response);
+  }
+
+  // receive on the client
+  {
+    Communication::InputBuffer message = client->ReadMessage();
+    Communication::ProcType proc;
+    message >> proc;
+    EXPECT_EQ(Communication::ProcType::PROC_IS_FIRST_RUN_ACTION_NEEDED, proc);
+
+    std::string text;
+    message >> text;
+    EXPECT_EQ("Response", text);
+
+    int64_t integer64;
+    message >> integer64;
+    EXPECT_EQ(46ll, integer64);
+
+    int32_t integer32;
+    message >> integer32;
+    EXPECT_EQ(23, integer32);
+
+    bool boolean;
+    message >> boolean;
+    EXPECT_FALSE(boolean);
+
+    message >> boolean;
+    EXPECT_TRUE(boolean);
+  }
+}
+
 namespace
 {
-  DWORD WINAPI CreatePipe(LPVOID param)
-  {
-    Communication::Pipe pipe(pipeName, Communication::Pipe::MODE_CREATE);
-    return 0;
-  }
-
-  DWORD WINAPI ReceiveSend(LPVOID param)
-  {
-    Communication::Pipe pipe(pipeName, Communication::Pipe::MODE_CREATE);
-
-    Communication::InputBuffer message = pipe.ReadMessage();
-
-    std::string stringValue;
-    std::wstring wstringValue;
-    int64_t int64Value;
-    int32_t int32Value;
-    bool boolValue;
-    message >> stringValue >> wstringValue >> int64Value >> int32Value >> boolValue;
-
-    stringValue += " Received";
-    wstringValue += L" \u043f\u0440\u0438\u043d\u044f\u0442\u043e";
-    int64Value += 1;
-    int32Value += 2;
-    boolValue = !boolValue;
-
-    Communication::OutputBuffer response;
-    response << stringValue << wstringValue << int64Value << int32Value << boolValue;
-    pipe.WriteMessage(response);
-
-    return 0;
-  }
-}
-
-TEST(CommunicationTest, ConnectPipe)
-{
-  AutoHandle thread(CreateThread(0, 0, CreatePipe, 0, 0, 0));
-
-  Sleep(100);
-
-  ASSERT_NO_THROW(Communication::Pipe pipe(pipeName, Communication::Pipe::MODE_CONNECT));
-}
-
-TEST(CommunicationTest, SendReceive)
-{
-  AutoHandle thread(CreateThread(0, 0, ReceiveSend, 0, 0, 0));
-
-  Sleep(100);
-
-  Communication::Pipe pipe(pipeName, Communication::Pipe::MODE_CONNECT);
-
-  Communication::OutputBuffer message;
-  message << std::string("Foo") << std::wstring(L"Bar") << int64_t(9876543210L) << int32_t(5) << true;
-  pipe.WriteMessage(message);
-
-  Communication::InputBuffer response = pipe.ReadMessage();
-
-  std::string stringValue;
-  std::wstring wstringValue;
-  int64_t int64Value;
-  int32_t int32Value;
-  bool boolValue;
-
-  ASSERT_ANY_THROW(response >> wstringValue);
-  ASSERT_ANY_THROW(response >> int64Value);
-  ASSERT_ANY_THROW(response >> int32Value);
-  ASSERT_ANY_THROW(response >> boolValue);
-
-  response >> stringValue >> wstringValue;
-
-  ASSERT_ANY_THROW(response >> stringValue);
-  ASSERT_ANY_THROW(response >> wstringValue);
-  ASSERT_ANY_THROW(response >> int32Value);
-  ASSERT_ANY_THROW(response >> boolValue);
-
-  response >> int64Value >> int32Value >> boolValue;
-
-  ASSERT_ANY_THROW(response >> stringValue);
-  ASSERT_ANY_THROW(response >> wstringValue);
-  ASSERT_ANY_THROW(response >> int64Value);
-  ASSERT_ANY_THROW(response >> int32Value);
-  ASSERT_ANY_THROW(response >> boolValue);
-
-  ASSERT_EQ("Foo Received", stringValue);
-  ASSERT_EQ(L"Bar \u043f\u0440\u0438\u043d\u044f\u0442\u043e", wstringValue);
-  ASSERT_EQ(9876543211L, int64Value);
-  ASSERT_EQ(7, int32Value);
-  ASSERT_FALSE(boolValue);
-}
-
-void SendReceiveStrings(const std::vector<std::string>& src)
-{
-  Communication::OutputBuffer outputBuffer;
-  outputBuffer << src;
-  Communication::InputBuffer inputBuffer(outputBuffer.Get());
-  std::vector<std::string> dst;
-  inputBuffer >> dst;
-  auto dstSize = dst.size();
-  ASSERT_EQ(dstSize, src.size());
-  for (auto i = 0; i < dstSize; ++i)
-  {
-    EXPECT_EQ(dst[i], src[i]);
-  }
-}
-
-TEST(InputOutputBuffersTests, EmptyStrings)
+  template<typename TestedType, typename AnotherType>
+  void SendReceiveTypes(TestedType value, AnotherType anotherTypeValue)
+  {
+    Communication::OutputBuffer message;
+    message << anotherTypeValue << value;
+    Communication::InputBuffer response(message.Get());
+    TestedType received;
+    ASSERT_ANY_THROW(response >> received);
+    AnotherType anotherTypeValueReceived;
+    response >> anotherTypeValueReceived;
+    response >> received;
+    EXPECT_EQ(value, received);
+  }
+}
+
+TEST(CommunicationTest, TypeProc)
+{
+  SendReceiveTypes(Communication::ProcType::PROC_GET_ELEMHIDE_SELECTORS, std::string("non-ProcType"));
+}
+
+TEST(CommunicationTest, TypeString)
+{
+  int32_t nonStringTypeValue = 10;
+  SendReceiveTypes(std::string("stringValue"), nonStringTypeValue);
+}
+
+TEST(CommunicationTest, SendReceiveEmptyString)

[Eric, 2015/02/18 18:12:47]

How about 'TypeStringNarrowEmpty'?

[sergei, 2015/02/19 17:26:57]

For this case and below, TypeString is not what we test here, we test that
sending and receiving of the specific strings work.

Regarding Narrow and Wide, I find the current version to be close to
`ToWstring`. Would not it be better to use Utf8 and Utf16 instead?

Regarding the underscore, I really find it easier to read and understand with it
than without it, but I can remove it of course to be consistent.
SendStringReceiveWString
vs
SendString_ReceiveWString

+{
+  Communication::OutputBuffer message;
+  message << std::string();
+  Communication::InputBuffer response(message.Get());
+  std::string received = "clean me";
+  response >> received;
+  EXPECT_EQ("", received);
+}
+
+TEST(CommunicationTest, TypeStringSendWideReceiveWide)
+{
+  int32_t nonStringTypeValue = 10;
+  SendReceiveTypes(std::wstring(L"stringValue"), nonStringTypeValue);
+}
+
+TEST(CommunicationTest, SendString_ReceiveWString)

[Eric, 2015/02/18 18:12:47]

How about 'TypeStringSendNarrowReceiveWide'?

+{
+  Communication::OutputBuffer message;
+  message << std::string("test");
+  Communication::InputBuffer response(message.Get());
+  std::wstring received;
+  response >> received;
+  EXPECT_EQ(L"test", received);
+}
+
+TEST(CommunicationTest, SendWString_ReceiveString)

[Eric, 2015/02/18 18:12:47]

How about 'TypeStringSendWideReceiveNarrow'?

+{
+  Communication::OutputBuffer message;
+  message << std::wstring(L"test");
+  Communication::InputBuffer response(message.Get());
+  std::string received;
+  response >> received;
+  EXPECT_EQ("test", received);
+}
+
+TEST(CommunicationTest, SendStrings_ReceiveWStrings)

[Eric, 2015/02/18 18:12:47]

How about 'TypeStringSendNarrowReceiveWide'?

+{
+  Communication::OutputBuffer message;
+  std::vector<std::string> strings;
+  strings.emplace_back("test0");
+  strings.emplace_back("test1");
+  message << strings;
+  Communication::InputBuffer response(message.Get());
+  std::vector<std::wstring> received;
+  response >> received;
+  ASSERT_EQ(2u, received.size());
+  EXPECT_EQ(L"test0", received[0]);
+  EXPECT_EQ(L"test1", received[1]);
+}
+
+TEST(CommunicationTest, TypeInt64)
+{
+  SendReceiveTypes<int64_t>(10, std::string("non-ProcType"));
+}
+
+TEST(CommunicationTest, TypeInt32)
+{
+  SendReceiveTypes<int32_t>(10, std::string("non-ProcType"));
+}
+
+TEST(CommunicationTest, TypeBool)
+{
+  SendReceiveTypes<bool>(true, std::string("non-ProcType"));
+  SendReceiveTypes<bool>(false, std::string("non-ProcType"));
+}
+
+namespace
+{
+  void SendReceiveStrings(const std::vector<std::string>& src)
+  {
+    Communication::OutputBuffer outputBuffer;
+    outputBuffer << src;
+    Communication::InputBuffer inputBuffer(outputBuffer.Get());
+    std::vector<std::string> dst;
+    inputBuffer >> dst;
+    auto dstSize = dst.size();
+    ASSERT_EQ(src.size(), dstSize);
+    for (auto i = 0u; i < dstSize; ++i)
+    {
+      EXPECT_EQ(src[i], dst[i]);
+    }
+  }
+
+  std::vector<std::wstring> SendReceiveStringsToWStrings(const std::vector<std::string>& src)
+  {
+    Communication::OutputBuffer outputBuffer;
+    outputBuffer << src;
+    Communication::InputBuffer inputBuffer(outputBuffer.Get());
+    std::vector<std::wstring> dst;
+    inputBuffer >> dst;
+    return dst;
+  }
+}
+
+TEST(InputOutputBuffersTests, InputOutputBufferEmptyStrings)

[Eric, 2015/02/18 18:12:47]

Consistent naming would start this name and below with "TestStrings..." or
"TestStringVector...". I suggest the second form because noticing the difference
between "String" and "Strings" inside a InterCaps name is just too slight a
difference to be a good name.

[sergei, 2015/02/19 17:26:57]

We don't start test names with Test. I've fixed 'InputOutputBuffersTests' to
'InputOutputBuffersTest' and removed 'InputOutputBuffer' from test name.

 {
   SendReceiveStrings(std::vector<std::string>());
 }
 
-TEST(InputOutputBuffersTests, StringsWithOneValue)
+TEST(InputOutputBuffersTests, InputOutputBufferEmptyStringsToWideStrings)
+{
+  EXPECT_EQ(0u, SendReceiveStringsToWStrings(std::vector<std::string>()).size());
+}
+
+TEST(InputOutputBuffersTests, InputOutputBufferStringsWithOneValue)
 {
   std::vector<std::string> src;
   src.emplace_back("string1");
   SendReceiveStrings(src);
 }
 
-TEST(InputOutputBuffersTests, MultivalueStrings)
+TEST(InputOutputBuffersTests, InputOutputBufferStringsWithOneValueToWStrings)
+{
+  std::vector<std::string> src;
+  src.emplace_back("string1");
+  auto dst = SendReceiveStringsToWStrings(src);
+  ASSERT_EQ(1u, dst.size());
+  EXPECT_EQ(L"string1", dst[0]);
+}
+
+TEST(InputOutputBuffersTests, InputOutputBufferWithMultivalueStrings)
 {
   std::vector<std::string> src;
   src.emplace_back("string1");
   src.emplace_back("str2");
   src.emplace_back("value");
   SendReceiveStrings(src);
+}
+
+TEST(InputOutputBuffersTests, InputOutputBufferWithMultivalueStringsToWStrings)
+{
+  std::vector<std::string> src;
+  src.emplace_back("string1");
+  src.emplace_back("str2");
+  src.emplace_back("value");
+  auto dst = SendReceiveStringsToWStrings(src);
+  ASSERT_EQ(3u, dst.size());
+  EXPECT_EQ(L"string1", dst[0]);
+  EXPECT_EQ(L"str2", dst[1]);
+  EXPECT_EQ(L"value", dst[2]);
 }