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.
  *
  * 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, PipeReadWrite)

[Eric, 2015/02/17 18:15:13]

As long as we're improving the unit tests, can we separate the pipe tests from
the buffer tests? As far as I'm concerned, they can stay in the same file; I
also won't object if you want two files.

[sergei, 2015/02/18 14:13:06]

Actually I would not like to improve tests in that changeset, it's more like "I
had to", so let's keep them here and do it later.

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

OK. I am most concerned with eliminating the explicit conversions. I won't block
that by insisting on perfect unit tests.

+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, SendReceivePipe)
-{
-  std::unique_ptr<Communication::Pipe> server;
-  auto serverThread = std::thread([&server]

[Eric, 2015/02/17 18:15:13]

Is it necessary to use a separate thread to initialize the server? You are,
after all, using the server object in the current thread below.

-  {
-    server.reset(new Communication::Pipe(pipeName, Communication::Pipe::MODE_CREATE));
+TEST(CommunicationTest, PipeSendReceive)
+{
+  std::unique_ptr<Communication::Pipe> client;
+  auto clientThread = std::thread([&client]
+  {
+    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::sleep_for(i * std::chrono::milliseconds(20));
+      }
+    }
   });
-  std::this_thread::sleep_for(std::chrono::milliseconds(100));

[Eric, 2015/02/17 18:15:13]

I myself have had trouble with the Communication unit tests failing
intermittently, so I appreciate the improvement.

I believe there is, however, a better was of doing it than a fixed delay. The
system call used for MODE_CONNECT, CreateFile(), will block if the pipe exists
but isn't ready (at least as I understand it). Instead of hoping that it's ready
within 100 ms, initialize the client in a separate thread and wait for it to
complete. Its initialization thread will terminate as soon as the client is
ready to use.

This will be particularly useful to do if you split up the unit tests as I've
suggested below, since you won't be multiplying the delay.

-  Communication::Pipe client(pipeName, Communication::Pipe::MODE_CONNECT);
+  Communication::Pipe server(pipeName, Communication::Pipe::MODE_CREATE);
+  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);
+    client->WriteMessage(message);
   }
   // receive on the server and send back
   {
     Communication::OutputBuffer response;
-    Communication::InputBuffer message = server->ReadMessage();
+    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;
 

[Eric, 2015/02/17 18:15:13]

Each of these individual tests on disparate variable types deserves a separate
unit test. Should one ever fail, you'd want to know if only one fails or if
they're all failing. The present test structure doesn't allow that distinction.

Separately, it's a useful unit test to send multiple variables in sequence.

[sergei, 2015/02/18 14:13:06]

The tests of each type are below, it's not a testing of buffers, it's a testing
of Pipe. Instead of testing on some completely abstract data I had to use our
buffers because the Pipe functions work only with our buffers.
Actually if something fails we will get the information about it (line number,
expected value, actual value). But I agree that it's quite comprehensive test
rather than "unit test".
Although I find it already better than it was, should I extract it into a
separate test?
Actually, I would like to have it as it's now because it covers what we actually
do while the client is communicating with the server.

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

I don't want to belabor the point here, but they're different enough to be worth
testing separately. The buffer classes prepare data to move across the pipe, and
the pipe class actually moves it. These two can be tested separately.
Yes, that's basically the point I was making. No need to change it right now,
although now would be appropriate if you were inclined to.
As to the immediate issue, I'd be glad to see separate tests, but I won't even
request it.

There are two kinds of tests here: testing each element individually and testing
the sequences used for communication. Both are valuable. At present we only have
the second kind of test.

Changing this can wait. The real value of unit tests occurs when you have
extensive unit tests available before you find yourself rewriting a class (or
writing it the first time, for that matter). Since we have higher priorities
right now than rewriting, there's no pressing need to enhance the test suite.

     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);
+    server.WriteMessage(response);
   }
 
   // receive on the client
   {
-    Communication::InputBuffer message = client.ReadMessage();
+    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);
   }
-  serverThread.join();
 }
 
 namespace
 {
   template<typename TestedType, typename AnotherType>
   void SendReceiveTypes(TestedType value, AnotherType anotherTypeValue)

[Eric, 2015/02/17 18:15:13]

You're combining two kinds of test here.
1) Ensure that a value sent matches the value received.
2) Ensure that (a) a value sent is not interpretable as another type, (b) when
it attempted in error, it doesn't advance the internal stream pointer.

I'd separate out the first kind of test from the second, as always to partition
types of failure between different tests.

[sergei, 2015/02/18 14:13:06]

Before the everything was in one test, would you mind if we make such
improvements in another commit?

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

That's fine.

   {
     Communication::OutputBuffer message;
     message << anotherTypeValue << value;
     Communication::InputBuffer response(message.Get());

[Eric, 2015/02/17 18:15:13]

I don't particularly care for Get() in the current code base. Whatever changes
might be needed to improve it aren't relevant to the present review, nor is
there a need to discuss it here (although, in very brief, there's lots of
extraneous copying). Get() shouldn't really be public, since it's only used to
communicate with the pipe; but it is, and there's no reason to change it here.

On the other hand, we can isolate Get() so that it still feels like an internal
function, in the eventuality that the buffer and pipe classes are ever reworked.
What's being implemented here is the moral equivalent of an "in-memory pipe",
and furthermore, it's only good for a single use (which is all the code ever
uses it for).

So how about a utility function called 'ConvertOutputToInput' or 'SimulatePipe'
or something similar? We have both copy constructor and copy assignment already
defined, so the function is a one liner. And I won't quibble about efficiency
for a few unit tests.

I'd also like to see this for documentary reasons. It took rather too long for
me to understand what Get() was doing in the first place. A good name for the
utility function should be adequate documentation.

[sergei, 2015/02/18 14:13:06]

I find it as overkill here.

On 2015/02/17 18:15:13, Eric wrote:
I also feel it but actually in the current version public Get looks good.
Despite the buffers are used only with Pipe, they are not named as
PipeInputBuffer and PipeOutputBuffer, currently they are like some helpers to
serialize our data into array of bytes. BTW, regarding it, actually Get should
return std::vector, not std::string.

     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, SendWString_ReceiveWString)

[Eric, 2015/02/17 18:15:13]

Probably better to spell out "Wide" here and below. Might also want to be
explicit about "Narrow" below.

Although I personally think that the underscore here is fine, it's not in our
code style.

How about "TypeStringSendWideReceiveWide"? Wordy but consistent and descriptive.

[sergei, 2015/02/18 14:13:06]

done.

+TEST(CommunicationTest, SendReceiveEmptyString)
+{
+  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)
 {
   Communication::OutputBuffer message;
   message << std::string("test");
   Communication::InputBuffer response(message.Get());
@@ skipping 38 matching lines @@
 }
 
 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)

[Eric, 2015/02/17 18:15:13]

We should test narrow-wide conversions here as well.

[sergei, 2015/02/18 14:13:06]

added

   {
     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());
+    ASSERT_EQ(src.size(), dstSize);
     for (auto i = 0u; i < dstSize; ++i)
     {
-      EXPECT_EQ(dst[i], src[i]);
+      EXPECT_EQ(src[i], dst[i]);
     }
   }
-}
-
-TEST(InputOutputBuffersTests, EmptyStrings)
+
+  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(InputOutputBuffersTest, EmptyStrings)
 {
   SendReceiveStrings(std::vector<std::string>());
 }
 
-TEST(InputOutputBuffersTests, StringsWithOneValue)
+TEST(InputOutputBuffersTest, EmptyStringsToWideStrings)
+{
+  EXPECT_EQ(0u, SendReceiveStringsToWStrings(std::vector<std::string>()).size());
+}
+
+TEST(InputOutputBuffersTest, StringsWithOneValue)
 {
   std::vector<std::string> src;
   src.emplace_back("string1");
   SendReceiveStrings(src);
 }
 
-TEST(InputOutputBuffersTests, MultivalueStrings)
+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(InputOutputBuffersTest, WithMultivalueStrings)
 {
   std::vector<std::string> src;
   src.emplace_back("string1");
   src.emplace_back("str2");
   src.emplace_back("value");
   SendReceiveStrings(src);
+}
+
+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]);
 }