Client-to-server messages in GCMNetworkChannel

sync/notifier/gcm_network_channel_unittest.cc

Index: sync/notifier/gcm_network_channel_unittest.cc
diff --git a/sync/notifier/gcm_network_channel_unittest.cc b/sync/notifier/gcm_network_channel_unittest.cc
index 8656385dbcc61a148622d819b7dbbfab4f1e0f17..71f924e492cfac8f286659ef9703eb7c9d765b99 100644
--- a/sync/notifier/gcm_network_channel_unittest.cc
+++ b/sync/notifier/gcm_network_channel_unittest.cc
@@ -2,28 +2,67 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/run_loop.h"
+#include "google_apis/gaia/google_service_auth_error.h"
+#include "net/url_request/test_url_fetcher_factory.h"
+#include "net/url_request/url_request_test_util.h"
 #include "sync/notifier/gcm_network_channel.h"
-
-#include "base/compiler_specific.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace syncer {
 namespace {
 
+class TestGCMNetworkChannelDelegate : public GCMNetworkChannelDelegate {
+ public:
+  virtual void RequestToken(RequestTokenCallback callback) OVERRIDE {
+    request_token_callback = callback;
+  }
+
+  virtual void InvalidateToken(const std::string& token) OVERRIDE {
+    invalidated_token = token;
+  }
+
+  virtual void Register(RegisterCallback callback) OVERRIDE {
+    register_callback = callback;
+  }
+
+  RequestTokenCallback request_token_callback;
+  std::string invalidated_token;
+  RegisterCallback register_callback;
+};
+
 class GCMNetworkChannelTest
     : public ::testing::Test,
       public SyncNetworkChannel::Observer {
  protected:
   GCMNetworkChannelTest()
-      : gcm_network_channel_() {
-    gcm_network_channel_.AddObserver(this);
-    gcm_network_channel_.SetMessageReceiver(
+      : url_fetchers_created_count_(0) {
+  }
+
+  virtual ~GCMNetworkChannelTest() {
+  }
+
+  virtual void SetUp() {
+    request_context_getter_ = new net::TestURLRequestContextGetter(
+        base::MessageLoopProxy::current());
+    // Ownership of delegate goes to GCNMentworkChannel but test needs pointer
+    // to it.
+    delegate_ = new TestGCMNetworkChannelDelegate();
+    scoped_ptr<GCMNetworkChannelDelegate> delegate(
+        delegate_);

[rlarocque, 2014/01/16 18:52:48]

Can this fit on one line?

[pavely, 2014/01/17 00:44:39]

Done.

+    gcm_network_channel_.reset(new GCMNetworkChannel(request_context_getter_,
+        delegate.Pass()));

[rlarocque, 2014/01/16 18:52:48]

nit: alignment again.

[pavely, 2014/01/17 00:44:39]

Done.

+    gcm_network_channel_->AddObserver(this);
+    gcm_network_channel_->SetMessageReceiver(
         invalidation::NewPermanentCallback(
             this, &GCMNetworkChannelTest::OnIncomingMessage));
+    url_fetcher_factory_.reset(new net::FakeURLFetcherFactory(NULL,
+        base::Bind(&GCMNetworkChannelTest::CreateURLFetcher,
+            base::Unretained(this))));
   }
 
-  virtual ~GCMNetworkChannelTest() {
-    gcm_network_channel_.RemoveObserver(this);
+  virtual void TearDown() {
+    gcm_network_channel_->RemoveObserver(this);
   }
 
   virtual void OnNetworkChannelStateChanged(
@@ -33,8 +72,163 @@ class GCMNetworkChannelTest
   void OnIncomingMessage(std::string incoming_message) {
   }
 
-  GCMNetworkChannel gcm_network_channel_;
+  GCMNetworkChannel* network_channel() {
+    return gcm_network_channel_.get();
+  }
+
+  TestGCMNetworkChannelDelegate* delegate() {
+    return delegate_;
+  }
+
+  int url_fetchers_created_count() {
+    return url_fetchers_created_count_;
+  }
+
+  net::FakeURLFetcherFactory* url_fetcher_factory() {
+    return url_fetcher_factory_.get();
+  }
+
+  scoped_ptr<net::FakeURLFetcher> CreateURLFetcher(
+      const GURL& url,
+      net::URLFetcherDelegate* delegate,
+      const std::string& response_data,
+      net::HttpStatusCode response_code,
+      net::URLRequestStatus::Status status) {
+    url_fetchers_created_count_++;
+    return scoped_ptr<net::FakeURLFetcher>(new net::FakeURLFetcher(
+        url, delegate, response_data, response_code, status));
+  }
+
+ private:
+  base::MessageLoop message_loop_;
+  TestGCMNetworkChannelDelegate* delegate_;
+  scoped_ptr<GCMNetworkChannel> gcm_network_channel_;
+  scoped_refptr<net::TestURLRequestContextGetter> request_context_getter_;
+  scoped_ptr<net::FakeURLFetcherFactory> url_fetcher_factory_;
+  int url_fetchers_created_count_;
 };
 
+TEST_F(GCMNetworkChannelTest, HappyCase) {
+  GURL url("http://invalid.url.com");
+  url_fetcher_factory()->SetFakeResponse(url, "", net::HTTP_OK,
+      net::URLRequestStatus::SUCCESS);
+
+  // After construction GCMNetworkChannel should have called Register.
+  EXPECT_FALSE(delegate()->register_callback.is_null());
+  // Return valid registration id.
+  delegate()->register_callback.Run("registration.id", gcm::GCMClient::SUCCESS);
+
+  network_channel()->SendMessage("abra.cadabra");
+  // SendMessage should have triggered RequestToken. No HTTP request should be
+  // started yet.
+  EXPECT_FALSE(delegate()->request_token_callback.is_null());
+  EXPECT_EQ(url_fetchers_created_count(), 0);
+  // Return valid access token. This should trigger HTTP request.
+  delegate()->request_token_callback.Run(
+      GoogleServiceAuthError::AuthErrorNone(), "access.token");
+  {
+    base::RunLoop run_loop;
+    run_loop.RunUntilIdle();
+  }
+  EXPECT_EQ(url_fetchers_created_count(), 1);
+
+  // Return another access token. Message should be cleared by now and shouldn't
+  // be sent.
+  delegate()->request_token_callback.Run(
+      GoogleServiceAuthError::AuthErrorNone(), "access.token2");
+  {
+    base::RunLoop run_loop;
+    run_loop.RunUntilIdle();
+  }
+  EXPECT_EQ(url_fetchers_created_count(), 1);
+}
+
+TEST_F(GCMNetworkChannelTest, FailedRegister) {
+  // After construction GCMNetworkChannel should have called Register.
+  EXPECT_FALSE(delegate()->register_callback.is_null());
+  // Return error from Register call.
+  delegate()->register_callback.Run("", gcm::GCMClient::SERVER_ERROR);
+
+  network_channel()->SendMessage("abra.cadabra");
+  // SendMessage shouldn't trigger RequestToken.
+  EXPECT_TRUE(delegate()->request_token_callback.is_null());
+  EXPECT_EQ(url_fetchers_created_count(), 0);
+}
+
+TEST_F(GCMNetworkChannelTest, RegisterFinishesAfterSendMessage) {
+  GURL url("http://invalid.url.com");
+  url_fetcher_factory()->SetFakeResponse(url, "", net::HTTP_OK,
+      net::URLRequestStatus::SUCCESS);
+
+  // After construction GCMNetworkChannel should have called Register.
+  EXPECT_FALSE(delegate()->register_callback.is_null());
+
+  network_channel()->SendMessage("abra.cadabra");
+  // SendMessage shouldn't trigger RequestToken.
+  EXPECT_TRUE(delegate()->request_token_callback.is_null());
+  EXPECT_EQ(url_fetchers_created_count(), 0);
+
+  // Return valid registration id.
+  delegate()->register_callback.Run("registration.id", gcm::GCMClient::SUCCESS);
+
+  EXPECT_FALSE(delegate()->request_token_callback.is_null());
+  EXPECT_EQ(url_fetchers_created_count(), 0);
+  // Return valid access token. This should trigger HTTP request.
+  delegate()->request_token_callback.Run(
+      GoogleServiceAuthError::AuthErrorNone(), "access.token");
+  {
+    base::RunLoop run_loop;
+    run_loop.RunUntilIdle();
+  }
+  EXPECT_EQ(url_fetchers_created_count(), 1);
+}
+
+TEST_F(GCMNetworkChannelTest, RequestTokenFailure) {
+  // After construction GCMNetworkChannel should have called Register.
+  EXPECT_FALSE(delegate()->register_callback.is_null());
+  // Return valid registration id.
+  delegate()->register_callback.Run("registration.id", gcm::GCMClient::SUCCESS);
+
+  network_channel()->SendMessage("abra.cadabra");
+  // SendMessage should have triggered RequestToken. No HTTP request should be
+  // started yet.
+  EXPECT_FALSE(delegate()->request_token_callback.is_null());
+  EXPECT_EQ(url_fetchers_created_count(), 0);
+  // RequestToken returns failure.
+  delegate()->request_token_callback.Run(
+      GoogleServiceAuthError::FromConnectionError(1), "");
+
+  // Should be no HTTP requests.
+  EXPECT_EQ(url_fetchers_created_count(), 0);
+}
+
+TEST_F(GCMNetworkChannelTest, AuthErrorFromTango) {
+  // Setup fake response to return AUTH_ERROR.
+  GURL url("http://invalid.url.com");
+  url_fetcher_factory()->SetFakeResponse(url, "", net::HTTP_UNAUTHORIZED,
+      net::URLRequestStatus::SUCCESS);
+
+  // After construction GCMNetworkChannel should have called Register.
+  EXPECT_FALSE(delegate()->register_callback.is_null());
+  // Return valid registration id.
+  delegate()->register_callback.Run("registration.id", gcm::GCMClient::SUCCESS);
+
+  network_channel()->SendMessage("abra.cadabra");
+  // SendMessage should have triggered RequestToken. No HTTP request should be
+  // started yet.
+  EXPECT_FALSE(delegate()->request_token_callback.is_null());
+  EXPECT_EQ(url_fetchers_created_count(), 0);
+  // Return valid access token. This should trigger HTTP request.
+  delegate()->request_token_callback.Run(
+      GoogleServiceAuthError::AuthErrorNone(), "access.token");
+  {
+    base::RunLoop run_loop;
+    run_loop.RunUntilIdle();
+  }
+  EXPECT_EQ(url_fetchers_created_count(), 1);
+  EXPECT_EQ(delegate()->invalidated_token, "access.token");
+}
+

[rlarocque, 2014/01/16 18:52:48]

Does your current design still support the possibility that the callbacks
following a register or auth token refresh request will not be invoked?  Would
it make sense to have some tests for those scenarios?

[pavely, 2014/01/17 00:44:39]

It is only possible during shutdown. Not sure what to assert in this case.

On 2014/01/16 18:52:48, rlarocque wrote:

[rlarocque, 2014/01/17 01:40:47]

Are you sure that case is shutdown only?  What if the user disconnects their
account?

----

If you're worried that there could be crashes or memory leaks specific to the
shutdown scenario, then it might make sense to add the tests even if there are
no useful assertions in them.  Sometimes it makes sense to write tests knowing
that they're only useful when they're run under Valgrind or ASAN.  

If you're not worried about crashes or memory leaks, or if there are no special
risks around the shutdown scenario, then I agree there's no point in adding
extra tests.

On 2014/01/17 00:44:39, pavely wrote:

+

[rlarocque, 2014/01/16 18:52:48]

Remove extra whitespace.

[pavely, 2014/01/17 00:44:39]

Done.

 }  // namespace
 }  // namespace syncer