Turn ArcSupportHost from Observer model to Delegate

chrome/browser/chromeos/arc/arc_support_host_unittest.cc

+// Copyright 2017 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/chromeos/arc/arc_support_host.h"
+
+#include "base/memory/ptr_util.h"
+#include "chrome/browser/chromeos/arc/extensions/fake_arc_support.h"
+#include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
+#include "chrome/browser/chromeos/login/users/scoped_user_manager_enabler.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using arc::FakeArcSupport;
+using testing::_;
+using testing::Eq;
+
+namespace {
+
+class MockAuthDelegate : public ArcSupportHost::AuthDelegate {
+ public:
+  MOCK_METHOD1(OnAuthSucceeded, void(const std::string& auth_code));
+  MOCK_METHOD0(OnAuthFailed, void());
+  MOCK_METHOD0(OnAuthRetryClicked, void());
+};
+
+class MockTermsOfServiceDelegate
+    : public ArcSupportHost::TermsOfServiceDelegate {
+ public:
+  MOCK_METHOD3(OnTermsAgreed,
+               void(bool is_metrics_enabled,
+                    bool is_backup_and_restore_enabled,
+                    bool is_location_service_enabled));
+  MOCK_METHOD0(OnTermsRejected, void());
+  MOCK_METHOD0(OnTermsRetryClicked, void());
+};
+
+class MockErrorDelegate : public ArcSupportHost::ErrorDelegate {
+ public:
+  MOCK_METHOD0(OnOptInAborted, void());
+  MOCK_METHOD0(OnRetryClicked, void());
+  MOCK_METHOD0(OnSendFeedbackClicked, void());
+};
+
+class DelegateRAII {

[hidehiko, 2017/05/15 09:05:44]

Can this be merged into ArcSupportHostTest?
Actual RAII can be done in TearDown(), instead.

[victorhsieh0, 2017/05/15 22:31:53]

Done.

+ public:
+  explicit DelegateRAII(ArcSupportHost* support_host)
+      : support_host_(support_host) {}
+
+  MockAuthDelegate* CreateMockAuthDelegate() {
+    auth_delegate_ = base::MakeUnique<MockAuthDelegate>();
+    support_host_->SetAuthDelegate(auth_delegate_.get());
+    return auth_delegate_.get();
+  }
+
+  MockTermsOfServiceDelegate* CreateMockTermsOfServiceDelegate() {
+    tos_delegate_ = base::MakeUnique<MockTermsOfServiceDelegate>();
+    support_host_->SetTermsOfServiceDelegate(tos_delegate_.get());
+    return tos_delegate_.get();
+  }
+
+  MockErrorDelegate* CreateMockErrorDelegate() {
+    error_delegate_ = base::MakeUnique<MockErrorDelegate>();
+    support_host_->SetErrorDelegate(error_delegate_.get());
+    return error_delegate_.get();
+  }
+
+  virtual ~DelegateRAII() {
+    support_host_->SetAuthDelegate(nullptr);
+    support_host_->SetTermsOfServiceDelegate(nullptr);
+    support_host_->SetErrorDelegate(nullptr);
+  }
+
+ private:
+  ArcSupportHost* support_host_;  // not owned
+  std::unique_ptr<MockAuthDelegate> auth_delegate_;
+  std::unique_ptr<MockTermsOfServiceDelegate> tos_delegate_;
+  std::unique_ptr<MockErrorDelegate> error_delegate_;
+};
+
+class ArcSupportHostTest : public testing::Test {
+ public:
+  ArcSupportHostTest() = default;
+  ~ArcSupportHostTest() override = default;
+
+  void SetUp() override {
+    user_manager_enabler_ =
+        base::MakeUnique<chromeos::ScopedUserManagerEnabler>(
+            new chromeos::FakeChromeUserManager());
+
+    profile_ = base::MakeUnique<TestingProfile>();
+    support_host_ = base::MakeUnique<ArcSupportHost>(profile_.get());
+    fake_arc_support_ = base::MakeUnique<FakeArcSupport>(support_host_.get());
+  }
+
+  void TearDown() override {
+    fake_arc_support_.reset();
+    support_host_.reset();
+    profile_.reset();
+    user_manager_enabler_.reset();
+  }
+
+  ArcSupportHost* support_host() { return support_host_.get(); }
+  FakeArcSupport* fake_arc_support() { return fake_arc_support_.get(); }
+
+ private:
+  // Fake as if the current testing thread is UI thread.
+  content::TestBrowserThreadBundle bundle_;
+
+  std::unique_ptr<TestingProfile> profile_;
+  std::unique_ptr<ArcSupportHost> support_host_;
+  std::unique_ptr<FakeArcSupport> fake_arc_support_;
+  std::unique_ptr<chromeos::ScopedUserManagerEnabler> user_manager_enabler_;
+
+  DISALLOW_COPY_AND_ASSIGN(ArcSupportHostTest);
+};
+
+TEST_F(ArcSupportHostTest, AuthSucceeded) {
+  DelegateRAII raii(support_host());
+  MockAuthDelegate* auth_delegate = raii.CreateMockAuthDelegate();
+  support_host()->SetAuthDelegate(auth_delegate);
+
+  support_host()->ShowLso();
+
+  EXPECT_CALL(*auth_delegate, OnAuthSucceeded(Eq("fake_code")));

[hidehiko, 2017/05/15 09:05:44]

IIUC, even if OnAuthSucceeded is not called, the test would pass?
At least StrictMock is needed?

# Or, alternatively how about getting rid of gmock, because it is very difficult
to set up and to write right expectation precisely, although the syntax can look
simple at quick glance, so the maintenance cost sounds very expensive, according
to my experience...

[victorhsieh0, 2017/05/15 22:31:54]

Did you mean uninterested call?  IIUC this call must be called as expected, but
if OnAuthFailed is called, it will still pass.  StrictMock can prevent that if
desired.
Any alternatives? :)  Otherwise, I'd prefer gmock as it seems to be commonly
used already.

[hidehiko, 2017/05/25 12:05:52]

If gmock is continued to be used, StrictMock sounds better.
If we can keep this size, I'm ok with gmock. However, if there is a possibility
to extend the test, hand crafted fake (not mock) implementation sounds better,
IMHO?

[victorhsieh0, 2017/05/26 19:38:41]

Done
I really hope we won't have to grow the responsibility of this class.  To me, I
don't see a problem using gmock at this moment.

+  fake_arc_support()->EmulateAuthCodeResponse("fake_code");
+}
+
+TEST_F(ArcSupportHostTest, AuthFailed) {
+  DelegateRAII raii(support_host());
+  MockAuthDelegate* auth_delegate = raii.CreateMockAuthDelegate();
+  support_host()->SetAuthDelegate(auth_delegate);
+
+  support_host()->ShowLso();
+
+  EXPECT_CALL(*auth_delegate, OnAuthFailed());
+  fake_arc_support()->EmulateAuthFailure();
+}
+
+TEST_F(ArcSupportHostTest, AuthRetryOnError) {
+  // Auth error can only happen after terms accepted.
+  DelegateRAII raii(support_host());
+  MockAuthDelegate* auth_delegate = raii.CreateMockAuthDelegate();
+  support_host()->SetAuthDelegate(auth_delegate);
+  MockErrorDelegate* error_delegate = raii.CreateMockErrorDelegate();
+  support_host()->SetErrorDelegate(error_delegate);
+
+  support_host()->ShowError(ArcSupportHost::Error::NETWORK_UNAVAILABLE_ERROR,
+                            false /* should_show_send_feedback */);
+
+  EXPECT_CALL(*auth_delegate, OnAuthRetryClicked());
+  EXPECT_CALL(*error_delegate, OnOptInAborted()).Times(0);
+  fake_arc_support()->ClickRetryButton();
+}
+
+TEST_F(ArcSupportHostTest, TermsOfServiceAccept) {
+  MockTermsOfServiceDelegate tos_delegate;
+  support_host()->SetTermsOfServiceDelegate(&tos_delegate);
+
+  support_host()->ShowTermsOfService();
+
+  EXPECT_CALL(tos_delegate, OnTermsAgreed(_, _, _));

[hidehiko, 2017/05/15 09:05:44]

Can args be checked, too?

[victorhsieh0, 2017/05/15 22:31:54]

Those parameters (whether the user agree to show metrics, enabling B&R,
location) don't look relevant to the test.  So I'd not add expectation here.

[hidehiko, 2017/05/25 12:05:52]

WDY mean? My understanding is that the preference value is passed here, on
agreement, and it is a part of function's spec?

[victorhsieh0, 2017/05/26 19:38:41]

Oh, I missed the point.  Done.

+  fake_arc_support()->ClickAgreeButton();
+
+  support_host()->SetTermsOfServiceDelegate(nullptr);

[hidehiko, 2017/05/15 09:05:44]

Unnecessary, too?

[victorhsieh0, 2017/05/15 22:31:54]

Done.

+}
+
+TEST_F(ArcSupportHostTest, TermsOfServiceRejectOrCloseWindow) {
+  DelegateRAII raii(support_host());
+  MockTermsOfServiceDelegate* tos_delegate =
+      raii.CreateMockTermsOfServiceDelegate();
+  support_host()->SetTermsOfServiceDelegate(tos_delegate);
+  MockErrorDelegate* error_delegate = raii.CreateMockErrorDelegate();
+  support_host()->SetErrorDelegate(error_delegate);
+
+  support_host()->ShowTermsOfService();
+
+  EXPECT_CALL(*tos_delegate, OnTermsRejected());
+  EXPECT_CALL(*error_delegate, OnOptInAborted()).Times(0);
+  // Rejecting ToS and closing the window when ToS is showing is the same thing.
+  fake_arc_support()->Close();
+}
+
+TEST_F(ArcSupportHostTest, TermsOfServiceRetryOnError) {
+  DelegateRAII raii(support_host());
+  MockTermsOfServiceDelegate* tos_delegate =
+      raii.CreateMockTermsOfServiceDelegate();
+  support_host()->SetTermsOfServiceDelegate(tos_delegate);
+  MockErrorDelegate* error_delegate = raii.CreateMockErrorDelegate();
+  support_host()->SetErrorDelegate(error_delegate);
+
+  support_host()->ShowError(ArcSupportHost::Error::NETWORK_UNAVAILABLE_ERROR,
+                            false /* should_show_send_feedback */);
+
+  EXPECT_CALL(*tos_delegate, OnTermsRetryClicked());
+  EXPECT_CALL(*error_delegate, OnOptInAborted()).Times(0);
+  fake_arc_support()->ClickRetryButton();
+
+  support_host()->SetTermsOfServiceDelegate(nullptr);

[hidehiko, 2017/05/15 09:05:44]

Ditto.

[victorhsieh0, 2017/05/15 22:31:54]

Done.

+}
+
+TEST_F(ArcSupportHostTest, CloseWindowDuringManualAuth) {
+  // No TermsOfServiceDelegate since it is not ongoing.
+  DelegateRAII raii(support_host());
+  MockErrorDelegate* error_delegate = raii.CreateMockErrorDelegate();
+  support_host()->SetErrorDelegate(error_delegate);
+  MockAuthDelegate* auth_delegate = raii.CreateMockAuthDelegate();
+  support_host()->SetAuthDelegate(auth_delegate);
+
+  support_host()->ShowArcLoading();
+
+  EXPECT_CALL(*error_delegate, OnOptInAborted());
+  fake_arc_support()->Close();
+}
+
+TEST_F(ArcSupportHostTest, CloseWindowWithoutTermsOfServiceOrAuthOnging) {
+  // No TermsOfServiceDelegate since it is not ongoing.
+  DelegateRAII raii(support_host());
+  MockErrorDelegate* error_delegate = raii.CreateMockErrorDelegate();
+  support_host()->SetErrorDelegate(error_delegate);
+
+  support_host()->ShowArcLoading();
+
+  EXPECT_CALL(*error_delegate, OnOptInAborted());
+  fake_arc_support()->Close();
+}
+
+TEST_F(ArcSupportHostTest, RetryOnGeneralError) {
+  // No TermsOfServiceDelegate and AuthDelegate since it is not ongoing.
+  DelegateRAII raii(support_host());
+  MockErrorDelegate* error_delegate = raii.CreateMockErrorDelegate();
+  support_host()->SetErrorDelegate(error_delegate);
+
+  support_host()->ShowError(ArcSupportHost::Error::NETWORK_UNAVAILABLE_ERROR,
+                            false /* should_show_send_feedback */);
+
+  EXPECT_CALL(*error_delegate, OnRetryClicked());
+  fake_arc_support()->ClickRetryButton();
+}
+
+TEST_F(ArcSupportHostTest, SendFeedbackOnError) {
+  DelegateRAII raii(support_host());
+  MockErrorDelegate* error_delegate = raii.CreateMockErrorDelegate();
+  support_host()->SetErrorDelegate(error_delegate);
+
+  support_host()->ShowError(ArcSupportHost::Error::NETWORK_UNAVAILABLE_ERROR,
+                            true /* should_show_send_feedback */);
+
+  EXPECT_CALL(*error_delegate, OnSendFeedbackClicked());
+  fake_arc_support()->ClickSendFeedbackButton();
+}
+
+}  // namespace