Implements unittests for hats notification controller

chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc

+// Copyright 2016 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 "base/strings/string_split.h"
+#include "chrome/browser/chromeos/first_run/goodies_displayer.h"

[stevenjb, 2016/07/22 16:14:54]

needed?

[malaykeshav, 2016/07/22 20:03:09]

Removed

+#include "chrome/browser/chromeos/hats/hats_dialog.h"
+#include "chrome/browser/chromeos/hats/hats_notification_controller.h"
+#include "chrome/browser/notifications/message_center_notification_manager.h"
+#include "chrome/browser/notifications/notification.h"
+#include "chrome/browser/notifications/notification_ui_manager.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/browser_with_test_window_test.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "chrome/test/base/testing_profile.h"
+#include "chrome/test/base/testing_profile_manager.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/portal_detector/network_portal_detector.h"
+#include "components/prefs/pref_service.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/test_utils.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/message_center/fake_message_center_tray_delegate.h"
+#include "ui/message_center/message_center.h"

[stevenjb, 2016/07/22 16:14:54]

This is a lot of includes, are they all needed?

[malaykeshav, 2016/07/22 20:03:09]

Done

+
+using testing::_;
+using testing::AtLeast;
+using testing::Return;
+using testing::SaveArg;
+using testing::StrictMock;
+
+namespace chromeos {
+
+namespace {
+
+class MockNetworkPortalDetector : public NetworkPortalDetector {
+ public:
+  MockNetworkPortalDetector() {}
+  ~MockNetworkPortalDetector() override {}
+
+  MOCK_METHOD1(AddObserver,
+               void(chromeos::NetworkPortalDetector::Observer* observer));
+  MOCK_METHOD1(RemoveObserver,
+               void(chromeos::NetworkPortalDetector::Observer* observer));
+  MOCK_METHOD1(AddAndFireObserver,
+               void(chromeos::NetworkPortalDetector::Observer* observer));
+  MOCK_METHOD1(GetCaptivePortalState,
+               chromeos::NetworkPortalDetector::CaptivePortalState(
+                   const std::string& service_path));
+  MOCK_METHOD0(IsEnabled, bool());
+  MOCK_METHOD1(Enable, void(bool start_detection));
+  MOCK_METHOD0(StartDetectionIfIdle, bool());
+  MOCK_METHOD1(SetStrategy,
+               void(chromeos::PortalDetectorStrategy::StrategyId id));
+  MOCK_METHOD0(OnLockScreenRequest, void());
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockNetworkPortalDetector);
+};
+
+}  // namespace
+
+class HatsNotificationControllerTest : public BrowserWithTestWindowTest {
+ public:
+  HatsNotificationControllerTest() {}
+  ~HatsNotificationControllerTest() override {}
+
+  void SetUp() override {
+    BrowserWithTestWindowTest::SetUp();
+
+    profile_manager_.reset(
+        new TestingProfileManager(TestingBrowserProcess::GetGlobal()));
+    ASSERT_TRUE(profile_manager_->SetUp());
+
+    MessageCenterNotificationManager* manager =
+        static_cast<MessageCenterNotificationManager*>(
+            g_browser_process->notification_ui_manager());
+    manager->SetMessageCenterTrayDelegateForTest(
+        new message_center::FakeMessageCenterTrayDelegate(
+            message_center::MessageCenter::Get()));
+
+    network_portal_detector::InitializeForTesting(
+        &mock_network_portal_detector_);
+  }
+
+  void TearDown() override {
+    g_browser_process->notification_ui_manager()->CancelAll();
+    profile_manager_.reset();
+    network_portal_detector::InitializeForTesting(nullptr);
+    BrowserWithTestWindowTest::TearDown();
+  }
+
+  TestingProfile profile_;
+  StrictMock<MockNetworkPortalDetector> mock_network_portal_detector_;
+
+ private:
+  std::unique_ptr<TestingProfileManager> profile_manager_;
+
+  DISALLOW_COPY_AND_ASSIGN(HatsNotificationControllerTest);
+};
+
+TEST_F(HatsNotificationControllerTest, NewDevice_ShouldNotInitialize) {
+  int64_t now_timestamp = base::Time::Now().ToInternalValue();
+  PrefService* pref_service = profile_.GetPrefs();
+  pref_service->SetInt64(prefs::kHatsLastInteractionTimestamp, now_timestamp);
+
+  // The initialization will fail since the function IsNewDevice() will return
+  // true.

[stevenjb, 2016/07/22 16:14:54]

How are we testing that initialization fails?

[malaykeshav, 2016/07/22 20:03:09]

When initialization fails, the timestamp for kHatsLastInteractionTimestamp is
updated to Time::Now().

[stevenjb, 2016/07/22 20:13:53]

That is not at all obvious, we should add a comment. Also "now_timestamp" is
very confusing since "now" is no longer true in the ASSERT. I would suggest
"initial_timestamp".

[malaykeshav, 2016/07/22 22:48:55]

Done

+  scoped_refptr<HatsNotificationController> hats_notification_controller =
+      new HatsNotificationController(&profile_);
+
+  // HatsController::IsNewDevice() is run on a blocking thread.
+  content::RunAllBlockingPoolTasksUntilIdle();
+
+  int64_t new_timestamp =
+      pref_service->GetInt64(prefs::kHatsLastInteractionTimestamp);
+
+  // The flag should be updated to a new timestamp.
+  ASSERT_TRUE(base::Time::FromInternalValue(new_timestamp) >
+              base::Time::FromInternalValue(now_timestamp));
+
+  // Destructor for HatsController removes self from observer list.
+  EXPECT_CALL(mock_network_portal_detector_,
+              RemoveObserver(hats_notification_controller.get()))
+      .Times(1);
+}
+
+TEST_F(HatsNotificationControllerTest, OldDevice_ShouldInitialize) {
+  scoped_refptr<HatsNotificationController> hats_notification_controller =
+      new HatsNotificationController(&profile_);
+  // HatsController::IsNewDevice() is run on a blocking thread.
+  content::RunAllBlockingPoolTasksUntilIdle();

[stevenjb, 2016/07/22 16:14:53]

Does it make sense to group the above into a helper method?

[malaykeshav, 2016/07/22 20:03:09]

Done

+
+  // On initialization, HatsNotificationController adds itself as an observer to
+  // NetworkPortalDetector.
+  EXPECT_CALL(mock_network_portal_detector_,
+              AddAndFireObserver(hats_notification_controller.get()))
+      .Times(1);
+
+  hats_notification_controller->Initialize(false);

[stevenjb, 2016/07/22 16:14:54]

So, are we relying entirely on AddAndFireObserve getting called in the portal
detector to test successful initialization? Is there a way we can do that
explicitly?

[malaykeshav, 2016/07/22 20:03:09]

Its more of a starting with the initialization. It starts a check for internet
connectivity.

Creation of notification and adding it to the notification center is a different
test.

[stevenjb, 2016/07/22 20:13:53]

My point is, it is not at all clear that anything is actually getting tested
here. One specifically should fail here if initialization does not start?
Ideally we should be able to explicitly confirm that initialization started. At
minimum we should comment how we are testing for that. Adding an observer does
not necessarily imply that initialization succeeded.

[malaykeshav, 2016/07/22 22:48:55]

Combined this with the test below to check for whether the notification is
created.

+
+  // Destructor for HatsNotificationController removes self from observer list.
+  EXPECT_CALL(mock_network_portal_detector_,
+              RemoveObserver(hats_notification_controller.get()))
+      .Times(1);
+}
+
+TEST_F(HatsNotificationControllerTest, NoInternet_DoNotShowNotification) {
+  scoped_refptr<HatsNotificationController> hats_notification_controller =
+      new HatsNotificationController(&profile_);
+
+  // Upon destruction HatsNotificationController removes itself as an observer
+  // from NetworkPortalDetector.
+  EXPECT_CALL(mock_network_portal_detector_,
+              RemoveObserver(hats_notification_controller.get()))
+      .Times(1);
+
+  NetworkPortalDetector::CaptivePortalState online_state;
+  hats_notification_controller->OnPortalDetectionCompleted(
+      new NetworkState(std::string()), online_state);
+
+  online_state.status = NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_OFFLINE;
+  hats_notification_controller->OnPortalDetectionCompleted(
+      new NetworkState(std::string()), online_state);
+
+  online_state.status = NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PORTAL;
+  hats_notification_controller->OnPortalDetectionCompleted(
+      new NetworkState(std::string()), online_state);
+
+  online_state.status =
+      NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PROXY_AUTH_REQUIRED;
+  hats_notification_controller->OnPortalDetectionCompleted(
+      new NetworkState(std::string()), online_state);

[stevenjb, 2016/07/22 16:14:54]

It is not clear here where we are actually testing anything.

[malaykeshav, 2016/07/22 20:03:09]

Added the final check condition

+}
+
+TEST_F(HatsNotificationControllerTest, InternetConnected_ShowNotification) {
+  scoped_refptr<HatsNotificationController> hats_notification_controller =
+      new HatsNotificationController(&profile_);
+
+  EXPECT_CALL(mock_network_portal_detector_,
+              RemoveObserver(hats_notification_controller.get()))
+      .Times(AtLeast(1));
+
+  NetworkPortalDetector::CaptivePortalState online_state;
+  online_state.status = NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_ONLINE;
+  hats_notification_controller->OnPortalDetectionCompleted(
+      new NetworkState(std::string()), online_state);
+
+  MessageCenterNotificationManager* manager =
+      static_cast<MessageCenterNotificationManager*>(
+          g_browser_process->notification_ui_manager());

[stevenjb, 2016/07/22 16:14:54]

Do we need the static cast? Also, no need for a local here for a single use.

[malaykeshav, 2016/07/22 20:03:09]

Removed

+  const Notification* notification =
+      manager->FindById(HatsNotificationController::kDelegateId, &profile_);
+  EXPECT_TRUE(notification != nullptr);
+}
+
+TEST_F(HatsNotificationControllerTest, DismissNotification_ShouldUpdatePref) {
+  int64_t now_timestamp = base::Time::Now().ToInternalValue();
+  PrefService* pref_service = profile_.GetPrefs();
+  pref_service->SetInt64(prefs::kHatsLastInteractionTimestamp, now_timestamp);
+
+  scoped_refptr<HatsNotificationController> hats_notification_controller =
+      new HatsNotificationController(&profile_);
+
+  hats_notification_controller->Close(true);
+
+  content::RunAllBlockingPoolTasksUntilIdle();
+
+  int64_t new_timestamp =
+      pref_service->GetInt64(prefs::kHatsLastInteractionTimestamp);
+  // The flag should be updated to a new timestamp.
+  ASSERT_TRUE(base::Time::FromInternalValue(new_timestamp) >
+              base::Time::FromInternalValue(now_timestamp));
+
+  // Destructor for HatsController removes self from observer list.
+  EXPECT_CALL(mock_network_portal_detector_,
+              RemoveObserver(hats_notification_controller.get()))
+      .Times(1);
+}
+
+}  // namespace chromeos