[Sync] Rework Invalidator-related unit tests

chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc

 // Copyright (c) 2012 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 "chrome/browser/sync/glue/chrome_sync_notification_bridge.h"
 
 #include <cstddef>
 
+#include "base/bind.h"
 #include "base/compiler_specific.h"
+#include "base/location.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/memory/weak_ptr.h"
 #include "base/message_loop.h"
+#include "base/message_loop_proxy.h"
+#include "base/run_loop.h"
 #include "base/sequenced_task_runner.h"
-#include "base/synchronization/waitable_event.h"
-#include "base/test/test_timeouts.h"
 #include "base/threading/thread.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/test/base/profile_mock.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/test/test_browser_thread.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/base/model_type_state_map.h"
-#include "sync/notifier/invalidation_handler.h"
+#include "sync/notifier/fake_invalidation_handler.h"
 #include "sync/notifier/object_id_state_map_test_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace browser_sync {
 namespace {
 
-using ::testing::Mock;
 using ::testing::NiceMock;
-using ::testing::StrictMock;
-using content::BrowserThread;
 
-// Receives a ChromeSyncNotificationBridge to register to, and an expected
-// ModelTypeStateMap. ReceivedProperNotification() will return true only
-// if the observer has received a notification with the proper source and
-// state.
-// Note: Because this object lives on the sync thread, we use a fake
-// (vs a mock) so we don't have to worry about possible thread safety
-// issues within GTest/GMock.
-class FakeInvalidationHandler : public syncer::InvalidationHandler {
- public:
-  FakeInvalidationHandler(
-      const scoped_refptr<base::SequencedTaskRunner>& sync_task_runner,
-      ChromeSyncNotificationBridge* bridge,
-      const syncer::ObjectIdStateMap& expected_states,
-      syncer::IncomingNotificationSource expected_source)
-      : sync_task_runner_(sync_task_runner),
-        bridge_(bridge),
-        received_improper_notification_(false),
-        notification_count_(0),
-        expected_states_(expected_states),
-        expected_source_(expected_source) {
-    DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
-    bridge_->RegisterHandler(this);
-    const syncer::ObjectIdSet& ids =
-        syncer::ObjectIdStateMapToSet(expected_states);
-    bridge_->UpdateRegisteredIds(this, ids);
-  }
+// Since all the interesting stuff happens on the sync thread, we use
+// a fake (vs a mock) so we don't have to worry about possible thread
+// safety issues within GTest/GMock.

[Nicolas Zea, 2012/08/29 23:41:05]

I don't think this comment makes sense here anymore. PErhaps in
fake_invalidation_handler.h instead?

[akalin, 2012/08/30 22:20:38]

Hmm actually I think the general idea belongs here -- rewrote the comment.

 
-  virtual ~FakeInvalidationHandler() {
-    DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
-    bridge_->UnregisterHandler(this);
-  }
-
-  // InvalidationHandler implementation.
-  virtual void OnIncomingNotification(
-      const syncer::ObjectIdStateMap& id_state_map,
-      syncer::IncomingNotificationSource source) OVERRIDE {
-    DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
-    notification_count_++;
-    if (source != expected_source_) {
-      LOG(ERROR) << "Received notification with wrong source";
-      received_improper_notification_ = true;
-    }
-    if (!::testing::Matches(Eq(expected_states_))(id_state_map)) {
-      LOG(ERROR) << "Received wrong state";
-      received_improper_notification_ = true;
-    }
-  }
-  virtual void OnNotificationsEnabled() OVERRIDE {
-    NOTREACHED();
-  }
-  virtual void OnNotificationsDisabled(
-      syncer::NotificationsDisabledReason reason) OVERRIDE {
-    NOTREACHED();
-  }
-
-  bool ReceivedProperNotification() const {
-    DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
-    return (notification_count_ == 1) && !received_improper_notification_;
-  }
-
- private:
-  const scoped_refptr<base::SequencedTaskRunner> sync_task_runner_;
-  ChromeSyncNotificationBridge* const bridge_;
-  bool received_improper_notification_;
-  size_t notification_count_;
-  const syncer::ObjectIdStateMap expected_states_;
-  const syncer::IncomingNotificationSource expected_source_;
-};
+// Needed by BlockForSyncThread().
+void DoNothing() {}
 
 class ChromeSyncNotificationBridgeTest : public testing::Test {
  public:
   ChromeSyncNotificationBridgeTest()
-      : ui_thread_(BrowserThread::UI),
+      : ui_thread_(content::BrowserThread::UI, &ui_loop_),
         sync_thread_("Sync thread"),
         sync_handler_(NULL),
-        sync_handler_notification_failure_(false),
-        done_(true, false) {}
+        sync_handler_notification_success_(false) {}
 
   virtual ~ChromeSyncNotificationBridgeTest() {}
 
  protected:
   virtual void SetUp() OVERRIDE {
     ASSERT_TRUE(sync_thread_.Start());
     bridge_.reset(
         new ChromeSyncNotificationBridge(
             &mock_profile_, sync_thread_.message_loop_proxy()));
   }
 
   virtual void TearDown() OVERRIDE {
     bridge_->StopForShutdown();
     sync_thread_.Stop();
     // Must be reset only after the sync thread is stopped.
     bridge_.reset();
     EXPECT_EQ(NULL, sync_handler_);
-    if (sync_handler_notification_failure_)
-      ADD_FAILURE() << "Sync Observer did not receive proper notification.";
+    if (!sync_handler_notification_success_)
+      ADD_FAILURE() << "Sync handler did not receive proper notification.";
   }
 
-  void VerifyAndDestroyObserver() {
+  void VerifyAndDestroyObserver(
+      const syncer::ModelTypeStateMap& expected_states,
+      syncer::IncomingNotificationSource expected_source) {
     ASSERT_TRUE(sync_thread_.message_loop_proxy()->PostTask(
         FROM_HERE,
         base::Bind(&ChromeSyncNotificationBridgeTest::
                        VerifyAndDestroyObserverOnSyncThread,
-                   base::Unretained(this))));
+                   base::Unretained(this),
+                   expected_states,
+                   expected_source)));
     BlockForSyncThread();
   }
 
-  void CreateObserverWithExpectations(
-      const syncer::ModelTypeStateMap& expected_states,
-      syncer::IncomingNotificationSource expected_source) {
-    const syncer::ObjectIdStateMap& expected_id_state_map =
-        syncer::ModelTypeStateMapToObjectIdStateMap(expected_states);
+  void CreateObserver() {
     ASSERT_TRUE(sync_thread_.message_loop_proxy()->PostTask(
         FROM_HERE,
         base::Bind(
             &ChromeSyncNotificationBridgeTest::CreateObserverOnSyncThread,
-            base::Unretained(this),
-            expected_id_state_map,
-            expected_source)));
+            base::Unretained(this))));
     BlockForSyncThread();
   }
 
-  void UpdateBridgeEnabledTypes(syncer::ModelTypeSet enabled_types) {
+  void UpdateEnabledTypes(syncer::ModelTypeSet enabled_types) {
     ASSERT_TRUE(sync_thread_.message_loop_proxy()->PostTask(
         FROM_HERE,
         base::Bind(
             &ChromeSyncNotificationBridgeTest::
-                UpdateBridgeEnabledTypesOnSyncThread,
+                UpdateEnabledTypesOnSyncThread,
             base::Unretained(this),
             enabled_types)));
     BlockForSyncThread();
   }
 
   void TriggerRefreshNotification(
       int type,
       const syncer::ModelTypeStateMap& state_map) {
     content::NotificationService::current()->Notify(
         type,
         content::Source<Profile>(&mock_profile_),
         content::Details<const syncer::ModelTypeStateMap>(&state_map));
+    BlockForSyncThread();
   }
 
  private:
-  void VerifyAndDestroyObserverOnSyncThread() {
+  void VerifyAndDestroyObserverOnSyncThread(
+      const syncer::ModelTypeStateMap& expected_states,
+      syncer::IncomingNotificationSource expected_source) {
     DCHECK(sync_thread_.message_loop_proxy()->RunsTasksOnCurrentThread());
-    if (!sync_handler_) {
-      sync_handler_notification_failure_ = true;
+    if (sync_handler_) {
+      sync_handler_notification_success_ =
+          (sync_handler_->GetNotificationCount() == 1) &&
+          ObjectIdStateMapEquals(
+              sync_handler_->GetLastNotificationIdStateMap(),
+              syncer::ModelTypeStateMapToObjectIdStateMap(expected_states)) &&
+          (sync_handler_->GetLastNotificationSource() == expected_source);
+      bridge_->UnregisterHandler(sync_handler_);
     } else {
-      sync_handler_notification_failure_ =
-          !sync_handler_->ReceivedProperNotification();
-      delete sync_handler_;
-      sync_handler_ = NULL;
+      sync_handler_notification_success_ = false;
     }
+    delete sync_handler_;

[Nicolas Zea, 2012/08/29 23:41:05]

make sync_handler_ a scoped_ptr?

[akalin, 2012/08/30 22:20:38]

Done.

+    sync_handler_ = NULL;
   }
 
-  void CreateObserverOnSyncThread(
-      const syncer::ObjectIdStateMap& expected_states,
-      syncer::IncomingNotificationSource expected_source) {
+  void CreateObserverOnSyncThread() {
     DCHECK(sync_thread_.message_loop_proxy()->RunsTasksOnCurrentThread());
-    sync_handler_ = new FakeInvalidationHandler(
-        sync_thread_.message_loop_proxy(),
-        bridge_.get(),
-        expected_states,
-        expected_source);
+    sync_handler_ = new syncer::FakeInvalidationHandler();
+    bridge_->RegisterHandler(sync_handler_);
   }
 
-  void UpdateBridgeEnabledTypesOnSyncThread(
+  void UpdateEnabledTypesOnSyncThread(
       syncer::ModelTypeSet enabled_types) {
     DCHECK(sync_thread_.message_loop_proxy()->RunsTasksOnCurrentThread());
-    bridge_->UpdateEnabledTypes(enabled_types);
-  }
-
-  void SignalOnSyncThread() {
-    DCHECK(sync_thread_.message_loop_proxy()->RunsTasksOnCurrentThread());
-    done_.Signal();
+    bridge_->UpdateRegisteredIds(
+        sync_handler_, ModelTypeSetToObjectIdSet(enabled_types));
   }
 
   void BlockForSyncThread() {
-    done_.Reset();
-    ASSERT_TRUE(sync_thread_.message_loop_proxy()->PostTask(
+    // Post a task to the sync thread's message loop and block until
+    // it runs.
+    base::RunLoop run_loop;
+    ASSERT_TRUE(sync_thread_.message_loop_proxy()->PostTaskAndReply(
         FROM_HERE,
-        base::Bind(&ChromeSyncNotificationBridgeTest::SignalOnSyncThread,
-                   base::Unretained(this))));
-    done_.TimedWait(TestTimeouts::action_timeout());
-    if (!done_.IsSignaled())
-      ADD_FAILURE() << "Timed out waiting for sync thread.";
+        base::Bind(&DoNothing),
+        run_loop.QuitClosure()));

[Nicolas Zea, 2012/08/29 23:41:05]

don't you need to do run_loop.run()?

[akalin, 2012/08/30 22:20:38]

Done.

   }
 
+  MessageLoop ui_loop_;
   content::TestBrowserThread ui_thread_;
   base::Thread sync_thread_;
   NiceMock<ProfileMock> mock_profile_;
   // Created/used/destroyed on sync thread.
-  FakeInvalidationHandler* sync_handler_;
-  bool sync_handler_notification_failure_;
+  syncer::FakeInvalidationHandler* sync_handler_;
+  bool sync_handler_notification_success_;
   scoped_ptr<ChromeSyncNotificationBridge> bridge_;
-  base::WaitableEvent done_;
 };
 
 // Adds an observer on the sync thread, triggers a local refresh
 // notification, and ensures the bridge posts a LOCAL_NOTIFICATION
 // with the proper state to it.
 TEST_F(ChromeSyncNotificationBridgeTest, LocalNotification) {
   syncer::ModelTypeStateMap state_map;
   state_map.insert(
       std::make_pair(syncer::SESSIONS, syncer::InvalidationState()));
-  CreateObserverWithExpectations(state_map, syncer::LOCAL_NOTIFICATION);
+  CreateObserver();
+  UpdateEnabledTypes(syncer::ModelTypeSet(syncer::SESSIONS));
   TriggerRefreshNotification(chrome::NOTIFICATION_SYNC_REFRESH_LOCAL,
                              state_map);
-  VerifyAndDestroyObserver();
+  VerifyAndDestroyObserver(state_map, syncer::LOCAL_NOTIFICATION);
 }
 
 // Adds an observer on the sync thread, triggers a remote refresh
 // notification, and ensures the bridge posts a REMOTE_NOTIFICATION
 // with the proper state to it.
 TEST_F(ChromeSyncNotificationBridgeTest, RemoteNotification) {
   syncer::ModelTypeStateMap state_map;
   state_map.insert(
       std::make_pair(syncer::SESSIONS, syncer::InvalidationState()));
-  CreateObserverWithExpectations(state_map, syncer::REMOTE_NOTIFICATION);
+  CreateObserver();
+  UpdateEnabledTypes(syncer::ModelTypeSet(syncer::SESSIONS));
   TriggerRefreshNotification(chrome::NOTIFICATION_SYNC_REFRESH_REMOTE,
                              state_map);
-  VerifyAndDestroyObserver();
+  VerifyAndDestroyObserver(state_map, syncer::REMOTE_NOTIFICATION);
 }
 
 // Adds an observer on the sync thread, triggers a local refresh
 // notification with empty state map and ensures the bridge posts a
 // LOCAL_NOTIFICATION with the proper state to it.
 TEST_F(ChromeSyncNotificationBridgeTest, LocalNotificationEmptyPayloadMap) {
   const syncer::ModelTypeSet enabled_types(
       syncer::BOOKMARKS, syncer::PASSWORDS);
   const syncer::ModelTypeStateMap enabled_types_state_map =
       syncer::ModelTypeSetToStateMap(enabled_types, std::string());
-  CreateObserverWithExpectations(
-      enabled_types_state_map, syncer::LOCAL_NOTIFICATION);
-  UpdateBridgeEnabledTypes(enabled_types);
+  CreateObserver();
+  UpdateEnabledTypes(enabled_types);
   TriggerRefreshNotification(chrome::NOTIFICATION_SYNC_REFRESH_LOCAL,
                              syncer::ModelTypeStateMap());
-  VerifyAndDestroyObserver();
+  VerifyAndDestroyObserver(
+      enabled_types_state_map, syncer::LOCAL_NOTIFICATION);
 }
 
 // Adds an observer on the sync thread, triggers a remote refresh
 // notification with empty state map and ensures the bridge posts a
 // REMOTE_NOTIFICATION with the proper state to it.
 TEST_F(ChromeSyncNotificationBridgeTest, RemoteNotificationEmptyPayloadMap) {
   const syncer::ModelTypeSet enabled_types(
       syncer::BOOKMARKS, syncer::TYPED_URLS);
   const syncer::ModelTypeStateMap enabled_types_state_map =
       syncer::ModelTypeSetToStateMap(enabled_types, std::string());
-  CreateObserverWithExpectations(
-      enabled_types_state_map, syncer::REMOTE_NOTIFICATION);
-  UpdateBridgeEnabledTypes(enabled_types);
+  CreateObserver();
+  UpdateEnabledTypes(enabled_types);
   TriggerRefreshNotification(chrome::NOTIFICATION_SYNC_REFRESH_REMOTE,
                              syncer::ModelTypeStateMap());
-  VerifyAndDestroyObserver();
+  VerifyAndDestroyObserver(
+      enabled_types_state_map, syncer::REMOTE_NOTIFICATION);
 }
 
 }  // namespace
 }  // namespace browser_sync