Initial RemoteCommandService

chrome/browser/policy/remote_commands/remote_commands_browsertest.cc

Index: chrome/browser/policy/remote_commands/remote_commands_browsertest.cc
diff --git a/chrome/browser/policy/remote_commands/remote_commands_browsertest.cc b/chrome/browser/policy/remote_commands/remote_commands_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..830302b707b95c3267974b185120601208f61073
--- /dev/null
+++ b/chrome/browser/policy/remote_commands/remote_commands_browsertest.cc
@@ -0,0 +1,466 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/command_line.h"
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/run_loop.h"
+#include "base/thread_task_runner_handle.h"
+#include "base/time/time.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/policy/cloud/test_request_interceptor.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "components/policy/core/browser/browser_policy_connector.h"
+#include "components/policy/core/common/policy_switches.h"
+#include "components/policy/core/common/remote_commands/remote_command_job.h"
+#include "components/policy/core/common/remote_commands/remote_commands_factory.h"
+#include "components/policy/core/common/remote_commands/remote_commands_service.h"
+#include "components/policy/core/common/remote_commands/test_remote_command_job.h"
+#include "components/policy/core/common/remote_commands/testing_remote_commands_server.h"
+#include "content/public/browser/browser_thread.h"
+#include "net/base/net_errors.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "policy/proto/device_management_backend.pb.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h"
+#include "chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.h"
+#else
+#include "chrome/browser/policy/cloud/user_cloud_policy_manager_factory.h"
+#include "components/policy/core/common/cloud/user_cloud_policy_manager.h"
+#endif
+
+using testing::AnyNumber;
+using testing::InvokeWithoutArgs;
+using testing::ReturnNew;
+using testing::_;
+
+namespace policy {
+
+namespace {
+const char kTestToken[] = "secret_token";
+const char kTestClientID[] = "testing_client_id";
+const char kTestPayload[] = "_testing_payload_";
+}  // namespace
+
+namespace em = enterprise_management;
+
+// Mocked RemoteCommand factory to allow us to build test commands.
+class MockTestRemoteCommandFactory : public RemoteCommandsFactory {
+ public:
+  MockTestRemoteCommandFactory() {}
+  ~MockTestRemoteCommandFactory() override {}
+
+  MOCK_METHOD0(BuildTestCommand, TestRemoteCommandJob*());

[bartfab (slow), 2015/02/18 13:21:53]

It looks like BuildTestCommand() always does the same thing. You could use
WillByDefault() and then only set the number of times that you expect the method
to be called in tests.

[binjin, 2015/02/18 15:37:49]

Done.

+
+ private:
+  // RemoteCommandJobsFactory:
+  scoped_ptr<RemoteCommandJob> BuildJobForType(
+      em::RemoteCommand_Type type) override {
+    if (type != em::RemoteCommand_Type_COMMAND_ECHO_TEST) {
+      NOTREACHED();

[bartfab (slow), 2015/02/18 13:21:53]

Since this is test code, use ADD_FAILURE(). NOTREACHED() would produce a crash.
ADD_FAILURE() produces a test failure.

[binjin, 2015/02/18 15:37:49]

Done.

+      return nullptr;
+    }
+    return make_scoped_ptr<RemoteCommandJob>(BuildTestCommand());
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(MockTestRemoteCommandFactory);
+};
+
+// Mocked TestingRemoteCommandsServer for verifying commands results.
+class MockRemoteCommandsServer : public TestingRemoteCommandsServer {

[bartfab (slow), 2015/02/18 13:21:52]

Is this the only implementation of TestingRemoteCommandsServer? If so, why not
combine the two classes into one?

[binjin, 2015/02/18 15:37:49]

I would like to to keep the derived class, since these three methods is just
meant to make code clearer, and they are too specified to move into the base
class. These method are also mocked.

[bartfab (slow), 2015/02/23 13:13:56]

I understand the purpose of MockRemoteCommandsServer and its mock methods. But
what purpose does the bare TestingRemoteCommandsServer serve? Nobody uses it in
that form.

[binjin, 2015/02/24 05:29:49]

Class removed. N/A now

+ public:
+  MockRemoteCommandsServer() {}
+  ~MockRemoteCommandsServer() override {}
+
+  MOCK_CONST_METHOD2(SucceededJobReported,
+                     void(const std::string&, base::Time));
+  MOCK_CONST_METHOD1(FailedJobReported, void(base::Time));
+  MOCK_CONST_METHOD1(IgnoredJobReported, void(base::Time));
+
+ private:
+  // TestingRemoteCommandsServer:
+  void OnJobResultReported(const enterprise_management::RemoteCommandResult&
+                               job_result) const override {
+    base::Time timestamp =

[bartfab (slow), 2015/02/18 13:21:53]

Nit: const.

[binjin, 2015/02/18 15:37:48]

Done.

+        base::TimeDelta::FromMilliseconds(job_result.timestamp()) +
+        base::Time::UnixEpoch();
+    switch (job_result.result()) {
+      case em::RemoteCommandResult_ResultType_RESULT_SUCCESS:
+        SucceededJobReported(job_result.payload(), timestamp);
+        break;
+      case em::RemoteCommandResult_ResultType_RESULT_FAILURE:
+        FailedJobReported(timestamp);
+        break;
+      case em::RemoteCommandResult_ResultType_RESULT_IGNORED:
+        IgnoredJobReported(timestamp);
+        break;
+      default:
+        NOTREACHED();

[bartfab (slow), 2015/02/18 13:21:52]

Nit: Use ADD_FAILURE() instead.

[binjin, 2015/02/18 15:37:49]

Done.

+    }
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(MockRemoteCommandsServer);
+};
+
+// Base class for all browser tests regarding remote commands service.
+class RemoteCommandsBrowserTest : public InProcessBrowserTest {
+ protected:
+  RemoteCommandsBrowserTest() {}
+  ~RemoteCommandsBrowserTest() override {}
+
+  // Register on DMServer with faked token and client id.
+  void Register() {
+    ASSERT_TRUE(policy_manager());
+    ASSERT_TRUE(policy_manager()->core()->client());
+
+    EXPECT_FALSE(policy_manager()->core()->client()->is_registered());
+    policy_manager()->core()->client()->SetupRegistration(kTestToken,
+                                                          kTestClientID);
+    EXPECT_TRUE(policy_manager()->core()->client()->is_registered());
+  }
+
+  // Start the service, must be called after Register(). Note that remote
+  // commands service will immediately start fetching commands.
+  void StartService(scoped_ptr<MockTestRemoteCommandFactory> factory) {
+    ASSERT_FALSE(service_started_);

[bartfab (slow), 2015/02/18 13:21:52]

Nit: s/ASSERT/EXPECT/

[binjin, 2015/02/18 15:37:49]

Done.

+    service_started_ = true;
+
+    mock_factory_ = factory.get();

[bartfab (slow), 2015/02/18 13:21:52]

Nit: Not used.

[binjin, 2015/02/18 15:37:49]

Done.

+
+    policy_manager()->core()->StartRemoteCommandsService(factory.Pass());
+  }
+
+  // Start the service, but will wait until the initial remote commands
+  // fetch completes (with no command fetched).
+  void StartServiceWithoutCommandsFetched(
+      scoped_ptr<MockTestRemoteCommandFactory> factory) {
+    base::RunLoop run_loop;
+    interceptor_->PushJobCallback(

[bartfab (slow), 2015/02/18 13:21:53]

Lines 149-151 are copy & pasted to many places. Could you refactor them into a
helper method? A nice solution would be to make the RunLoop a member, i.e.:

The member is:
scoped_ptr<base::RunLoop> run_loop_;

You add a helper:

void ExpectFetchCommands(size_t expected_command_results, size_t
expected_fetched_commands) {
  run_loop_.reset(new base::RunLoop);
  interceptor_->PushJobCallback(TestRequestInterceptor::FetchRemoteCommandsJob(
      server_.get(),
      expected_command_results,
      expected_fetched_commands));
  interceptor_->AddRequestServicedCallback(run_loop_->QuitClosure());
}

Tests can then do:

ExpectFetchCommands(0u, 0u);
... whatever command should trigger the fetch...
run_loop_->Run();

[binjin, 2015/02/18 15:37:49]

Done. But with more methods used.

+        TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), 0u, 0u));
+    interceptor_->AddRequestServicedCallback(run_loop.QuitClosure());
+    StartService(factory.Pass());
+    run_loop.Run();
+  }
+
+  void SetUpInProcessBrowserTestFixture() override {
+    base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+    command_line->AppendSwitchASCII(switches::kDeviceManagementUrl,
+                                    "http://localhost");
+  }
+
+  void SetUpOnMainThread() override {
+    // Set up interceptor and ignore 'register' and 'policy' requests.
+    interceptor_.reset(new TestRequestInterceptor(
+        "localhost", content::BrowserThread::GetMessageLoopProxyForThread(
+                         content::BrowserThread::IO)));
+    interceptor_->AddIgnoredRequestType("register");
+    interceptor_->AddIgnoredRequestType("policy");
+
+    server_.reset(new MockRemoteCommandsServer());
+
+    BrowserPolicyConnector* const connector =
+        g_browser_process->browser_policy_connector();
+    connector->ScheduleServiceInitialization(0);
+
+    ASSERT_TRUE(policy_manager());
+
+#if !defined(OS_CHROMEOS)
+    policy_manager()->Connect(

[bartfab (slow), 2015/02/18 13:21:52]

It feels like you are using a browser test so that you can obtain a
CloudPolicyClient from somewhere. And since the place where you get it from on
Chrome OS does not exist for other OSes, you have to create one yourself here.

Why not make this all a unit test, where you explicitly create a
CloudPolicyClient and pass it in as a dependency?

[binjin, 2015/02/18 15:37:49]

I'm not sure, but TestRequestInteceptor is used in this CL. It's on IO thread
and the code is at "chrome/". 

Eventually I will add browser tests for each remote commands implementation
based on classes here. So I prefer to keep these browser tests.

[bartfab (slow), 2015/02/23 13:13:56]

Having a browser test for each command is definitely a good idea. And using a
common base class for all these tests is also good. However:

1) We are only implementing device commands for now, so your code, which uses
the current user's command queue, will not be appropriate.

2) Tests that verify the actual commands work correctly belong into a browser
test. But tests that verify the RemoteCommandsService behaves expected belong
into a unit test. That unit test will not need the TestRequestInterceptor,
because you can mock out the entire fetching as well.

[binjin, 2015/02/24 05:29:49]

1) It's always okay to redefine policy_manager() latter or left it being virtual
for actual browser tests on various platform.

2) I managed to migrate all browser tests to unit tests. The test server
previously runs on a different thread in browser tests and now runs on the main
thread in unit tests. Since I inherited a lot of things from browser tests, the
current code looks very ugly.

+        g_browser_process->local_state(),
+        g_browser_process->system_request_context(),
+        UserCloudPolicyManager::CreateCloudPolicyClient(
+            connector->device_management_service(),
+            g_browser_process->system_request_context()).Pass());
+#endif
+  }
+
+  void TearDownOnMainThread() override {
+    EXPECT_EQ(0u, interceptor_->GetPendingSize());
+    server_.reset();

[bartfab (slow), 2015/02/18 13:21:52]

Nit: Is this actually necessary?

[binjin, 2015/02/18 15:37:48]

It can only be destroyed on main thread.

+    interceptor_.reset();

[bartfab (slow), 2015/02/18 13:21:52]

Nit: Is this actually necessary?

[binjin, 2015/02/18 15:37:49]

It depends on |server_|.

+  }
+
+#if defined(OS_CHROMEOS)
+  UserCloudPolicyManagerChromeOS* policy_manager() {
+    return UserCloudPolicyManagerFactoryChromeOS::GetForProfile(
+        browser()->profile());
+  }
+#else
+  UserCloudPolicyManager* policy_manager() {
+    return UserCloudPolicyManagerFactory::GetForBrowserContext(
+        browser()->profile());
+  }
+#endif  // defined(OS_CHROMEOS)
+
+  scoped_ptr<TestRequestInterceptor> interceptor_;

[bartfab (slow), 2015/02/18 13:21:52]

Why can this not just be |TestRequestInterceptor interceptor_;|?

[binjin, 2015/02/18 15:37:49]

Use of TestRequestInterceptor is kind of restricted and I think it can only not
be initialized before main thread is ready.

+  scoped_ptr<MockRemoteCommandsServer> server_;

[bartfab (slow), 2015/02/18 13:21:53]

Why can this not just be |MockRemoteCommandsServer server_;|?

[binjin, 2015/02/18 15:37:49]

constructor of TestingRemoteCommandsServer will try to get TaskRunner for
current thread, so it cannot be initialized before main thread is ready.

+  MockTestRemoteCommandFactory* mock_factory_ = nullptr;

[bartfab (slow), 2015/02/18 13:21:52]

Nit: Not used.

[binjin, 2015/02/18 15:37:49]

Done.

+
+ private:
+  bool service_started_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(RemoteCommandsBrowserTest);
+};
+
+// Tests that no command will be fetched if no commands is issued.
+IN_PROC_BROWSER_TEST_F(RemoteCommandsBrowserTest, NoCommands) {
+  scoped_ptr<MockTestRemoteCommandFactory> factory(
+      new MockTestRemoteCommandFactory());
+  EXPECT_CALL(*factory, BuildTestCommand()).Times(0);
+
+  Register();
+  StartServiceWithoutCommandsFetched(factory.Pass());
+
+  // A follow up fetch requst should also get nothing from server.
+  base::RunLoop run_loop;
+  interceptor_->PushJobCallback(
+      TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), 0u, 0u));
+  interceptor_->AddRequestServicedCallback(run_loop.QuitClosure());
+  EXPECT_TRUE(policy_manager()

[bartfab (slow), 2015/02/18 13:21:52]

Nit: This piece is copy & pasted to other places. Please add a helper method for
it.

[binjin, 2015/02/18 15:37:49]

Done.

+                  ->core()
+                  ->remote_commands_service()
+                  ->FetchRemoteCommands());
+  run_loop.Run();
+}
+
+// Tests that existing commands issued before service started will be fetched.
+IN_PROC_BROWSER_TEST_F(RemoteCommandsBrowserTest, ExistingCommand) {
+  scoped_ptr<MockTestRemoteCommandFactory> factory(
+      new MockTestRemoteCommandFactory());
+  EXPECT_CALL(*factory, BuildTestCommand())

[bartfab (slow), 2015/02/18 13:21:52]

This code is copy & pasted to other places. Please add a helper method for it.

[binjin, 2015/02/18 15:37:49]

Done. Used WillByDefault() though.

+      .Times(1)
+      .WillOnce(ReturnNew<TestRemoteCommandJob>(
+          true, base::TimeDelta::FromSeconds(1)));

[bartfab (slow), 2015/02/18 13:21:53]

Nit: Can you create a constant for |base::TimeDelta::FromSeconds(1)| with a
descriptive name? From looking at the code, it is not clear what this one second
indicates.

[binjin, 2015/02/18 15:37:49]

I'm not sure if it's okay to create a global instance of TimeDelta which have
its own ctor. Added a int constant instead.

[bartfab (slow), 2015/02/23 13:13:56]

The way you did it is correct. A static initializer would not have been allowed.

[binjin, 2015/02/24 05:29:49]

Acknowledged.

+
+  Register();
+
+  // Issue a command before service started.
+  server_->IssueCommand(em::RemoteCommand_Type_COMMAND_ECHO_TEST, kTestPayload,
+                        false);
+
+  {
+    // Start the service, run until the command is fetched.
+    base::RunLoop run_loop;
+    interceptor_->PushJobCallback(
+        TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), 0u, 1u));
+    interceptor_->AddRequestServicedCallback(run_loop.QuitClosure());
+    StartService(factory.Pass());
+    run_loop.Run();
+  }
+
+  {
+    // And run until the command result is reported.
+    base::RunLoop run_loop;
+    interceptor_->PushJobCallback(
+        TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), 1u, 0u));
+    EXPECT_CALL(*server_, SucceededJobReported(kTestPayload, _))
+        .Times(1)
+        .WillOnce(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
+    run_loop.Run();
+  }
+
+  EXPECT_EQ(0u, server_->NumberOfCommandsPendingResult());
+}
+
+// Tests that commands issued after service started will be fetched.
+IN_PROC_BROWSER_TEST_F(RemoteCommandsBrowserTest, NewCommand) {
+  scoped_ptr<MockTestRemoteCommandFactory> factory(
+      new MockTestRemoteCommandFactory());
+  EXPECT_CALL(*factory, BuildTestCommand())
+      .Times(1)
+      .WillOnce(ReturnNew<TestRemoteCommandJob>(
+          true, base::TimeDelta::FromSeconds(1)));
+
+  Register();
+  StartServiceWithoutCommandsFetched(factory.Pass());
+
+  // The first request will fetch one command, and the second will fetch none
+  // but provide result for the previous command instead.
+  interceptor_->PushJobCallback(
+      TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), 0u, 1u));
+  interceptor_->PushJobCallback(
+      TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), 1u, 0u));
+
+  base::RunLoop run_loop;
+  EXPECT_CALL(*server_, SucceededJobReported(kTestPayload, _))
+      .Times(1)
+      .WillOnce(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
+  server_->IssueCommand(em::RemoteCommand_Type_COMMAND_ECHO_TEST, kTestPayload,
+                        false);
+
+  // Manually trigger a command fetch immediately, it's supposed to be
+  // triggered by invalidation service though.
+  EXPECT_TRUE(policy_manager()
+                  ->core()
+                  ->remote_commands_service()
+                  ->FetchRemoteCommands());
+
+  // Run until the result of commands is reported.
+  run_loop.Run();
+
+  EXPECT_EQ(0u, server_->NumberOfCommandsPendingResult());
+}
+
+// Tests that commands issued after service will be fetched even if there is
+// delay between command issued and invalidation service triggerred fetch.

[bartfab (slow), 2015/02/18 13:21:53]

I am not sure this test is necessary. You tested the various orders of calls.
Adding delays should not be necessary.

[binjin, 2015/02/18 15:37:49]

Deleted.

+IN_PROC_BROWSER_TEST_F(RemoteCommandsBrowserTest, NewDelayedCommand) {
+  scoped_ptr<MockTestRemoteCommandFactory> factory(
+      new MockTestRemoteCommandFactory());
+  EXPECT_CALL(*factory, BuildTestCommand())
+      .Times(1)
+      .WillOnce(ReturnNew<TestRemoteCommandJob>(
+          true, base::TimeDelta::FromSeconds(1)));
+
+  Register();
+  StartServiceWithoutCommandsFetched(factory.Pass());
+
+  // The first request will fetch one command, and the second will fetch none
+  // but provide result for the previous command instead.
+  interceptor_->PushJobCallback(
+      TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), 0u, 1u));
+  interceptor_->PushJobCallback(
+      TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), 1u, 0u));
+
+  base::RunLoop run_loop;
+  EXPECT_CALL(*server_, SucceededJobReported(kTestPayload, _))
+      .Times(1)
+      .WillOnce(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));

[bartfab (slow), 2015/02/18 13:21:53]

AFAICT, all of the code above is shared with the previous test. Please move it
to a helper method.

[binjin, 2015/02/18 15:37:49]

Acknowledged.

+
+  // Issue the command and simulate one second delay for invalidation service.
+  server_->IssueCommand(em::RemoteCommand_Type_COMMAND_ECHO_TEST, kTestPayload,
+                        false);
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(

[bartfab (slow), 2015/02/18 13:21:52]

You cannot do this in tests. Tests must not slow down execution with artificial
delay. You *can* PostDelayedTask() with a zero delay though.

[binjin, 2015/02/18 15:37:48]

Acknowledged.

+      FROM_HERE,
+      base::Bind(
+          base::IgnoreResult(&RemoteCommandsService::FetchRemoteCommands),
+          base::Unretained(
+              policy_manager()->core()->remote_commands_service())),
+      base::TimeDelta::FromSeconds(1));
+
+  // Run until the result of command is reported.
+  run_loop.Run();
+
+  EXPECT_EQ(0u, server_->NumberOfCommandsPendingResult());
+}
+
+// Tests that commands issued after service will be fetched even if the
+// network is unstable.

[bartfab (slow), 2015/02/18 13:21:52]

Nice. Good to see you are testing such an (sadly, all too common) error case.

[binjin, 2015/02/18 15:37:49]

Acknowledged.

+IN_PROC_BROWSER_TEST_F(RemoteCommandsBrowserTest, NewCommandWithBadConnection) {
+  scoped_ptr<MockTestRemoteCommandFactory> factory(
+      new MockTestRemoteCommandFactory());
+  EXPECT_CALL(*factory, BuildTestCommand())
+      .Times(1)
+      .WillOnce(ReturnNew<TestRemoteCommandJob>(
+          true, base::TimeDelta::FromSeconds(1)));
+
+  Register();
+  StartServiceWithoutCommandsFetched(factory.Pass());
+
+  // Inserts some bad request resposne due to network here.
+  interceptor_->PushJobCallback(
+      TestRequestInterceptor::ErrorJob(net::ERR_NETWORK_CHANGED));
+  interceptor_->PushJobCallback(
+      TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), 0u, 1u));
+  interceptor_->PushJobCallback(
+      TestRequestInterceptor::ErrorJob(net::ERR_NETWORK_CHANGED));
+  interceptor_->PushJobCallback(
+      TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), 1u, 0u));
+
+  base::RunLoop run_loop;
+  EXPECT_CALL(*server_, SucceededJobReported(kTestPayload, _))
+      .Times(1)
+      .WillOnce(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
+  server_->IssueCommand(em::RemoteCommand_Type_COMMAND_ECHO_TEST, kTestPayload,
+                        false);
+
+  EXPECT_TRUE(policy_manager()
+                  ->core()
+                  ->remote_commands_service()
+                  ->FetchRemoteCommands());
+
+  // Run until the result of command is reported.
+  run_loop.Run();
+
+  EXPECT_EQ(0u, server_->NumberOfCommandsPendingResult());
+}
+
+// Tests that commands issued after service started will be fetched, even if
+// the command is issued when a fetch request is ongoing.
+IN_PROC_BROWSER_TEST_F(RemoteCommandsBrowserTest, NewCommandFollwingFetch) {
+  scoped_ptr<MockTestRemoteCommandFactory> factory(
+      new MockTestRemoteCommandFactory());
+  EXPECT_CALL(*factory, BuildTestCommand())
+      .Times(1)
+      .WillOnce(ReturnNew<TestRemoteCommandJob>(
+          true, base::TimeDelta::FromSeconds(1)));
+
+  Register();
+  StartServiceWithoutCommandsFetched(factory.Pass());
+
+  // Add a command which will be issued after first fetch.
+  server_->IssueCommand(em::RemoteCommand_Type_COMMAND_ECHO_TEST, kTestPayload,
+                        true);
+
+  {
+    base::RunLoop run_loop;
+    interceptor_->PushJobCallback(
+        TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), 0u, 0u));
+    interceptor_->AddRequestServicedCallback(run_loop.QuitClosure());
+
+    // Attempts to fetch commands.
+    EXPECT_TRUE(policy_manager()
+                    ->core()
+                    ->remote_commands_service()
+                    ->FetchRemoteCommands());
+
+    // There should be not issued command at this point.
+    EXPECT_EQ(0u, server_->NumberOfCommandsPendingResult());
+
+    // The command fetch should be in progress.
+    EXPECT_TRUE(policy_manager()
+                    ->core()
+                    ->remote_commands_service()
+                    ->IsCommandFetchInProgressForTesting());
+
+    // And second a following up fetch request should be ignored.

[bartfab (slow), 2015/02/18 13:21:52]

It is not ignored but enqueued.

[binjin, 2015/02/18 15:37:49]

Done.

+    EXPECT_FALSE(policy_manager()
+                     ->core()
+                     ->remote_commands_service()
+                     ->FetchRemoteCommands());
+
+    // Run until first fetch request is completed.
+    run_loop.Run();
+  }
+
+  // The delayed command should be issued now.

[bartfab (slow), 2015/02/18 13:21:52]

In reality, the order would be different: The second command is issued, an
invalidation is sent and the second fetch request is made all *before* the first
fetch request finishes. I think the wording can be reworked a bit to make this
clearer in the comments.

[binjin, 2015/02/18 15:37:49]

Done.

+  EXPECT_EQ(1u, server_->NumberOfCommandsPendingResult());
+
+  interceptor_->PushJobCallback(
+      TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), 0u, 1u));
+  interceptor_->PushJobCallback(
+      TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), 1u, 0u));
+
+  // No further fetch request is made, but the new issued command should be
+  // fetched and executed.
+  base::RunLoop run_loop;
+  EXPECT_CALL(*server_, SucceededJobReported(kTestPayload, _))
+      .Times(1)
+      .WillOnce(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
+
+  run_loop.Run();
+
+  EXPECT_EQ(0u, server_->NumberOfCommandsPendingResult());
+}
+
+}  // namespace policy