Rationalize (GCM)AccountTracker unittests for ChromeOS

The GCMAccountTracker and AccountTracker classes listen for the
GoogleSigninSucceeded/GoogleSignedOut callbacks indirectly via
ProfileIdentityProvider. Their unittests cause these callbacks to fire
via calls to FakeIdentityProvider::{LogIn, LogOut}.

These classes are also used on ChromeOS, where the callbacks will never
fire (ChromeOS does not use SigninManager, which fires the callbacks).
The structure of these unittests presents a challenge to refactoring the
production code to depend on SigninManager directly: the
SigninManagerBase test infrastructure, which is all that can be used
on ChromeOS, rightfully has no mechanism for firing the callbacks.

This CL rationalizes these tests for ChromeOS via:
- Commenting out the tests that actually exercise the callbacks firing
- Changing the rest of the tests to simply set the primary account info
  *without* causing the callbacks to fire
- Renaming utility functions as relevant to make the structure clear

Making this change paves the way for a followup change that ports these
classes (and their tests) to depend directly on SigninManager.

Bug: 809923
Change-Id: Ia6c067d845f7bbb7bd78928ac163bda2fdd77a20
Reviewed-on: https://chromium-review.googlesource.com/1042391Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Roger Tawa <rogerta@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558235}

components/gcm_driver/account_tracker_unittest.cc

@@ -9,6 +9,7 @@
 
 #include "base/message_loop/message_loop.h"
 #include "base/strings/stringprintf.h"
+#include "build/build_config.h"
 #include "google_apis/gaia/fake_identity_provider.h"
 #include "google_apis/gaia/fake_oauth2_token_service.h"
 #include "google_apis/gaia/gaia_oauth_client.h"
@@ -263,11 +264,21 @@ class IdentityAccountTrackerTest : public testing::Test {
 
   // Helpers to pass fake events to the tracker.
 
+  void SetActiveAccount(const std::string& account_key) {
+    identity_provider()->SetActiveUsername(account_key);
+  }
+
+// NOTE: On ChromeOS, the login callback is never fired in production (since the
+// underlying GoogleSigninSucceeded callback is never sent). Tests that
+// exercise functionality dependent on that callback firing are not relevant
+// on ChromeOS and should simply not run on that platform.
+#if !defined(OS_CHROMEOS)
   void NotifyLogin(const std::string& account_key) {
     identity_provider()->LogIn(account_key);
   }
 
   void NotifyLogout() { identity_provider()->LogOut(); }
+#endif
 
   void NotifyTokenAvailable(const std::string& username) {
     fake_oauth2_token_service_->AddAccount(username);
@@ -297,7 +308,7 @@ class IdentityAccountTrackerTest : public testing::Test {
 
   void SetupPrimaryLogin() {
     // Initial setup for tests that start with a signed in profile.
-    NotifyLogin(kPrimaryAccountKey);
+    SetActiveAccount(kPrimaryAccountKey);
     NotifyTokenAvailable(kPrimaryAccountKey);
     ReturnOAuthUrlFetchSuccess(kPrimaryAccountKey);
     observer()->Clear();
@@ -353,12 +364,17 @@ void IdentityAccountTrackerTest::ReturnOAuthUrlFetchFailure(
 TEST_F(IdentityAccountTrackerTest, PrimaryNoEventsBeforeLogin) {
   NotifyTokenAvailable(kPrimaryAccountKey);
   NotifyTokenRevoked(kPrimaryAccountKey);
+
+// Logout is not possible on ChromeOS.
+#if !defined(OS_CHROMEOS)
   NotifyLogout();
+#endif
+
   EXPECT_TRUE(observer()->CheckEvents());
 }
 
 TEST_F(IdentityAccountTrackerTest, PrimaryLoginThenTokenAvailable) {
-  NotifyLogin(kPrimaryAccountKey);
+  SetActiveAccount(kPrimaryAccountKey);
   NotifyTokenAvailable(kPrimaryAccountKey);
   EXPECT_TRUE(observer()->CheckEvents());
 
@@ -367,6 +383,8 @@ TEST_F(IdentityAccountTrackerTest, PrimaryLoginThenTokenAvailable) {
       observer()->CheckEvents(TrackingEvent(SIGN_IN, kPrimaryAccountKey)));
 }
 
+// These tests exercise true login/logout, which are not possible on ChromeOS.
+#if !defined(OS_CHROMEOS)
 TEST_F(IdentityAccountTrackerTest, PrimaryTokenAvailableThenLogin) {
   NotifyTokenAvailable(kPrimaryAccountKey);
   EXPECT_TRUE(observer()->CheckEvents());
@@ -386,9 +404,10 @@ TEST_F(IdentityAccountTrackerTest, PrimaryTokenAvailableAndRevokedThenLogin) {
   EXPECT_TRUE(
       observer()->CheckEvents(TrackingEvent(SIGN_IN, kPrimaryAccountKey)));
 }
+#endif
 
 TEST_F(IdentityAccountTrackerTest, PrimaryRevoke) {
-  NotifyLogin(kPrimaryAccountKey);
+  SetActiveAccount(kPrimaryAccountKey);
   NotifyTokenAvailable(kPrimaryAccountKey);
   ReturnOAuthUrlFetchSuccess(kPrimaryAccountKey);
   observer()->Clear();
@@ -399,18 +418,18 @@ TEST_F(IdentityAccountTrackerTest, PrimaryRevoke) {
 }
 
 TEST_F(IdentityAccountTrackerTest, PrimaryRevokeThenLogin) {
-  NotifyLogin(kPrimaryAccountKey);
+  SetActiveAccount(kPrimaryAccountKey);
   NotifyTokenAvailable(kPrimaryAccountKey);
   ReturnOAuthUrlFetchSuccess(kPrimaryAccountKey);
   NotifyTokenRevoked(kPrimaryAccountKey);
   observer()->Clear();
 
-  NotifyLogin(kPrimaryAccountKey);
+  SetActiveAccount(kPrimaryAccountKey);
   EXPECT_TRUE(observer()->CheckEvents());
 }
 
 TEST_F(IdentityAccountTrackerTest, PrimaryRevokeThenTokenAvailable) {
-  NotifyLogin(kPrimaryAccountKey);
+  SetActiveAccount(kPrimaryAccountKey);
   NotifyTokenAvailable(kPrimaryAccountKey);
   ReturnOAuthUrlFetchSuccess(kPrimaryAccountKey);
   NotifyTokenRevoked(kPrimaryAccountKey);
@@ -421,6 +440,8 @@ TEST_F(IdentityAccountTrackerTest, PrimaryRevokeThenTokenAvailable) {
       observer()->CheckEvents(TrackingEvent(SIGN_IN, kPrimaryAccountKey)));
 }
 
+// These tests exercise true login/logout, which are not possible on ChromeOS.
+#if !defined(OS_CHROMEOS)
 TEST_F(IdentityAccountTrackerTest, PrimaryLogoutThenRevoke) {
   NotifyLogin(kPrimaryAccountKey);
   NotifyTokenAvailable(kPrimaryAccountKey);
@@ -442,12 +463,13 @@ TEST_F(IdentityAccountTrackerTest, PrimaryLogoutFetchCancelAvailable) {
   NotifyLogout();
   EXPECT_TRUE(observer()->CheckEvents());
 
-  NotifyLogin(kPrimaryAccountKey);
+  SetActiveAccount(kPrimaryAccountKey);
   NotifyTokenAvailable(kPrimaryAccountKey);
   ReturnOAuthUrlFetchSuccess(kPrimaryAccountKey);
   EXPECT_TRUE(
       observer()->CheckEvents(TrackingEvent(SIGN_IN, kPrimaryAccountKey)));
 }
+#endif
 
 // Non-primary accounts
 
@@ -558,6 +580,8 @@ TEST_F(IdentityAccountTrackerTest, AvailableTokenFetchFailAvailable) {
       observer()->CheckEvents(TrackingEvent(SIGN_IN, "user@example.com")));
 }
 
+// These tests exercise true login/logout, which are not possible on ChromeOS.
+#if !defined(OS_CHROMEOS)
 TEST_F(IdentityAccountTrackerTest, MultiSignOutSignIn) {
   SetupPrimaryLogin();
 
@@ -604,6 +628,7 @@ TEST_F(IdentityAccountTrackerTest, MultiSignOutSignIn) {
   EXPECT_TRUE(
       observer()->CheckEvents(TrackingEvent(SIGN_IN, kPrimaryAccountKey)));
 }
+#endif
 
 // Primary/non-primary interactions
 
@@ -612,10 +637,17 @@ TEST_F(IdentityAccountTrackerTest, MultiNoEventsBeforeLogin) {
   NotifyTokenAvailable("user@example.com");
   NotifyTokenRevoked("user@example.com");
   NotifyTokenRevoked(kPrimaryAccountKey);
+
+// Logout is not possible on ChromeOS.
+#if !defined(OS_CHROMEOS)
   NotifyLogout();
+#endif
+
   EXPECT_TRUE(observer()->CheckEvents());
 }
 
+// This test exercises true login/logout, which are not possible on ChromeOS.
+#if !defined(OS_CHROMEOS)
 TEST_F(IdentityAccountTrackerTest, MultiLogoutRemovesAllAccounts) {
   NotifyLogin(kPrimaryAccountKey);
   NotifyTokenAvailable(kPrimaryAccountKey);
@@ -630,9 +662,10 @@ TEST_F(IdentityAccountTrackerTest, MultiLogoutRemovesAllAccounts) {
       observer()->CheckEvents(TrackingEvent(SIGN_OUT, kPrimaryAccountKey),
                               TrackingEvent(SIGN_OUT, "user@example.com")));
 }
+#endif
 
 TEST_F(IdentityAccountTrackerTest, MultiRevokePrimaryDoesNotRemoveAllAccounts) {
-  NotifyLogin(kPrimaryAccountKey);
+  SetActiveAccount(kPrimaryAccountKey);
   NotifyTokenAvailable(kPrimaryAccountKey);
   ReturnOAuthUrlFetchSuccess(kPrimaryAccountKey);
   NotifyTokenAvailable("user@example.com");

components/gcm_driver/gcm_account_tracker_unittest.cc

@@ -160,13 +160,13 @@ class GCMAccountTrackerTest : public testing::Test {
   GCMAccountTrackerTest();
   ~GCMAccountTrackerTest() override;
 
-  // Helpers to pass fake events to the tracker. Tests should have either a pair
-  // of Start/FinishAccountSignIn or SignInAccount per account. Don't mix.
-  // Call to SignOutAccount is not mandatory.
-  void StartAccountSignIn(const std::string& account_key);
-  void FinishAccountSignIn(const std::string& account_key);
-  void SignInAccount(const std::string& account_key);
-  void SignOutAccount(const std::string& account_key);
+  // Helpers to pass fake info to the tracker. Tests should have either a pair
+  // of Start/FinishAccountAddition or AddAccount per account. Don't mix.
+  // Call to RemoveAccount is not mandatory.
+  void StartAccountAddition(const std::string& account_key);
+  void FinishAccountAddition(const std::string& account_key);
+  void AddAccount(const std::string& account_key);
+  void RemoveAccount(const std::string& account_key);
 
   // Helpers for dealing with OAuth2 access token requests.
   void IssueAccessToken(const std::string& account_key);
@@ -211,12 +211,19 @@ GCMAccountTrackerTest::~GCMAccountTrackerTest() {
     tracker_->Shutdown();
 }
 
-void GCMAccountTrackerTest::StartAccountSignIn(const std::string& account_key) {
-  fake_identity_provider_->LogIn(account_key);
+void GCMAccountTrackerTest::StartAccountAddition(
+    const std::string& account_key) {
+  // NOTE: On ChromeOS, the login callback is never fired in production (since
+  // the underlying GoogleSigninSucceeded callback is never sent). To be
+  // faithful to the production flow, we also avoid sending that callback here
+  // (i.e., we do not call FakeIdentityProvider::LogIn()).
+  // None of these tests exercise functionality depending on that callback being
+  // fired.
+  fake_identity_provider_->SetActiveUsername(account_key);
   fake_token_service_->AddAccount(account_key);
 }
 
-void GCMAccountTrackerTest::FinishAccountSignIn(
+void GCMAccountTrackerTest::FinishAccountAddition(
     const std::string& account_key) {
   IssueAccessToken(account_key);
 
@@ -228,12 +235,12 @@ void GCMAccountTrackerTest::FinishAccountSignIn(
   fetcher->delegate()->OnURLFetchComplete(fetcher);
 }
 
-void GCMAccountTrackerTest::SignInAccount(const std::string& account_key) {
-  StartAccountSignIn(account_key);
-  FinishAccountSignIn(account_key);
+void GCMAccountTrackerTest::AddAccount(const std::string& account_key) {
+  StartAccountAddition(account_key);
+  FinishAccountAddition(account_key);
 }
 
-void GCMAccountTrackerTest::SignOutAccount(const std::string& account_key) {
+void GCMAccountTrackerTest::RemoveAccount(const std::string& account_key) {
   fake_token_service_->RemoveAccount(account_key);
 }
 
@@ -278,7 +285,7 @@ TEST_F(GCMAccountTrackerTest, NoAccounts) {
 // with a specific scope. In this scenario, the underlying account tracker is
 // still working when the CompleteCollectingTokens is called for the first time.
 TEST_F(GCMAccountTrackerTest, SingleAccount) {
-  StartAccountSignIn(kAccountId1);
+  StartAccountAddition(kAccountId1);
 
   tracker()->Start();
   // We don't have any accounts to report, but given the inner account tracker
@@ -286,7 +293,7 @@ TEST_F(GCMAccountTrackerTest, SingleAccount) {
   EXPECT_FALSE(driver()->update_accounts_called());
 
   // This concludes the work of inner account tracker.
-  FinishAccountSignIn(kAccountId1);
+  FinishAccountAddition(kAccountId1);
   IssueAccessToken(kAccountId1);
 
   EXPECT_TRUE(driver()->update_accounts_called());
@@ -297,17 +304,17 @@ TEST_F(GCMAccountTrackerTest, SingleAccount) {
 }
 
 TEST_F(GCMAccountTrackerTest, MultipleAccounts) {
-  StartAccountSignIn(kAccountId1);
-  StartAccountSignIn(kAccountId2);
+  StartAccountAddition(kAccountId1);
+  StartAccountAddition(kAccountId2);
 
   tracker()->Start();
   EXPECT_FALSE(driver()->update_accounts_called());
 
-  FinishAccountSignIn(kAccountId1);
+  FinishAccountAddition(kAccountId1);
   IssueAccessToken(kAccountId1);
   EXPECT_FALSE(driver()->update_accounts_called());
 
-  FinishAccountSignIn(kAccountId2);
+  FinishAccountAddition(kAccountId2);
   IssueAccessToken(kAccountId2);
   EXPECT_TRUE(driver()->update_accounts_called());
 
@@ -321,7 +328,7 @@ TEST_F(GCMAccountTrackerTest, AccountAdded) {
   tracker()->Start();
   driver()->ResetResults();
 
-  SignInAccount(kAccountId1);
+  AddAccount(kAccountId1);
   EXPECT_FALSE(driver()->update_accounts_called());
 
   IssueAccessToken(kAccountId1);
@@ -333,8 +340,8 @@ TEST_F(GCMAccountTrackerTest, AccountAdded) {
 }
 
 TEST_F(GCMAccountTrackerTest, AccountRemoved) {
-  SignInAccount(kAccountId1);
-  SignInAccount(kAccountId2);
+  AddAccount(kAccountId1);
+  AddAccount(kAccountId2);
 
   tracker()->Start();
   IssueAccessToken(kAccountId1);
@@ -344,7 +351,7 @@ TEST_F(GCMAccountTrackerTest, AccountRemoved) {
   driver()->ResetResults();
   EXPECT_FALSE(driver()->update_accounts_called());
 
-  SignOutAccount(kAccountId2);
+  RemoveAccount(kAccountId2);
   EXPECT_TRUE(driver()->update_accounts_called());
 
   std::vector<GCMClient::AccountTokenInfo> expected_accounts;
@@ -353,8 +360,8 @@ TEST_F(GCMAccountTrackerTest, AccountRemoved) {
 }
 
 TEST_F(GCMAccountTrackerTest, GetTokenFailed) {
-  SignInAccount(kAccountId1);
-  SignInAccount(kAccountId2);
+  AddAccount(kAccountId1);
+  AddAccount(kAccountId2);
 
   tracker()->Start();
   IssueAccessToken(kAccountId1);
@@ -372,14 +379,14 @@ TEST_F(GCMAccountTrackerTest, GetTokenFailed) {
 }
 
 TEST_F(GCMAccountTrackerTest, GetTokenFailedAccountRemoved) {
-  SignInAccount(kAccountId1);
-  SignInAccount(kAccountId2);
+  AddAccount(kAccountId1);
+  AddAccount(kAccountId2);
 
   tracker()->Start();
   IssueAccessToken(kAccountId1);
 
   driver()->ResetResults();
-  SignOutAccount(kAccountId2);
+  RemoveAccount(kAccountId2);
   IssueError(kAccountId2);
 
   EXPECT_TRUE(driver()->update_accounts_called());
@@ -390,14 +397,14 @@ TEST_F(GCMAccountTrackerTest, GetTokenFailedAccountRemoved) {
 }
 
 TEST_F(GCMAccountTrackerTest, AccountRemovedWhileRequestsPending) {
-  SignInAccount(kAccountId1);
-  SignInAccount(kAccountId2);
+  AddAccount(kAccountId1);
+  AddAccount(kAccountId2);
 
   tracker()->Start();
   IssueAccessToken(kAccountId1);
   EXPECT_FALSE(driver()->update_accounts_called());
 
-  SignOutAccount(kAccountId2);
+  RemoveAccount(kAccountId2);
   IssueAccessToken(kAccountId2);
   EXPECT_TRUE(driver()->update_accounts_called());
 
@@ -418,9 +425,9 @@ TEST_F(GCMAccountTrackerTest, TrackerObservesConnection) {
 // Makes sure that token fetching happens only after connection is established.
 TEST_F(GCMAccountTrackerTest, PostponeTokenFetchingUntilConnected) {
   driver()->SetConnected(false);
-  StartAccountSignIn(kAccountId1);
+  StartAccountAddition(kAccountId1);
   tracker()->Start();
-  FinishAccountSignIn(kAccountId1);
+  FinishAccountAddition(kAccountId1);
 
   EXPECT_EQ(0UL, tracker()->get_pending_token_request_count());
   driver()->SetConnected(true);
@@ -429,11 +436,11 @@ TEST_F(GCMAccountTrackerTest, PostponeTokenFetchingUntilConnected) {
 }
 
 TEST_F(GCMAccountTrackerTest, InvalidateExpiredTokens) {
-  StartAccountSignIn(kAccountId1);
-  StartAccountSignIn(kAccountId2);
+  StartAccountAddition(kAccountId1);
+  StartAccountAddition(kAccountId2);
   tracker()->Start();
-  FinishAccountSignIn(kAccountId1);
-  FinishAccountSignIn(kAccountId2);
+  FinishAccountAddition(kAccountId1);
+  FinishAccountAddition(kAccountId2);
 
   EXPECT_EQ(2UL, tracker()->get_pending_token_request_count());
 
@@ -453,8 +460,8 @@ TEST_F(GCMAccountTrackerTest, IsTokenFetchingRequired) {
   tracker()->Start();
   driver()->SetConnected(false);
   EXPECT_FALSE(IsFetchingRequired());
-  StartAccountSignIn(kAccountId1);
-  FinishAccountSignIn(kAccountId1);
+  StartAccountAddition(kAccountId1);
+  FinishAccountAddition(kAccountId1);
   EXPECT_TRUE(IsFetchingRequired());
 
   driver()->SetConnected(true);
@@ -463,8 +470,8 @@ TEST_F(GCMAccountTrackerTest, IsTokenFetchingRequired) {
   EXPECT_FALSE(IsFetchingRequired());
 
   driver()->SetConnected(false);
-  StartAccountSignIn(kAccountId2);
-  FinishAccountSignIn(kAccountId2);
+  StartAccountAddition(kAccountId2);
+  FinishAccountAddition(kAccountId2);
   EXPECT_TRUE(IsFetchingRequired());
 
   IssueExpiredAccessToken(kAccountId2);
@@ -506,12 +513,12 @@ TEST_F(GCMAccountTrackerTest, IsTokenReportingRequired) {
   driver()->SetLastTokenFetchTime(base::Time::Now());
   EXPECT_FALSE(IsTokenReportingRequired());
 
-  SignInAccount(kAccountId1);
+  AddAccount(kAccountId1);
   IssueAccessToken(kAccountId1);
   driver()->ResetResults();
   // Reporting was triggered, which means testing for required will give false,
   // but we have the update call.
-  SignOutAccount(kAccountId1);
+  RemoveAccount(kAccountId1);
   EXPECT_TRUE(driver()->update_accounts_called());
   EXPECT_FALSE(IsTokenReportingRequired());
 }

google_apis/gaia/fake_identity_provider.cc

@@ -13,8 +13,12 @@ FakeIdentityProvider::FakeIdentityProvider(OAuth2TokenService* token_service)
 FakeIdentityProvider::~FakeIdentityProvider() {
 }
 
-void FakeIdentityProvider::LogIn(const std::string& account_id) {
+void FakeIdentityProvider::SetActiveUsername(const std::string& account_id) {
   account_id_ = account_id;
+}
+
+void FakeIdentityProvider::LogIn(const std::string& account_id) {
+  SetActiveUsername(account_id);
   FireOnActiveAccountLogin();
 }
 

google_apis/gaia/fake_identity_provider.h

@@ -19,7 +19,13 @@ class FakeIdentityProvider : public IdentityProvider {
   explicit FakeIdentityProvider(OAuth2TokenService* token_service);
   ~FakeIdentityProvider() override;
 
+  // Sets the active username.
+  void SetActiveUsername(const std::string& account_id);
+
+  // Sets the active username and fires the OnActiveAccountLogin() callback.
   void LogIn(const std::string& account_id);
+
+  // Clears the active username and fires the OnActiveAccountLogout() callback.
   void LogOut();
 
   // IdentityProvider: