Use ChromeSigninClient::PreSignOut() instead of SigninManager::SignOut() in ChromeSigninClientTests

This CL is step forward to move the test away from SigninManager, PO2TS and AccountTrackerService
completely. The CL:

1) removes the knowledge of the tests about internals of SigninManager (eg that
SigninManager::OnSignoutDecisionReached is called during the SignOut routine).

2) Removes MockSigninManager class completely, after tests switched away to use
ChromeSigninClient::PreSignOut() instead of SigninManager::SignOut().

3) Calls ChromeSigninClient::AfterCredentialsCopied instead of SigninManager::CopyCredentialsFrom
keeping the same behavior.

BUG=890787

Change-Id: Ib9dbc5feb073ec7bbda452a680e71de27a361e79
Reviewed-on: https://chromium-review.googlesource.com/c/1394663
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619936}

chrome/browser/signin/chrome_signin_client_unittest.cc

@@ -22,7 +22,6 @@
 #include "components/signin/core/browser/account_consistency_method.h"
 #include "content/public/browser/network_service_instance.h"
 #include "content/public/test/test_browser_thread_bundle.h"
-#include "google_apis/gaia/fake_oauth2_token_service.h"
 #include "services/network/test/test_network_connection_tracker.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -136,24 +135,12 @@ class MockChromeSigninClient : public ChromeSigninClient {
 
   MOCK_METHOD1(ShowUserManager, void(const base::FilePath&));
   MOCK_METHOD1(LockForceSigninProfile, void(const base::FilePath&));
-};
 
-class MockSigninManager : public SigninManager {
- public:
-  explicit MockSigninManager(SigninClient* client)
-      : SigninManager(client,
-                      nullptr,
-                      &fake_service_,
-                      nullptr,
-                      signin::AccountConsistencyMethod::kDisabled) {}
-
-  MOCK_METHOD4(OnSignoutDecisionReached,
+  MOCK_METHOD4(SignOutCallback,
                void(signin_metrics::ProfileSignout,
                     signin_metrics::SignoutDelete,
-                    RemoveAccountsOption remove_option,
+                    SigninManager::RemoveAccountsOption remove_option,
                     SigninClient::SignoutDecision signout_decision));
-
-  AccountTrackerService fake_service_;
 };
 
 class ChromeSigninClientSignoutTest : public BrowserWithTestWindowTest {
@@ -163,7 +150,6 @@ class ChromeSigninClientSignoutTest : public BrowserWithTestWindowTest {
 
     signin_util::SetForceSigninForTesting(true);
     CreateClient(browser()->profile());
-    manager_ = std::make_unique<MockSigninManager>(client_.get());
   }
 
   void TearDown() override {
@@ -173,12 +159,19 @@ class ChromeSigninClientSignoutTest : public BrowserWithTestWindowTest {
 
   void CreateClient(Profile* profile) {
     client_ = std::make_unique<MockChromeSigninClient>(profile);
-    token_service_ = std::make_unique<FakeOAuth2TokenService>();
+  }
+
+  void PreSignOut(signin_metrics::ProfileSignout source_metric,
+                  signin_metrics::SignoutDelete delete_metric) {
+    client_->PreSignOut(
+        base::BindOnce(&MockChromeSigninClient::SignOutCallback,
+                       base::Unretained(client_.get()), source_metric,
+                       delete_metric,
+                       SigninManager::RemoveAccountsOption::kRemoveAllAccounts),
+        source_metric);
   }
 
   std::unique_ptr<MockChromeSigninClient> client_;
-  std::unique_ptr<FakeOAuth2TokenService> token_service_;
-  std::unique_ptr<MockSigninManager> manager_;
 };
 
 TEST_F(ChromeSigninClientSignoutTest, SignOut) {
@@ -191,14 +184,14 @@ TEST_F(ChromeSigninClientSignoutTest, SignOut) {
       .Times(1);
   EXPECT_CALL(*client_, LockForceSigninProfile(browser()->profile()->GetPath()))
       .Times(1);
-  EXPECT_CALL(*manager_,
-              OnSignoutDecisionReached(
-                  source_metric, delete_metric,
-                  SigninManager::RemoveAccountsOption::kRemoveAllAccounts,
-                  SigninClient::SignoutDecision::ALLOW_SIGNOUT))
+  EXPECT_CALL(
+      *client_,
+      SignOutCallback(source_metric, delete_metric,
+                      SigninManager::RemoveAccountsOption::kRemoveAllAccounts,
+                      SigninClient::SignoutDecision::ALLOW_SIGNOUT))
       .Times(1);
 
-  manager_->SignOut(source_metric, delete_metric);
+  PreSignOut(source_metric, delete_metric);
 }
 
 TEST_F(ChromeSigninClientSignoutTest, SignOutWithoutManager) {
@@ -207,40 +200,41 @@ TEST_F(ChromeSigninClientSignoutTest, SignOutWithoutManager) {
   signin_metrics::SignoutDelete delete_metric =
       signin_metrics::SignoutDelete::IGNORE_METRIC;
 
-  MockSigninManager other_manager(client_.get());
-  other_manager.CopyCredentialsFrom(*manager_.get());
+  // Call the method below instead calling SigninManager::CopyCredentialsFrom,
+  // keeping the same behavior.
+  client_->AfterCredentialsCopied();
 
   EXPECT_CALL(*client_, ShowUserManager(browser()->profile()->GetPath()))
       .Times(0);
   EXPECT_CALL(*client_, LockForceSigninProfile(browser()->profile()->GetPath()))
       .Times(1);
-  EXPECT_CALL(*manager_,
-              OnSignoutDecisionReached(
-                  source_metric, delete_metric,
-                  SigninManager::RemoveAccountsOption::kRemoveAllAccounts,
-                  SigninClient::SignoutDecision::ALLOW_SIGNOUT))
+  EXPECT_CALL(
+      *client_,
+      SignOutCallback(source_metric, delete_metric,
+                      SigninManager::RemoveAccountsOption::kRemoveAllAccounts,
+                      SigninClient::SignoutDecision::ALLOW_SIGNOUT))
       .Times(1);
-  manager_->SignOut(source_metric, delete_metric);
 
-  ::testing::Mock::VerifyAndClearExpectations(manager_.get());
+  PreSignOut(source_metric, delete_metric);
+
+  ::testing::Mock::VerifyAndClearExpectations(client_.get());
 
   EXPECT_CALL(*client_, ShowUserManager(browser()->profile()->GetPath()))
       .Times(1);
   EXPECT_CALL(*client_, LockForceSigninProfile(browser()->profile()->GetPath()))
       .Times(1);
-  EXPECT_CALL(*manager_,
-              OnSignoutDecisionReached(
-                  source_metric, delete_metric,
-                  SigninManager::RemoveAccountsOption::kRemoveAllAccounts,
-                  SigninClient::SignoutDecision::ALLOW_SIGNOUT))
+  EXPECT_CALL(
+      *client_,
+      SignOutCallback(source_metric, delete_metric,
+                      SigninManager::RemoveAccountsOption::kRemoveAllAccounts,
+                      SigninClient::SignoutDecision::ALLOW_SIGNOUT))
       .Times(1);
-  manager_->SignOut(source_metric, delete_metric);
+  PreSignOut(source_metric, delete_metric);
 }
 
 TEST_F(ChromeSigninClientSignoutTest, SignOutWithoutForceSignin) {
   signin_util::SetForceSigninForTesting(false);
   CreateClient(browser()->profile());
-  manager_ = std::make_unique<MockSigninManager>(client_.get());
 
   signin_metrics::ProfileSignout source_metric =
       signin_metrics::ProfileSignout::USER_CLICKED_SIGNOUT_SETTINGS;
@@ -251,13 +245,13 @@ TEST_F(ChromeSigninClientSignoutTest, SignOutWithoutForceSignin) {
       .Times(0);
   EXPECT_CALL(*client_, LockForceSigninProfile(browser()->profile()->GetPath()))
       .Times(0);
-  EXPECT_CALL(*manager_,
-              OnSignoutDecisionReached(
-                  source_metric, delete_metric,
-                  SigninManager::RemoveAccountsOption::kRemoveAllAccounts,
-                  SigninClient::SignoutDecision::ALLOW_SIGNOUT))
+  EXPECT_CALL(
+      *client_,
+      SignOutCallback(source_metric, delete_metric,
+                      SigninManager::RemoveAccountsOption::kRemoveAllAccounts,
+                      SigninClient::SignoutDecision::ALLOW_SIGNOUT))
       .Times(1);
-  manager_->SignOut(source_metric, delete_metric);
+  PreSignOut(source_metric, delete_metric);
 }
 
 class ChromeSigninClientSignoutSourceTest
@@ -306,20 +300,19 @@ TEST_P(ChromeSigninClientSignoutSourceTest, UserSignoutAllowed) {
   std::unique_ptr<TestingProfile> profile = builder.Build();
 
   CreateClient(profile.get());
-  manager_ = std::make_unique<MockSigninManager>(client_.get());
   ASSERT_TRUE(signin_util::IsUserSignoutAllowedForProfile(profile.get()));
 
   // Verify SigninManager gets callback indicating sign-out is always allowed.
   signin_metrics::SignoutDelete delete_metric =
       signin_metrics::SignoutDelete::IGNORE_METRIC;
-  EXPECT_CALL(*manager_,
-              OnSignoutDecisionReached(
-                  signout_source, delete_metric,
-                  SigninManager::RemoveAccountsOption::kRemoveAllAccounts,
-                  SigninClient::SignoutDecision::ALLOW_SIGNOUT))
+  EXPECT_CALL(
+      *client_,
+      SignOutCallback(signout_source, delete_metric,
+                      SigninManager::RemoveAccountsOption::kRemoveAllAccounts,
+                      SigninClient::SignoutDecision::ALLOW_SIGNOUT))
       .Times(1);
 
-  manager_->SignOut(signout_source, delete_metric);
+  PreSignOut(signout_source, delete_metric);
 }
 
 #if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_MACOSX)
@@ -331,7 +324,6 @@ TEST_P(ChromeSigninClientSignoutSourceTest, UserSignoutDisallowed) {
   std::unique_ptr<TestingProfile> profile = builder.Build();
 
   CreateClient(profile.get());
-  manager_ = std::make_unique<MockSigninManager>(client_.get());
 
   ASSERT_TRUE(signin_util::IsUserSignoutAllowedForProfile(profile.get()));
   signin_util::SetUserSignoutAllowedForProfile(profile.get(), false);
@@ -345,14 +337,14 @@ TEST_P(ChromeSigninClientSignoutSourceTest, UserSignoutDisallowed) {
           : SigninClient::SignoutDecision::ALLOW_SIGNOUT;
   signin_metrics::SignoutDelete delete_metric =
       signin_metrics::SignoutDelete::IGNORE_METRIC;
-  EXPECT_CALL(*manager_,
-              OnSignoutDecisionReached(
-                  signout_source, delete_metric,
-                  SigninManager::RemoveAccountsOption::kRemoveAllAccounts,
-                  signout_decision))
+  EXPECT_CALL(
+      *client_,
+      SignOutCallback(signout_source, delete_metric,
+                      SigninManager::RemoveAccountsOption::kRemoveAllAccounts,
+                      signout_decision))
       .Times(1);
 
-  manager_->SignOut(signout_source, delete_metric);
+  PreSignOut(signout_source, delete_metric);
 }
 #endif