Issue 2005 - Refactor working with strings in InputBuffer and OutputBuffer

test/CommunicationTest.cpp

Index: test/CommunicationTest.cpp
diff --git a/test/CommunicationTest.cpp b/test/CommunicationTest.cpp
index fb9d4c34f35220fdaab146f480a15be2e9fff40d..3b72af03409f48f0b643ea1893c2ff50fcdd96fc 100644
--- a/test/CommunicationTest.cpp
+++ b/test/CommunicationTest.cpp
@@ -17,134 +17,283 @@
 
 #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");
 
-namespace
+TEST(CommunicationTest, PipeConnect)
 {
-  DWORD WINAPI CreatePipe(LPVOID param)
+  auto serverThread = std::thread([]
   {
     Communication::Pipe pipe(pipeName, Communication::Pipe::MODE_CREATE);
-    return 0;
+  });
+  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);
   }
 
-  DWORD WINAPI ReceiveSend(LPVOID param)
+  // receive on the client
   {
-    Communication::Pipe pipe(pipeName, Communication::Pipe::MODE_CREATE);
+    Communication::InputBuffer message = client->ReadMessage();
+    Communication::ProcType proc;
+    message >> proc;
+    EXPECT_EQ(Communication::ProcType::PROC_IS_FIRST_RUN_ACTION_NEEDED, proc);
 
-    Communication::InputBuffer message = pipe.ReadMessage();
+    std::string text;
+    message >> text;
+    EXPECT_EQ("Response", text);
 
-    std::string stringValue;
-    std::wstring wstringValue;
-    int64_t int64Value;
-    int32_t int32Value;
-    bool boolValue;
-    message >> stringValue >> wstringValue >> int64Value >> int32Value >> boolValue;
+    int64_t integer64;
+    message >> integer64;
+    EXPECT_EQ(46ll, integer64);
 
-    stringValue += " Received";
-    wstringValue += L" \u043f\u0440\u0438\u043d\u044f\u0442\u043e";
-    int64Value += 1;
-    int32Value += 2;
-    boolValue = !boolValue;
+    int32_t integer32;
+    message >> integer32;
+    EXPECT_EQ(23, integer32);
 
-    Communication::OutputBuffer response;
-    response << stringValue << wstringValue << int64Value << int32Value << boolValue;
-    pipe.WriteMessage(response);
+    bool boolean;
+    message >> boolean;
+    EXPECT_FALSE(boolean);
 
-    return 0;
+    message >> boolean;
+    EXPECT_TRUE(boolean);
   }
 }
 
-TEST(CommunicationTest, ConnectPipe)
+namespace
 {
-  AutoHandle thread(CreateThread(0, 0, CreatePipe, 0, 0, 0));
+  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);
+  }
+}
 
-  Sleep(100);
+TEST(CommunicationTest, TypeProc)
+{
+  SendReceiveTypes(Communication::ProcType::PROC_GET_ELEMHIDE_SELECTORS, std::string("non-ProcType"));
+}
 
-  ASSERT_NO_THROW(Communication::Pipe pipe(pipeName, Communication::Pipe::MODE_CONNECT));
+TEST(CommunicationTest, TypeString)
+{
+  int32_t nonStringTypeValue = 10;
+  SendReceiveTypes(std::string("stringValue"), nonStringTypeValue);
 }
 
-TEST(CommunicationTest, SendReceive)
+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

 {
-  AutoHandle thread(CreateThread(0, 0, ReceiveSend, 0, 0, 0));
+  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);
+}
 
-  Sleep(100);
+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);
+}
 
-  Communication::Pipe pipe(pipeName, Communication::Pipe::MODE_CONNECT);
+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;
-  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)
+  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)
   {
-    EXPECT_EQ(dst[i], src[i]);
+    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, EmptyStrings)
+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]);
 }