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
+ * 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.
  *
@@ skipping 20 matching lines @@
   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.

+    for (int i = 0; !client && i < 100; ++i)

[Eric, 2015/05/14 13:53:38]

There's no need to guard the loop with the term "!client" if you move the
assertion below to appear before the declaration of 'clientThread'

[sergei, 2015/05/15 09:45:55]

Before declaration of `clientThread` `client` is always `nullptr`.

     {
       try
       {
         client.reset(new Communication::Pipe(pipeName, Communication::Pipe::MODE_CONNECT));
       }
       catch(...)
       {
-        std::this_thread::yield();
+        std::this_thread::sleep_for(i * std::chrono::milliseconds(20));
       }
     }
   });
   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();
-
+  ASSERT_TRUE(client);

[Eric, 2015/05/14 13:53:38]

This assertion should appear before the declaration of 'clientThread'. If you're
going to check it at all and fail the test based on it, you should test it as
soon as possible.

   // send to the server
   {
     Communication::OutputBuffer message;
-    message << Communication::ProcType::PROC_IS_WHITELISTED_URL
+    message << Communication::ProcType::PROC_GET_WHITELISTING_FITER
       << 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);
+    EXPECT_EQ(Communication::ProcType::PROC_GET_WHITELISTING_FITER, 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);
@@ skipping 65 matching lines @@
 {
   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());
@@ skipping 38 matching lines @@
   {
     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.

+TEST(InputOutputBuffersTest, EmptyStrings)
 {
   SendReceiveStrings(std::vector<std::string>());
 }
 
-TEST(InputOutputBuffersTests, InputOutputBufferEmptyStringsToWideStrings)
+TEST(InputOutputBuffersTest, EmptyStringsToWideStrings)
 {
   EXPECT_EQ(0u, SendReceiveStringsToWStrings(std::vector<std::string>()).size());
 }
 
-TEST(InputOutputBuffersTests, InputOutputBufferStringsWithOneValue)
+TEST(InputOutputBuffersTest, StringsWithOneValue)
 {
   std::vector<std::string> src;
   src.emplace_back("string1");
   SendReceiveStrings(src);
 }
 
-TEST(InputOutputBuffersTests, InputOutputBufferStringsWithOneValueToWStrings)
+TEST(InputOutputBuffersTest, StringsWithOneValueToWStrings)
 {
   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)
+TEST(InputOutputBuffersTest, WithMultivalueStrings)
 {
   std::vector<std::string> src;
   src.emplace_back("string1");
   src.emplace_back("str2");
   src.emplace_back("value");
   SendReceiveStrings(src);
 }
 
-TEST(InputOutputBuffersTests, InputOutputBufferWithMultivalueStringsToWStrings)
+TEST(InputOutputBuffersTest, WithMultivalueStringsToWStrings)
 {
   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]);
 }