Made sure NativeMessagingHostTest.All can handle out of order responses.

remoting/host/setup/native_messaging_host_unittest.cc

Index: remoting/host/setup/native_messaging_host_unittest.cc
diff --git a/remoting/host/setup/native_messaging_host_unittest.cc b/remoting/host/setup/native_messaging_host_unittest.cc
index 209604a4f934c68709c7e5dfed11f443324c4062..a95271cd4f3f8ccd5d361e30b49c652ba17b65d1 100644
--- a/remoting/host/setup/native_messaging_host_unittest.cc
+++ b/remoting/host/setup/native_messaging_host_unittest.cc
@@ -27,7 +27,7 @@ using remoting::protocol::SynchronousPairingRegistry;
 
 namespace {
 
-void VerifyHelloResponse(const base::DictionaryValue* response) {
+void VerifyHelloResponse(scoped_ptr<base::DictionaryValue> response) {

[Lambros, 2013/09/06 03:03:13]

Changing to scoped_ptr means that these functions will delete the object passed
in - is that necessary?

[alexeypa (please no reviews), 2013/09/06 16:14:57]

Yes. It expresses the intent of these functions better. They are used to consume
the received messages.

   ASSERT_TRUE(response);
   std::string value;
   EXPECT_TRUE(response->GetString("type", &value));
@@ -36,7 +36,7 @@ void VerifyHelloResponse(const base::DictionaryValue* response) {
   EXPECT_EQ(STRINGIZE(VERSION), value);
 }
 
-void VerifyGetHostNameResponse(const base::DictionaryValue* response) {
+void VerifyGetHostNameResponse(scoped_ptr<base::DictionaryValue> response) {
   ASSERT_TRUE(response);
   std::string value;
   EXPECT_TRUE(response->GetString("type", &value));
@@ -45,7 +45,7 @@ void VerifyGetHostNameResponse(const base::DictionaryValue* response) {
   EXPECT_EQ(net::GetHostName(), value);
 }
 
-void VerifyGetPinHashResponse(const base::DictionaryValue* response) {
+void VerifyGetPinHashResponse(scoped_ptr<base::DictionaryValue> response) {
   ASSERT_TRUE(response);
   std::string value;
   EXPECT_TRUE(response->GetString("type", &value));
@@ -54,7 +54,7 @@ void VerifyGetPinHashResponse(const base::DictionaryValue* response) {
   EXPECT_EQ(remoting::MakeHostPinHash("my_host", "1234"), value);
 }
 
-void VerifyGenerateKeyPairResponse(const base::DictionaryValue* response) {
+void VerifyGenerateKeyPairResponse(scoped_ptr<base::DictionaryValue> response) {
   ASSERT_TRUE(response);
   std::string value;
   EXPECT_TRUE(response->GetString("type", &value));
@@ -63,7 +63,7 @@ void VerifyGenerateKeyPairResponse(const base::DictionaryValue* response) {
   EXPECT_TRUE(response->GetString("publicKey", &value));
 }
 
-void VerifyGetDaemonConfigResponse(const base::DictionaryValue* response) {
+void VerifyGetDaemonConfigResponse(scoped_ptr<base::DictionaryValue> response) {
   ASSERT_TRUE(response);
   std::string value;
   EXPECT_TRUE(response->GetString("type", &value));
@@ -73,7 +73,8 @@ void VerifyGetDaemonConfigResponse(const base::DictionaryValue* response) {
   EXPECT_TRUE(base::DictionaryValue().Equals(config));
 }
 
-void VerifyGetUsageStatsConsentResponse(const base::DictionaryValue* response) {
+void VerifyGetUsageStatsConsentResponse(
+    scoped_ptr<base::DictionaryValue> response) {
   ASSERT_TRUE(response);
   std::string value;
   EXPECT_TRUE(response->GetString("type", &value));
@@ -87,7 +88,7 @@ void VerifyGetUsageStatsConsentResponse(const base::DictionaryValue* response) {
   EXPECT_TRUE(set_by_policy);
 }
 
-void VerifyStopDaemonResponse(const base::DictionaryValue* response) {
+void VerifyStopDaemonResponse(scoped_ptr<base::DictionaryValue> response) {
   ASSERT_TRUE(response);
   std::string value;
   EXPECT_TRUE(response->GetString("type", &value));
@@ -96,7 +97,7 @@ void VerifyStopDaemonResponse(const base::DictionaryValue* response) {
   EXPECT_EQ("OK", value);
 }
 
-void VerifyGetDaemonStateResponse(const base::DictionaryValue* response) {
+void VerifyGetDaemonStateResponse(scoped_ptr<base::DictionaryValue> response) {
   ASSERT_TRUE(response);
   std::string value;
   EXPECT_TRUE(response->GetString("type", &value));
@@ -105,7 +106,8 @@ void VerifyGetDaemonStateResponse(const base::DictionaryValue* response) {
   EXPECT_EQ("STARTED", value);
 }
 
-void VerifyUpdateDaemonConfigResponse(const base::DictionaryValue* response) {
+void VerifyUpdateDaemonConfigResponse(
+    scoped_ptr<base::DictionaryValue> response) {
   ASSERT_TRUE(response);
   std::string value;
   EXPECT_TRUE(response->GetString("type", &value));
@@ -114,7 +116,7 @@ void VerifyUpdateDaemonConfigResponse(const base::DictionaryValue* response) {
   EXPECT_EQ("OK", value);
 }
 
-void VerifyStartDaemonResponse(const base::DictionaryValue* response) {
+void VerifyStartDaemonResponse(scoped_ptr<base::DictionaryValue> response) {
   ASSERT_TRUE(response);
   std::string value;
   EXPECT_TRUE(response->GetString("type", &value));
@@ -145,13 +147,7 @@ class MockDaemonController : public DaemonController {
   virtual void GetUsageStatsConsent(
       const GetUsageStatsConsentCallback& callback) OVERRIDE;
 
-  // Returns a record of functions called, so that unittests can verify these
-  // were called in the proper sequence.
-  std::string call_log() { return call_log_; }
-
  private:
-  std::string call_log_;
-
   DISALLOW_COPY_AND_ASSIGN(MockDaemonController);
 };
 
@@ -160,12 +156,10 @@ MockDaemonController::MockDaemonController() {}
 MockDaemonController::~MockDaemonController() {}
 
 DaemonController::State MockDaemonController::GetState() {
-  call_log_ += "GetState:";
   return DaemonController::STATE_STARTED;
 }
 
 void MockDaemonController::GetConfig(const GetConfigCallback& callback) {
-  call_log_ += "GetConfig:";
   scoped_ptr<base::DictionaryValue> config(new base::DictionaryValue());
   callback.Run(config.Pass());
 }
@@ -173,7 +167,6 @@ void MockDaemonController::GetConfig(const GetConfigCallback& callback) {
 void MockDaemonController::SetConfigAndStart(
     scoped_ptr<base::DictionaryValue> config, bool consent,
     const CompletionCallback& callback) {
-  call_log_ += "SetConfigAndStart:";
 
   // Verify parameters passed in.
   if (consent && config && config->HasKey("start")) {
@@ -186,7 +179,6 @@ void MockDaemonController::SetConfigAndStart(
 void MockDaemonController::UpdateConfig(
     scoped_ptr<base::DictionaryValue> config,
     const CompletionCallback& callback) {
-  call_log_ += "UpdateConfig:";
   if (config && config->HasKey("update")) {
     callback.Run(DaemonController::RESULT_OK);
   } else {
@@ -195,7 +187,6 @@ void MockDaemonController::UpdateConfig(
 }
 
 void MockDaemonController::Stop(const CompletionCallback& callback) {
-  call_log_ += "Stop:";
   callback.Run(DaemonController::RESULT_OK);
 }
 
@@ -208,7 +199,6 @@ void MockDaemonController::GetVersion(const GetVersionCallback& callback) {
 
 void MockDaemonController::GetUsageStatsConsent(
     const GetUsageStatsConsentCallback& callback) {
-  call_log_ += "GetUsageStatsConsent:";
   callback.Run(true, true, true);
 }
 
@@ -235,7 +225,6 @@ class NativeMessagingHostTest : public testing::Test {
  protected:
   // Reference to the MockDaemonController, which is owned by |host_|.
   MockDaemonController* daemon_controller_;
-  std::string call_log_;
 
  private:
   // Each test creates two unidirectional pipes: "input" and "output".
@@ -296,9 +285,7 @@ void NativeMessagingHostTest::Run() {
 
   // Destroy |host_| so that it closes its end of the output pipe, so that
   // TestBadRequest() will see EOF and won't block waiting for more data.
-  // Since |host_| owns |daemon_controller_|, capture its call log first.
-  call_log_ = daemon_controller_->call_log();
-  host_.reset(NULL);
+  host_.reset();
 }
 
 scoped_ptr<base::DictionaryValue>
@@ -343,6 +330,8 @@ void NativeMessagingHostTest::TestBadRequest(const base::Value& message) {
   base::DictionaryValue good_message;
   good_message.SetString("type", "hello");
 
+  // This test currently relies on synchronous processing of hello messages and
+  // message parameters verification.
   WriteMessageToInputPipe(good_message);
   WriteMessageToInputPipe(message);
   WriteMessageToInputPipe(good_message);
@@ -352,7 +341,7 @@ void NativeMessagingHostTest::TestBadRequest(const base::Value& message) {
   // Read from output pipe, and verify responses.
   scoped_ptr<base::DictionaryValue> response =
       ReadMessageFromOutputPipe();
-  VerifyHelloResponse(response.get());
+  VerifyHelloResponse(response.Pass());
 
   response = ReadMessageFromOutputPipe();
   EXPECT_FALSE(response);
@@ -360,31 +349,40 @@ void NativeMessagingHostTest::TestBadRequest(const base::Value& message) {
 
 // Test all valid request-types.
 TEST_F(NativeMessagingHostTest, All) {
+  int next_id = 0;
   base::DictionaryValue message;
+  message.SetInteger("id", next_id++);

[Lambros, 2013/09/06 03:03:13]

Optional: Maybe have WriteMessageWithIdToInputPipe() which automatically adds
the current id (protected member variable) and increments it? Or just fold it
into WriteMessageToInputPipe() ? I'd be happy with removing the test that sends
a message without an id, it doesn't seem important.

[alexeypa (please no reviews), 2013/09/06 16:14:57]

I thought about it. The missing id test seems to be useful as long as NMH has
code to handle this case.

   message.SetString("type", "hello");
   WriteMessageToInputPipe(message);
 
+  message.SetInteger("id", next_id++);
   message.SetString("type", "getHostName");
   WriteMessageToInputPipe(message);
 
+  message.SetInteger("id", next_id++);
   message.SetString("type", "getPinHash");
   message.SetString("hostId", "my_host");
   message.SetString("pin", "1234");
   WriteMessageToInputPipe(message);
 
   message.Clear();
+  message.SetInteger("id", next_id++);
   message.SetString("type", "generateKeyPair");
   WriteMessageToInputPipe(message);
 
+  message.SetInteger("id", next_id++);
   message.SetString("type", "getDaemonConfig");
   WriteMessageToInputPipe(message);
 
+  message.SetInteger("id", next_id++);
   message.SetString("type", "getUsageStatsConsent");
   WriteMessageToInputPipe(message);
 
+  message.SetInteger("id", next_id++);
   message.SetString("type", "stopDaemon");
   WriteMessageToInputPipe(message);
 
+  message.SetInteger("id", next_id++);
   message.SetString("type", "getDaemonState");
   WriteMessageToInputPipe(message);
 
@@ -392,6 +390,7 @@ TEST_F(NativeMessagingHostTest, All) {
   base::DictionaryValue config;
   config.SetBoolean("update", true);
   message.Set("config", config.DeepCopy());
+  message.SetInteger("id", next_id++);
   message.SetString("type", "updateDaemonConfig");
   WriteMessageToInputPipe(message);
 
@@ -399,47 +398,41 @@ TEST_F(NativeMessagingHostTest, All) {
   config.SetBoolean("start", true);
   message.Set("config", config.DeepCopy());
   message.SetBoolean("consent", true);
+  message.SetInteger("id", next_id++);
   message.SetString("type", "startDaemon");
   WriteMessageToInputPipe(message);
 
   Run();
 
-  // Read from output pipe, and verify responses.
-  scoped_ptr<base::DictionaryValue> response = ReadMessageFromOutputPipe();
-  VerifyHelloResponse(response.get());
-
-  response = ReadMessageFromOutputPipe();
-  VerifyGetHostNameResponse(response.get());
-
-  response = ReadMessageFromOutputPipe();
-  VerifyGetPinHashResponse(response.get());
-
-  response = ReadMessageFromOutputPipe();
-  VerifyGenerateKeyPairResponse(response.get());
-
-  response = ReadMessageFromOutputPipe();
-  VerifyGetDaemonConfigResponse(response.get());
-
-  response = ReadMessageFromOutputPipe();
-  VerifyGetUsageStatsConsentResponse(response.get());
-
-  response = ReadMessageFromOutputPipe();
-  VerifyStopDaemonResponse(response.get());
-
-  response = ReadMessageFromOutputPipe();
-  VerifyGetDaemonStateResponse(response.get());
-
-  response = ReadMessageFromOutputPipe();
-  VerifyUpdateDaemonConfigResponse(response.get());
-
-  response = ReadMessageFromOutputPipe();
-  VerifyStartDaemonResponse(response.get());
-
-  // Verify that DaemonController methods were called in the correct sequence.
-  // This detects cases where NativeMessagingHost might call a wrong method
-  // that takes the same parameters and writes out the same response.
-  EXPECT_EQ("GetConfig:GetUsageStatsConsent:Stop:GetState:UpdateConfig:"
-            "SetConfigAndStart:", call_log_);
+  void (*verify_routines[])(scoped_ptr<base::DictionaryValue>) = {
+    &VerifyHelloResponse,
+    &VerifyGetHostNameResponse,
+    &VerifyGetPinHashResponse,
+    &VerifyGenerateKeyPairResponse,
+    &VerifyGetDaemonConfigResponse,
+    &VerifyGetUsageStatsConsentResponse,
+    &VerifyStopDaemonResponse,
+    &VerifyGetDaemonStateResponse,
+    &VerifyUpdateDaemonConfigResponse,
+    &VerifyStartDaemonResponse,
+  };
+  ASSERT_EQ(arraysize(verify_routines), static_cast<size_t>(next_id));
+
+  // Read all responses from output pipe, and verify them.
+  for (int i = 0; i < next_id; ++i) {
+    scoped_ptr<base::DictionaryValue> response = ReadMessageFromOutputPipe();
+
+    // Make sure that id is available and is in the range.
+    int id;
+    ASSERT_TRUE(response->GetInteger("id", &id));
+    ASSERT_TRUE(0 <= id && id < next_id);
+
+    // Call the verification routine corresponding to the message id.

[Lambros, 2013/09/06 03:03:13]

ASSERT_TRUE(verify_routines[id] != NULL);
before trying to call the routine.

[alexeypa (please no reviews), 2013/09/06 16:14:57]

Calling a routine using a NULL pointer will crash the process in equally
efficient way. There is no point to duplicate the check.

[Lambros, 2013/09/06 18:55:42]

We shouldn't be crashing the test binary, as this means all subsequent tests
will fail to run. The ASSERT_TRUE macro takes care of only terminating the
current test.

[alexeypa (please no reviews), 2013/09/06 22:19:26]

Done.

+    verify_routines[id](response.Pass());
+
+    // Clear the pointer so that the routine cannot be called the second time.
+    verify_routines[id] = NULL;
+  }
 }
 
 // Verify that response ID matches request ID.