[GCM] Handling connection events in GCMAccountTracker

chrome/browser/services/gcm/gcm_account_tracker_unittest.cc

Index: chrome/browser/services/gcm/gcm_account_tracker_unittest.cc
diff --git a/chrome/browser/services/gcm/gcm_account_tracker_unittest.cc b/chrome/browser/services/gcm/gcm_account_tracker_unittest.cc
index 0abc15ac2d4daef69451b65f5e69d01d728b176c..d3911f022e6e58319955ad87312a288c0f92c085 100644
--- a/chrome/browser/services/gcm/gcm_account_tracker_unittest.cc
+++ b/chrome/browser/services/gcm/gcm_account_tracker_unittest.cc
@@ -9,6 +9,8 @@
 
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "components/gcm_driver/fake_gcm_driver.h"
 #include "google_apis/gaia/fake_identity_provider.h"
 #include "google_apis/gaia/fake_oauth2_token_service.h"
 #include "google_apis/gaia/google_service_auth_error.h"
@@ -61,6 +63,81 @@ void VerifyAccountTokens(
   }
 }
 
+class CustomFakeGCMDriver : public FakeGCMDriver {

[Nicolas Zea, 2014/10/08 00:36:35]

nit: comment what it does differently from the normal fake?

[fgorski, 2014/10/08 18:04:27]

Done.

+ public:
+  CustomFakeGCMDriver();
+  virtual ~CustomFakeGCMDriver();
+
+  virtual void SetAccountTokens(
+      const std::vector<GCMClient::AccountTokenInfo>& account_tokens) OVERRIDE;

[Nicolas Zea, 2014/10/08 00:36:35]

nit: OVERRIDE -> override

[fgorski, 2014/10/08 18:04:27]

Done.

+  virtual void AddConnectionObserver(GCMConnectionObserver* observer) OVERRIDE;
+  virtual void RemoveConnectionObserver(
+      GCMConnectionObserver* observer) OVERRIDE;
+  virtual bool IsConnected() const OVERRIDE { return connected_; }
+
+  void set_connected(bool connected) {

[Nicolas Zea, 2014/10/08 00:36:35]

uber nit: the hacker_style inline method is generally just for setters/getters.
This is also calling a virtual observer method, so should be named SetConnected.

[fgorski, 2014/10/08 18:04:27]

Uber done.

+    connected_ = connected;
+    if (connected && last_connection_observer_)
+      last_connection_observer_->OnConnected(ip_endpoint_);
+  }
+
+  // Test results and helpers.
+  void ResetResults();
+  bool update_accounts_called() const { return update_accounts_called_; }
+  const std::vector<GCMClient::AccountTokenInfo>& accounts() const {
+    return accounts_;
+  }
+  const GCMConnectionObserver* last_connection_observer() const {
+    return last_connection_observer_;
+  }
+  const GCMConnectionObserver* last_removed_connection_observer() const {
+    return removed_connection_observer_;
+  }
+
+ private:
+  bool connected_;
+  std::vector<GCMClient::AccountTokenInfo> accounts_;
+  bool update_accounts_called_;
+  GCMConnectionObserver* last_connection_observer_;
+  GCMConnectionObserver* removed_connection_observer_;
+  net::IPEndPoint ip_endpoint_;
+
+  DISALLOW_COPY_AND_ASSIGN(CustomFakeGCMDriver);
+};
+
+CustomFakeGCMDriver::CustomFakeGCMDriver()
+    : connected_(true),
+      update_accounts_called_(false),
+      last_connection_observer_(NULL),
+      removed_connection_observer_(NULL) {
+}
+
+CustomFakeGCMDriver::~CustomFakeGCMDriver() {
+}
+
+void CustomFakeGCMDriver::SetAccountTokens(
+    const std::vector<GCMClient::AccountTokenInfo>& accounts) {
+  update_accounts_called_ = true;
+  accounts_ = accounts;
+}
+
+void CustomFakeGCMDriver::AddConnectionObserver(
+    GCMConnectionObserver* observer) {
+  last_connection_observer_ = observer;
+}
+
+void CustomFakeGCMDriver::RemoveConnectionObserver(
+    GCMConnectionObserver* observer) {
+  removed_connection_observer_ = observer;
+}
+
+void CustomFakeGCMDriver::ResetResults() {
+  accounts_.clear();
+  update_accounts_called_ = false;
+  last_connection_observer_ = NULL;
+  removed_connection_observer_ = NULL;
+}
+
 }  // namespace
 
 class GCMAccountTrackerTest : public testing::Test {
@@ -68,9 +145,6 @@ class GCMAccountTrackerTest : public testing::Test {
   GCMAccountTrackerTest();
   virtual ~GCMAccountTrackerTest();
 
-  // Callback for the account tracker.
-  void UpdateAccounts(const std::vector<GCMClient::AccountTokenInfo>& accounts);
-
   // 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.
@@ -81,21 +155,15 @@ class GCMAccountTrackerTest : public testing::Test {
 
   // Helpers for dealing with OAuth2 access token requests.
   void IssueAccessToken(const std::string& account_key);
+  void IssueExpiredAccessToken(const std::string& account_key);
   void IssueError(const std::string& account_key);
 
-  // Test results and helpers.
-  void ResetResults();
-  bool update_accounts_called() const { return update_accounts_called_; }
-  const std::vector<GCMClient::AccountTokenInfo>& accounts() const {
-    return accounts_;
-  }
-
-  // Accessor to account tracker.
+  // Accessors to account tracker and gcm driver.
   GCMAccountTracker* tracker() { return tracker_.get(); }
+  CustomFakeGCMDriver* driver() { return &driver_; }
 
  private:
-  std::vector<GCMClient::AccountTokenInfo> accounts_;
-  bool update_accounts_called_;
+  CustomFakeGCMDriver driver_;
 
   base::MessageLoop message_loop_;
   net::TestURLFetcherFactory test_fetcher_factory_;
@@ -104,8 +172,7 @@ class GCMAccountTrackerTest : public testing::Test {
   scoped_ptr<GCMAccountTracker> tracker_;
 };
 
-GCMAccountTrackerTest::GCMAccountTrackerTest()
-    : update_accounts_called_(false) {
+GCMAccountTrackerTest::GCMAccountTrackerTest() {
   fake_token_service_.reset(new FakeOAuth2TokenService());
 
   fake_identity_provider_.reset(
@@ -116,10 +183,7 @@ GCMAccountTrackerTest::GCMAccountTrackerTest()
                                new net::TestURLRequestContextGetter(
                                    message_loop_.message_loop_proxy())));
 
-  tracker_.reset(new GCMAccountTracker(
-      gaia_account_tracker.Pass(),
-      base::Bind(&GCMAccountTrackerTest::UpdateAccounts,
-                 base::Unretained(this))));
+  tracker_.reset(new GCMAccountTracker(gaia_account_tracker.Pass(), &driver_));
 }
 
 GCMAccountTrackerTest::~GCMAccountTrackerTest() {
@@ -127,17 +191,6 @@ GCMAccountTrackerTest::~GCMAccountTrackerTest() {
     tracker_->Shutdown();
 }
 
-void GCMAccountTrackerTest::UpdateAccounts(
-    const std::vector<GCMClient::AccountTokenInfo>& accounts) {
-  update_accounts_called_ = true;
-  accounts_ = accounts;
-}
-
-void GCMAccountTrackerTest::ResetResults() {
-  accounts_.clear();
-  update_accounts_called_ = false;
-}
-
 void GCMAccountTrackerTest::StartAccountSignIn(const std::string& account_key) {
   fake_identity_provider_->LogIn(account_key);
   fake_token_service_->AddAccount(account_key);
@@ -169,19 +222,25 @@ void GCMAccountTrackerTest::IssueAccessToken(const std::string& account_key) {
       account_key, MakeAccessToken(account_key), base::Time::Max());
 }
 
+void GCMAccountTrackerTest::IssueExpiredAccessToken(
+    const std::string& account_key) {
+  fake_token_service_->IssueAllTokensForAccount(
+      account_key, MakeAccessToken(account_key), base::Time::Now());
+}
+
 void GCMAccountTrackerTest::IssueError(const std::string& account_key) {
   fake_token_service_->IssueErrorForAllPendingRequestsForAccount(
       account_key,
       GoogleServiceAuthError(GoogleServiceAuthError::SERVICE_UNAVAILABLE));
 }
 
+// TODO(fgorsk): This actually need fixing.

[Nicolas Zea, 2014/10/08 00:36:35]

Is this comment still relevant?

[fgorski, 2014/10/08 18:04:27]

Removing.

 TEST_F(GCMAccountTrackerTest, NoAccounts) {
-  EXPECT_FALSE(update_accounts_called());
+  EXPECT_FALSE(driver()->update_accounts_called());
   tracker()->Start();
   // Callback should not be called if there where no accounts provided.
-  EXPECT_FALSE(update_accounts_called());
-  EXPECT_TRUE(accounts().empty());
-  tracker()->Stop();
+  EXPECT_FALSE(driver()->update_accounts_called());
+  EXPECT_TRUE(driver()->accounts().empty());
 }
 
 // Verifies that callback is called after a token is issued for a single account
@@ -193,18 +252,17 @@ TEST_F(GCMAccountTrackerTest, SingleAccount) {
   tracker()->Start();
   // We don't have any accounts to report, but given the inner account tracker
   // is still working we don't make a call with empty accounts list.
-  EXPECT_FALSE(update_accounts_called());
+  EXPECT_FALSE(driver()->update_accounts_called());
 
   // This concludes the work of inner account tracker.
   FinishAccountSignIn(kAccountId1);
   IssueAccessToken(kAccountId1);
 
-  EXPECT_TRUE(update_accounts_called());
+  EXPECT_TRUE(driver()->update_accounts_called());
 
   std::vector<GCMClient::AccountTokenInfo> expected_accounts;
   expected_accounts.push_back(MakeAccountToken(kAccountId1));
-  VerifyAccountTokens(expected_accounts, accounts());
-  tracker()->Stop();
+  VerifyAccountTokens(expected_accounts, driver()->accounts());
 }
 
 TEST_F(GCMAccountTrackerTest, MultipleAccounts) {
@@ -212,39 +270,35 @@ TEST_F(GCMAccountTrackerTest, MultipleAccounts) {
   StartAccountSignIn(kAccountId2);
 
   tracker()->Start();
-  EXPECT_FALSE(update_accounts_called());
+  EXPECT_FALSE(driver()->update_accounts_called());
 
   FinishAccountSignIn(kAccountId1);
   IssueAccessToken(kAccountId1);
-  EXPECT_FALSE(update_accounts_called());
+  EXPECT_FALSE(driver()->update_accounts_called());
 
   FinishAccountSignIn(kAccountId2);
   IssueAccessToken(kAccountId2);
-  EXPECT_TRUE(update_accounts_called());
+  EXPECT_TRUE(driver()->update_accounts_called());
 
   std::vector<GCMClient::AccountTokenInfo> expected_accounts;
   expected_accounts.push_back(MakeAccountToken(kAccountId1));
   expected_accounts.push_back(MakeAccountToken(kAccountId2));
-  VerifyAccountTokens(expected_accounts, accounts());
-
-  tracker()->Stop();
+  VerifyAccountTokens(expected_accounts, driver()->accounts());
 }
 
 TEST_F(GCMAccountTrackerTest, AccountAdded) {
   tracker()->Start();
-  ResetResults();
+  driver()->ResetResults();
 
   SignInAccount(kAccountId1);
-  EXPECT_FALSE(update_accounts_called());
+  EXPECT_FALSE(driver()->update_accounts_called());
 
   IssueAccessToken(kAccountId1);
-  EXPECT_TRUE(update_accounts_called());
+  EXPECT_TRUE(driver()->update_accounts_called());
 
   std::vector<GCMClient::AccountTokenInfo> expected_accounts;
   expected_accounts.push_back(MakeAccountToken(kAccountId1));
-  VerifyAccountTokens(expected_accounts, accounts());
-
-  tracker()->Stop();
+  VerifyAccountTokens(expected_accounts, driver()->accounts());
 }
 
 TEST_F(GCMAccountTrackerTest, AccountRemoved) {
@@ -254,19 +308,17 @@ TEST_F(GCMAccountTrackerTest, AccountRemoved) {
   tracker()->Start();
   IssueAccessToken(kAccountId1);
   IssueAccessToken(kAccountId2);
-  EXPECT_TRUE(update_accounts_called());
+  EXPECT_TRUE(driver()->update_accounts_called());
 
-  ResetResults();
-  EXPECT_FALSE(update_accounts_called());
+  driver()->ResetResults();
+  EXPECT_FALSE(driver()->update_accounts_called());
 
   SignOutAccount(kAccountId2);
-  EXPECT_TRUE(update_accounts_called());
+  EXPECT_TRUE(driver()->update_accounts_called());
 
   std::vector<GCMClient::AccountTokenInfo> expected_accounts;
   expected_accounts.push_back(MakeAccountToken(kAccountId1));
-  VerifyAccountTokens(expected_accounts, accounts());
-
-  tracker()->Stop();
+  VerifyAccountTokens(expected_accounts, driver()->accounts());
 }
 
 TEST_F(GCMAccountTrackerTest, GetTokenFailed) {
@@ -275,16 +327,20 @@ TEST_F(GCMAccountTrackerTest, GetTokenFailed) {
 
   tracker()->Start();
   IssueAccessToken(kAccountId1);
-  EXPECT_FALSE(update_accounts_called());
+  EXPECT_FALSE(driver()->update_accounts_called());
 
   IssueError(kAccountId2);
-  EXPECT_TRUE(update_accounts_called());
+  EXPECT_FALSE(driver()->update_accounts_called());
+
+  EXPECT_EQ(1UL, tracker()->get_pending_token_request_count());
+
+  IssueAccessToken(kAccountId2);
+  EXPECT_TRUE(driver()->update_accounts_called());
 
   std::vector<GCMClient::AccountTokenInfo> expected_accounts;
   expected_accounts.push_back(MakeAccountToken(kAccountId1));
-  VerifyAccountTokens(expected_accounts, accounts());
-
-  tracker()->Stop();
+  expected_accounts.push_back(MakeAccountToken(kAccountId2));
+  VerifyAccountTokens(expected_accounts, driver()->accounts());
 }
 
 TEST_F(GCMAccountTrackerTest, GetTokenFailedAccountRemoved) {
@@ -295,15 +351,15 @@ TEST_F(GCMAccountTrackerTest, GetTokenFailedAccountRemoved) {
   IssueAccessToken(kAccountId1);
   IssueError(kAccountId2);
 
-  ResetResults();
+  driver()->ResetResults();
   SignOutAccount(kAccountId2);
-  EXPECT_TRUE(update_accounts_called());
+  IssueError(kAccountId2);
+
+  EXPECT_TRUE(driver()->update_accounts_called());
 
   std::vector<GCMClient::AccountTokenInfo> expected_accounts;
   expected_accounts.push_back(MakeAccountToken(kAccountId1));
-  VerifyAccountTokens(expected_accounts, accounts());
-
-  tracker()->Stop();
+  VerifyAccountTokens(expected_accounts, driver()->accounts());
 }
 
 TEST_F(GCMAccountTrackerTest, AccountRemovedWhileRequestsPending) {
@@ -312,17 +368,55 @@ TEST_F(GCMAccountTrackerTest, AccountRemovedWhileRequestsPending) {
 
   tracker()->Start();
   IssueAccessToken(kAccountId1);
-  EXPECT_FALSE(update_accounts_called());
+  EXPECT_FALSE(driver()->update_accounts_called());
 
   SignOutAccount(kAccountId2);
   IssueAccessToken(kAccountId2);
-  EXPECT_TRUE(update_accounts_called());
+  EXPECT_TRUE(driver()->update_accounts_called());
 
   std::vector<GCMClient::AccountTokenInfo> expected_accounts;
   expected_accounts.push_back(MakeAccountToken(kAccountId1));
-  VerifyAccountTokens(expected_accounts, accounts());
+  VerifyAccountTokens(expected_accounts, driver()->accounts());
+}
+
+// Makes sure that tracker observes GCM connection when running.
+TEST_F(GCMAccountTrackerTest, TrackerObservesConnection) {
+  EXPECT_EQ(NULL, driver()->last_connection_observer());
+  tracker()->Start();
+  EXPECT_EQ(tracker(), driver()->last_connection_observer());
+  tracker()->Shutdown();
+  EXPECT_EQ(tracker(), driver()->last_removed_connection_observer());
+}
+
+// Makes sure that token fetching happens only after connection is established.
+TEST_F(GCMAccountTrackerTest, PostponeTokenFetchingUntilConnected) {
+  driver()->set_connected(false);
+  StartAccountSignIn(kAccountId1);
+  tracker()->Start();
+  FinishAccountSignIn(kAccountId1);
 
-  tracker()->Stop();
+  EXPECT_EQ(0UL, tracker()->get_pending_token_request_count());
+  driver()->set_connected(true);
+
+  EXPECT_EQ(1UL, tracker()->get_pending_token_request_count());
+}
+
+TEST_F(GCMAccountTrackerTest, IvalidateExpiredTokens) {
+  StartAccountSignIn(kAccountId1);
+  StartAccountSignIn(kAccountId2);
+  tracker()->Start();
+  FinishAccountSignIn(kAccountId1);
+  FinishAccountSignIn(kAccountId2);
+
+  EXPECT_EQ(2UL, tracker()->get_pending_token_request_count());
+
+  IssueExpiredAccessToken(kAccountId1);
+  IssueAccessToken(kAccountId2);
+  // Because the first token is expired, we expect the sanitize to kick in and
+  // clean it up before the SetAccessToken is called. This also means a new
+  // token request will be issued
+  EXPECT_FALSE(driver()->update_accounts_called());
+  EXPECT_EQ(1UL, tracker()->get_pending_token_request_count());
 }
 
 // TODO(fgorski): Add test for adding account after removal >> make sure it does