Initial RemoteCommandsInvalidator

chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc

Index: chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc
diff --git a/chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc b/chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0c6c414027b0d41ee3c36546d7cc875a44db579c
--- /dev/null
+++ b/chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc
@@ -0,0 +1,311 @@
+// 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>

[bartfab (slow), 2015/05/15 13:28:09]

Nit: Not used.

[binjin, 2015/05/15 14:50:41]

Done.

+
+#include "base/macros.h"

[bartfab (slow), 2015/05/15 13:28:09]

Nit: Already included by header.

[binjin, 2015/05/15 14:50:41]

Done.

+#include "base/memory/ref_counted.h"

[bartfab (slow), 2015/05/15 13:28:08]

Nit: Already included by header.

[binjin, 2015/05/15 14:50:39]

Done.

+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "chrome/browser/invalidation/fake_invalidation_service.h"
+#include "chrome/browser/policy/cloud/remote_commands_invalidator_base.h"

[bartfab (slow), 2015/05/15 13:28:10]

Nit: Make this the first included, followed by a blank line.

[binjin, 2015/05/15 14:50:41]

Done.

+#include "components/invalidation/invalidation.h"
+#include "components/invalidation/invalidation_util.h"
+#include "policy/proto/device_management_backend.pb.h"

[bartfab (slow), 2015/05/15 13:28:09]

Nit: Already included by header.

[binjin, 2015/05/15 14:50:41]

Done.

+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace em = enterprise_management;
+
+using ::testing::InSequence;

[bartfab (slow), 2015/05/15 13:28:10]

Nit: Not actually needed.

[binjin, 2015/05/15 14:50:41]

I think it's used later.

+using ::testing::Mock;
+using ::testing::StrictMock;
+
+namespace policy {
+
+class MockRemoteCommandInvalidator : public RemoteCommandsInvalidatorBase {
+ public:
+  MockRemoteCommandInvalidator(
+      const scoped_refptr<base::SequencedTaskRunner>& task_runner)

[bartfab (slow), 2015/05/15 13:28:09]

Nit: #include "base/sequenced_task_runner.h" (scoped_refptr requires the type to
be defined to compile, even if you never dereference it)

[binjin, 2015/05/15 14:50:40]

Done.

+      : RemoteCommandsInvalidatorBase(task_runner) {}
+  ~MockRemoteCommandInvalidator() override {}

[bartfab (slow), 2015/05/15 13:28:08]

Nit: No need to override the destructor here at all.

[binjin, 2015/05/15 14:50:40]

Done.

+
+  MOCK_METHOD0(OnInitialize, void());
+  MOCK_METHOD0(OnShutdown, void());
+  MOCK_METHOD0(OnStart, void());
+  MOCK_METHOD0(OnStop, void());
+  MOCK_METHOD0(DoRemoteCommandsFetch, void());
+
+  void SetInvalidationObjectID(const invalidation::ObjectId& object_id) {
+    scoped_ptr<em::PolicyData> policy_data(new em::PolicyData());

[bartfab (slow), 2015/05/15 13:28:09]

Why do you not just use a stack-allocated |em::PolicyData| variable?

[binjin, 2015/05/15 14:50:40]

Done.

+    policy_data->set_command_invalidation_source(object_id.source());
+    policy_data->set_command_invalidation_name(object_id.name());
+    ReloadPolicyData(policy_data.get());
+  }
+
+  void ClearInvalidationObjectID() {
+    scoped_ptr<em::PolicyData> policy_data(new em::PolicyData());

[bartfab (slow), 2015/05/15 13:28:10]

As above, why not just allcoate the |em::PolicyData| on the stack?

[binjin, 2015/05/15 14:50:41]

Done.

+    ReloadPolicyData(policy_data.get());
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockRemoteCommandInvalidator);
+};
+
+class RemoteCommandsInvalidatorBaseTest : public testing::Test {
+ public:
+  RemoteCommandsInvalidatorBaseTest()
+      : kTestingObjectId1(123456, "abcdef"),
+        kTestingObjectId2(654321, "defabc"),
+        invalidation_service_(new invalidation::FakeInvalidationService),

[bartfab (slow), 2015/05/15 13:28:10]

Nit: Why does this need to be a scoped_ptr? Why can you not just have a
|invalidation::FakeInvalidationService| member directly?

[binjin, 2015/05/15 14:50:40]

Done.

+        invalidator_(new StrictMock<MockRemoteCommandInvalidator>(

[bartfab (slow), 2015/05/15 13:28:09]

Nit: Why does this need to be a scoped_ptr? Why can you not just have a
|StrictMock<MockRemoteCommandInvalidator>| member directly?

[binjin, 2015/05/15 14:50:40]

Done.

+            loop_.message_loop_proxy())) {}

[bartfab (slow), 2015/05/15 13:28:08]

Nit 1: MessageLoopProxy is deprecated. Use base::ThreadTaskRunnerHandle::Get()
instead.
Nit 2: When you have a multi-line inline method definition, the {} should be
split over two lines.

[binjin, 2015/05/15 14:50:41]

1: I can't use ThreadTaskRunnerHandle. It's my previous approach but it break
tests since MockAckHandler used by FakeInvalidationServices post tasks to
MessageLoopProxy.
2: Done

[bartfab (slow), 2015/05/15 15:37:12]

You are not passing the MessageLoopProxy you grab here to anyone. MockAckHandler
will grab its own MessageLoopProxy if it needs one (and it also could use a
ThreadTaskRunnerHandle instead, but that is another refactor for another day).

+
+  void EnableInvalidationService() {
+    invalidation_service_->SetInvalidatorState(syncer::INVALIDATIONS_ENABLED);

[bartfab (slow), 2015/05/15 13:28:09]

Nit: #include "components/invalidation/invalidator_state.h"

[binjin, 2015/05/15 14:50:41]

Done.

+  }
+
+  void DisableInvalidationService() {
+    invalidation_service_->SetInvalidatorState(
+        syncer::TRANSIENT_INVALIDATION_ERROR);
+  }
+
+  syncer::Invalidation FireInvalidation(
+      const invalidation::ObjectId& object_id) {
+    auto invalidation = syncer::Invalidation::InitUnknownVersion(object_id);

[bartfab (slow), 2015/05/15 13:28:10]

Nit 1: Do not use |auto| here. We only use it in tight loops where it
significantly improves readability, not to obscure types in general.
Nit 2: const.

[binjin, 2015/05/15 14:50:39]

Done.

+    invalidation_service_->EmitInvalidationForTest(invalidation);
+    return invalidation;
+  }
+
+  bool IsInvalidationSent(const syncer::Invalidation& invalidation) {
+    return !invalidation_service_->GetMockAckHandler()->IsUnsent(invalidation);

[bartfab (slow), 2015/05/15 13:28:09]

Nit: #include "components/invalidation/mock_ack_handler.h"

[binjin, 2015/05/15 14:50:41]

Done.

+  }
+
+  bool IsInvalidationAcknowledged(const syncer::Invalidation& invalidation) {
+    return invalidation_service_->GetMockAckHandler()->IsAcknowledged(
+        invalidation);
+  }
+
+  bool IsInvalidatorRegistered() {
+    return !invalidation_service_->invalidator_registrar()

[bartfab (slow), 2015/05/15 13:28:09]

Nit: #include "components/invalidation/invalidator_registrar.h"

[binjin, 2015/05/15 14:50:41]

Done.

+                .GetRegisteredIds(invalidator_.get())
+                .empty();
+  }
+
+  void VerifyExpectations() {
+    Mock::VerifyAndClearExpectations(invalidator_.get());
+  }
+
+ protected:
+  // Initialize and start the invalidator.
+  void InitializeAndStart() {
+    EXPECT_CALL(*invalidator_, OnInitialize()).Times(1);
+    invalidator_->Initialize(invalidation_service_.get());
+    EXPECT_CALL(*invalidator_, OnStart()).Times(1);

[bartfab (slow), 2015/05/15 13:28:09]

GMock does not allow mixing EXPECT_CALL() with invocations. You must always do a
VerifyAndClearExpectations() before setting the next EXPECT_CALL() after a
method invocation.

[binjin, 2015/05/15 14:50:41]

Done.

+    invalidator_->Start();
+
+    VerifyExpectations();
+  }
+
+  // Stop and shutdown the invalidator.
+  void StopAndShutdown() {
+    EXPECT_CALL(*invalidator_, OnStop()).Times(1);
+    EXPECT_CALL(*invalidator_, OnShutdown()).Times(1);
+    invalidator_->Shutdown();
+
+    VerifyExpectations();
+  }
+
+  // Fire an invalidation to verify that invalidaion is not working.

[bartfab (slow), 2015/05/15 13:28:08]

Nit: s/invalidaion/invalidation/

[binjin, 2015/05/15 14:50:40]

Done.

+  void VerifiesInvalidationDisabled(const invalidation::ObjectId& object_id) {

[bartfab (slow), 2015/05/15 13:28:10]

Nit: s/Verifies/Verify/

[binjin, 2015/05/15 14:50:40]

Done.

+    VerifyExpectations();

[bartfab (slow), 2015/05/15 13:28:09]

Why do you need to do this? Does any of the tests call this method without
verifying pending expectations first?

[binjin, 2015/05/15 14:50:41]

Done.

+
+    EXPECT_FALSE(invalidator_->invalidations_enabled());
+
+    syncer::Invalidation inv = FireInvalidation(object_id);

[bartfab (slow), 2015/05/15 13:28:08]

Nit 1: Avoid abbreviaitons like |inv|.
Nit 2: const.

[binjin, 2015/05/15 14:50:41]

Done.

+
+    loop_.RunUntilIdle();

[bartfab (slow), 2015/05/15 13:28:09]

Nit: Here and throughout the file: Never spin the loop directly. Use
RunLoop().RunUntilIdle().

[binjin, 2015/05/15 14:50:40]

Done.

+    EXPECT_FALSE(IsInvalidationSent(inv));
+    VerifyExpectations();

[bartfab (slow), 2015/05/15 13:28:09]

What expectations are you verifying here?

[binjin, 2015/05/15 14:50:40]

Done.

+  }
+
+  // Fire an invalidation to verify that invalidaion works.

[bartfab (slow), 2015/05/15 13:28:09]

Nit: s/invalidaion/invalidation/

[binjin, 2015/05/15 14:50:41]

Done.

+  void VerifiesInvalidationEnabled(const invalidation::ObjectId& object_id) {

[bartfab (slow), 2015/05/15 13:28:09]

Nit: s/Verifies/Verify/

[binjin, 2015/05/15 14:50:40]

Done.

+    VerifyExpectations();

[bartfab (slow), 2015/05/15 13:28:09]

Why do you need to do this? Does any of the tests call this method without
verifying pending expectations first?

[binjin, 2015/05/15 14:50:41]

Done.

+
+    EXPECT_TRUE(invalidator_->invalidations_enabled());
+
+    EXPECT_CALL(*invalidator_, DoRemoteCommandsFetch()).Times(1);
+    syncer::Invalidation inv = FireInvalidation(object_id);

[bartfab (slow), 2015/05/15 13:28:09]

Nit 1: Avoid abbreviaitons like |inv|.
Nit 2: const.

[binjin, 2015/05/15 14:50:41]

Done.

+
+    loop_.RunUntilIdle();
+    EXPECT_TRUE(IsInvalidationSent(inv));
+    EXPECT_TRUE(IsInvalidationAcknowledged(inv));
+    VerifyExpectations();
+  }
+
+  const invalidation::ObjectId kTestingObjectId1;
+  const invalidation::ObjectId kTestingObjectId2;
+
+  base::MessageLoop loop_;
+
+  scoped_ptr<invalidation::FakeInvalidationService> invalidation_service_;
+  scoped_ptr<MockRemoteCommandInvalidator> invalidator_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(RemoteCommandsInvalidatorBaseTest);
+};
+
+// Verifies that only the fired invalidations will be received.
+TEST_F(RemoteCommandsInvalidatorBaseTest, FiredInvalidation) {
+  InSequence dummy;

[bartfab (slow), 2015/05/15 13:28:10]

Nit: There is no need for a sequence in this test.

[binjin, 2015/05/15 14:50:40]

Done.

+
+  InitializeAndStart();
+
+  // Invalidator won't work at this point.
+  EXPECT_FALSE(invalidator_->invalidations_enabled());
+
+  // Load the policy data, it should work now.
+  invalidator_->SetInvalidationObjectID(kTestingObjectId1);
+  EXPECT_TRUE(invalidator_->invalidations_enabled());
+
+  // Disable invalidation service, it should stop working.
+  DisableInvalidationService();

[bartfab (slow), 2015/05/15 13:28:08]

Nit: The disabling and re-enabling is already being tested by
InvalidationServiceEnabledStateChanged(). Just move the
EXPECT_*(invalidator_->invalidations_enabled()) to that test and remove the
disabling and re-enabling here.

[binjin, 2015/05/15 14:50:40]

Done. Note that the invalidations_enabled() is actually already been tested in
InvalidationServiceEnabledStateChanged.

+  EXPECT_FALSE(invalidator_->invalidations_enabled());
+
+  // Re-enable invalidation service.
+  EnableInvalidationService();
+  EXPECT_TRUE(invalidator_->invalidations_enabled());
+
+  // No invalidation will be received if no invalidation is fired.
+  VerifyExpectations();

[bartfab (slow), 2015/05/15 13:28:10]

Nit 1: Should you not spin the loop before verifying this, to make sure there is
no pending task that receives a spurious invalidation?
Nit 2: This VerifyExpectations() is superfluous. The one in line 190 is
sufficient to catch any unexpected callbacks.

[binjin, 2015/05/15 14:50:41]

Done.

+
+  // Fire an invalidation with different object id, no invalidation will be
+  // received.
+  syncer::Invalidation inv1 = FireInvalidation(kTestingObjectId2);

[bartfab (slow), 2015/05/15 13:28:10]

Nit 1: Avoid abbreviaitons like |inv1|.
Nit 2: const.

[binjin, 2015/05/15 14:50:40]

Done.

+
+  loop_.RunUntilIdle();
+  EXPECT_FALSE(IsInvalidationSent(inv1));
+  VerifyExpectations();
+
+  // Fire the invalidation, it should be acknowledged and trigger a remote
+  // commands fetch.
+  EXPECT_CALL(*invalidator_, DoRemoteCommandsFetch()).Times(1);
+  syncer::Invalidation inv2 = FireInvalidation(kTestingObjectId1);
+
+  loop_.RunUntilIdle();
+  EXPECT_TRUE(IsInvalidationSent(inv2));
+  EXPECT_TRUE(IsInvalidationAcknowledged(inv2));
+  VerifyExpectations();
+
+  StopAndShutdown();
+}
+
+// Verifies that no invalidation will be received when invalidator is shutdown.
+TEST_F(RemoteCommandsInvalidatorBaseTest, ShutDown) {
+  EXPECT_FALSE(invalidator_->invalidations_enabled());
+  FireInvalidation(kTestingObjectId1);
+
+  loop_.RunUntilIdle();
+  EXPECT_FALSE(invalidator_->invalidations_enabled());
+}
+
+// Verifies that no invalidation will be received when invalidator is stopped.
+TEST_F(RemoteCommandsInvalidatorBaseTest, Stopped) {
+  EXPECT_CALL(*invalidator_, OnInitialize()).Times(1);
+  invalidator_->Initialize(invalidation_service_.get());
+  VerifyExpectations();
+
+  EXPECT_FALSE(invalidator_->invalidations_enabled());
+  FireInvalidation(kTestingObjectId2);
+
+  loop_.RunUntilIdle();
+  EXPECT_FALSE(invalidator_->invalidations_enabled());
+
+  EXPECT_CALL(*invalidator_, OnShutdown()).Times(1);
+  invalidator_->Shutdown();
+}
+
+// Verifies that stated/stopped state changes works as expected.
+TEST_F(RemoteCommandsInvalidatorBaseTest, StartedStateChange) {
+  InSequence dummy;
+
+  InitializeAndStart();
+
+  // Invalidator requires object id to work.
+  VerifiesInvalidationDisabled(kTestingObjectId1);
+  invalidator_->SetInvalidationObjectID(kTestingObjectId1);
+  VerifiesInvalidationEnabled(kTestingObjectId1);
+
+  // Stop and restart invalidator.
+  EXPECT_CALL(*invalidator_, OnStop()).Times(1);
+  invalidator_->Stop();
+  VerifyExpectations();
+
+  VerifiesInvalidationDisabled(kTestingObjectId1);
+
+  EXPECT_CALL(*invalidator_, OnStart()).Times(1);
+  invalidator_->Start();
+  VerifyExpectations();
+
+  // Invalidator requires object id to work.
+  invalidator_->SetInvalidationObjectID(kTestingObjectId1);
+  VerifiesInvalidationEnabled(kTestingObjectId1);
+
+  StopAndShutdown();
+}
+
+// Verifies that registered state changes works as expected.

[bartfab (slow), 2015/05/15 13:28:08]

Nit: s/works/work/

[binjin, 2015/05/15 14:50:40]

Done.

+TEST_F(RemoteCommandsInvalidatorBaseTest, RegistedStateChange) {
+  InSequence dummy;
+
+  InitializeAndStart();
+
+  invalidator_->SetInvalidationObjectID(kTestingObjectId1);
+  VerifiesInvalidationEnabled(kTestingObjectId1);
+
+  invalidator_->SetInvalidationObjectID(kTestingObjectId2);
+  VerifiesInvalidationEnabled(kTestingObjectId2);

[bartfab (slow), 2015/05/15 13:28:09]

Nit: Maybe double-check that |kTestingObjectId1| is disabled now?

[binjin, 2015/05/15 14:50:41]

Done.

+
+  invalidator_->SetInvalidationObjectID(kTestingObjectId1);
+  VerifiesInvalidationEnabled(kTestingObjectId1);

[bartfab (slow), 2015/05/15 13:28:09]

Nit: Maybe double-check that |kTestingObjectId2| is disabled now?

[binjin, 2015/05/15 14:50:41]

Done.

+
+  invalidator_->ClearInvalidationObjectID();
+  VerifiesInvalidationDisabled(kTestingObjectId1);
+
+  invalidator_->SetInvalidationObjectID(kTestingObjectId2);
+  VerifiesInvalidationEnabled(kTestingObjectId2);

[bartfab (slow), 2015/05/15 13:28:09]

Nit: Maybe double-check that |kTestingObjectId1| is disabled now?

[binjin, 2015/05/15 14:50:40]

Done.

+
+  StopAndShutdown();
+}
+
+// Verifies that invalidation service enabled state change works as expected.
+TEST_F(RemoteCommandsInvalidatorBaseTest,
+       InvalidationServiceEnabledStateChanged) {
+  InSequence dummy;
+
+  InitializeAndStart();
+
+  invalidator_->SetInvalidationObjectID(kTestingObjectId1);
+  VerifiesInvalidationEnabled(kTestingObjectId1);
+
+  DisableInvalidationService();
+  VerifiesInvalidationDisabled(kTestingObjectId2);

[bartfab (slow), 2015/05/15 13:28:09]

Here and below: Why are you sometimes testing the state of |kTestingObjectId2|
instead of |kTestingObjectId1|?

[binjin, 2015/05/15 14:50:40]

They are meant to be "EXPECT_FALSE(invalidator_.invalidations_enabled())",
replaced instead. 

+
+  EnableInvalidationService();
+  VerifiesInvalidationEnabled(kTestingObjectId1);
+
+  EnableInvalidationService();
+  VerifiesInvalidationEnabled(kTestingObjectId1);
+
+  DisableInvalidationService();
+  VerifiesInvalidationDisabled(kTestingObjectId2);
+
+  DisableInvalidationService();
+  VerifiesInvalidationDisabled(kTestingObjectId2);
+
+  StopAndShutdown();
+}
+
+}  // namespace policy