Create AccountServiceFlagFetcher which downloads an account's Gaia service flags.

chrome/browser/signin/account_service_flag_fetcher_unittest.cc

Index: chrome/browser/signin/account_service_flag_fetcher_unittest.cc
diff --git a/chrome/browser/signin/account_service_flag_fetcher_unittest.cc b/chrome/browser/signin/account_service_flag_fetcher_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..052e813f50473e7af7f8a8e0889fb20971631a34
--- /dev/null
+++ b/chrome/browser/signin/account_service_flag_fetcher_unittest.cc
@@ -0,0 +1,261 @@
+// Copyright 2014 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 "components/signin/core/browser/account_service_flag_fetcher.h"

[Bernhard Bauer, 2014/05/28 16:32:43]

This isn't really necessary. The point of this is that for every header file,
there should be one file that includes it first, so that the compiler will
complain about errors in the header for that file instead of an unrelated file
that includes it. We already have such a file though, so this include doesn't
need any special treatment.

[Marc Treib, 2014/06/02 09:14:41]

True. I did this for consistency, since most other unit tests seem to do the
same thing even when it's not strictly necessary. But sure, I've moved the
include down into the list.

+
+#include <string>
+#include <vector>
+
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/message_loop/message_loop_proxy.h"
+#include "base/strings/string_util.h"
+#include "chrome/browser/signin/fake_profile_oauth2_token_service.h"
+#include "google_apis/gaia/gaia_urls.h"
+#include "net/url_request/test_url_fetcher_factory.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+const char kAccountId[] = "user@gmail.com";
+const char kDifferentAccountId[] = "some_other_user@gmail.com";
+
+// TODO(treib): This class should really live in components/signin/ next to the
+// AccountServiceFlagFetcher, but it uses the FakePO2TS which lives in
+// chrome/browser/ (because it uses the AndroidPO2TS which depends on stuff from
+// chrome/browser/). So when the AndroidPO2TS is componentized, then this should
+// move as well.

[Bernhard Bauer, 2014/05/28 16:32:43]

Yeah, the whole class hierarchy is a bit weird there... :-(

+class AccountServiceFlagFetcherTest : public testing::Test {
+ public:
+  virtual void SetUp() OVERRIDE {
+    token_service_.reset(new FakeProfileOAuth2TokenService());
+    request_context_ = new net::TestURLRequestContextGetter(
+        base::MessageLoopProxy::current());
+    url_fetcher_factory_.reset(new net::FakeURLFetcherFactory(NULL));

[Andrew T Wilson (Slow), 2014/05/30 06:51:44]

nit: Does this need to be dynamically allocated? Consider making token_service_
and url_fetcher_factory_ be statically defined as class members instead of
scoped_ptr<>. In fact, I think all of these could be regular class members.

[Marc Treib, 2014/06/02 09:14:41]

The TestURLRequestContextGetter can't, because it has a protected dtor. The
others could, but I'd still want to realloc them in SetUp to make sure I get a
fresh instance for each test, and then I'd have a pointless extra initialization
in the ctor. So I'd prefer to keep it as is.

[Bernhard Bauer, 2014/06/02 10:25:42]

(Because it's refcounted).
The test fixture is destroyed after every test as well, so you'll get a fresh
instance in any case.

[Marc Treib, 2014/06/02 16:19:56]

Huh, so it is. Yep, in that case I can move everything into a ctor instead of
SetUp, and make the members non-pointers. Done.

+    service_flags_.clear();
+    service_flags_.push_back("some_flag");
+    service_flags_.push_back("another_flag");
+    service_flags_.push_back("andonemore");
+  }
+
+  MOCK_METHOD2(OnFlagsFetched,
+               void(AccountServiceFlagFetcher::ResultCode result,
+                    const std::vector<std::string>& flags));
+
+ protected:
+  void SetValidLoginResponse() {
+    url_fetcher_factory_->SetFakeResponse(
+        GaiaUrls::GetInstance()->oauth1_login_url(),
+        "SID=sid\nLSID=lsid\nAuth=auth\n",
+        net::HTTP_OK,
+        net::URLRequestStatus::SUCCESS);
+  }
+
+  void SetFailedLoginResponse() {
+    url_fetcher_factory_->SetFakeResponse(
+        GaiaUrls::GetInstance()->oauth1_login_url(),
+        "",

[Bernhard Bauer, 2014/05/28 16:32:43]

Use std::string() instead of the implicit constructor.

[Marc Treib, 2014/06/02 09:14:41]

Done.

+        net::HTTP_OK,
+        net::URLRequestStatus::CANCELED);
+  }
+
+  void SetValidGetUserInfoResponse() {
+    url_fetcher_factory_->SetFakeResponse(
+        GaiaUrls::GetInstance()->get_user_info_url(),
+        BuildGetUserInfoResponse(),
+        net::HTTP_OK,
+        net::URLRequestStatus::SUCCESS);
+  }
+
+  void SetInvalidGetUserInfoResponse() {
+    url_fetcher_factory_->SetFakeResponse(
+        GaiaUrls::GetInstance()->get_user_info_url(),
+        "allServicesIsMissing=true",
+        net::HTTP_OK,
+        net::URLRequestStatus::SUCCESS);
+  }
+
+  void SetFailedGetUserInfoResponse() {
+    url_fetcher_factory_->SetFakeResponse(
+        GaiaUrls::GetInstance()->get_user_info_url(),
+        "",
+        net::HTTP_OK,
+        net::URLRequestStatus::CANCELED);
+  }
+
+  std::string BuildGetUserInfoResponse() const {
+    return "allServices=" + JoinString(service_flags_, ',');
+  }
+
+  base::MessageLoop message_loop_;
+  scoped_ptr<FakeProfileOAuth2TokenService> token_service_;
+  scoped_refptr<net::TestURLRequestContextGetter> request_context_;
+  scoped_ptr<net::FakeURLFetcherFactory> url_fetcher_factory_;
+  net::ResponseCookies cookies_;
+  std::vector<std::string> service_flags_;
+};
+
+TEST_F(AccountServiceFlagFetcherTest, Success) {
+  token_service_->UpdateCredentials(kAccountId, "refresh_token");
+  SetValidLoginResponse();
+  SetValidGetUserInfoResponse();
+
+  AccountServiceFlagFetcher fetcher(
+      kAccountId,
+      token_service_.get(),
+      request_context_.get(),
+      base::Bind(&AccountServiceFlagFetcherTest::OnFlagsFetched,
+                 base::Unretained(this)));
+
+  // Since a refresh token is already available, we should immediately get a
+  // request for an access token.
+  EXPECT_EQ(token_service_->GetPendingRequests().size(), 1U);
+
+  token_service_->IssueAllTokensForAccount(
+      kAccountId,
+      "access_token",
+      base::Time::Now() + base::TimeDelta::FromHours(1));
+
+  EXPECT_CALL(*this, OnFlagsFetched(AccountServiceFlagFetcher::SUCCESS,
+                                    service_flags_));
+  message_loop_.RunUntilIdle();

[Bernhard Bauer, 2014/05/28 16:32:43]

Running a message loop directly is somewhat discouraged, as it can lead to
situations where the message loop is quit too early (or the wrong one is quit,
in the case of nested message loops), and Problems Ensue(tm). You should use
base::RunLoop().RunUntilIdle() here, or even better, instead of mocking out
AccountServiceFlagFetcherTest::OnFlagsFetched, implement it to take a callback
and run it, then bind that to the RunLoop::QuitClosure() above (which requires
you to construct the RunLoop further up). That way the run loop will run exactly
until OnFlagsFetched is called.

[Marc Treib, 2014/06/02 09:14:41]

Okay, I'm using RunLoop::RunUntilIdle now. I don't think the extra complexity to
run exactly until the OnFlagsFetched call is worth it here. And I'd still need a
separate mock method so I can EXPECT_CALL.

+}
+
+TEST_F(AccountServiceFlagFetcherTest, SuccessAfterWaitingForRefreshToken) {
+  SetValidLoginResponse();
+  SetValidGetUserInfoResponse();
+
+  AccountServiceFlagFetcher fetcher(
+      kAccountId,
+      token_service_.get(),
+      request_context_.get(),
+      base::Bind(&AccountServiceFlagFetcherTest::OnFlagsFetched,
+                 base::Unretained(this)));
+
+  // Since there is no refresh token yet, we should not get a request for an
+  // access token at this point.
+  EXPECT_EQ(token_service_->GetPendingRequests().size(), 0U);
+
+  token_service_->UpdateCredentials(kAccountId, "refresh_token");
+
+  // Now there is a refresh token and we should have got a request for an
+  // access token.
+  EXPECT_EQ(token_service_->GetPendingRequests().size(), 1U);
+
+  token_service_->IssueAllTokensForAccount(
+      kAccountId,
+      "access_token",
+      base::Time::Now() + base::TimeDelta::FromHours(1));
+
+  EXPECT_CALL(*this, OnFlagsFetched(AccountServiceFlagFetcher::SUCCESS,
+                                    service_flags_));
+  message_loop_.RunUntilIdle();
+}
+
+TEST_F(AccountServiceFlagFetcherTest, NoRefreshToken) {
+  AccountServiceFlagFetcher fetcher(
+      kAccountId,
+      token_service_.get(),
+      request_context_.get(),
+      base::Bind(&AccountServiceFlagFetcherTest::OnFlagsFetched,
+                 base::Unretained(this)));
+
+  token_service_->UpdateCredentials(kDifferentAccountId, "refresh_token");
+
+  // Credentials for a different user should be ignored, i.e. not result in a
+  // request for an access token.
+  EXPECT_EQ(token_service_->GetPendingRequests().size(), 0U);

[Bernhard Bauer, 2014/05/28 16:32:43]

Put the expected value first, for nicer error messages.

[Marc Treib, 2014/06/02 09:14:41]

Done.

+
+  // After all refresh tokens have been loaded, there is still no token for our
+  // user, so we expect a token error.
+  EXPECT_CALL(*this, OnFlagsFetched(AccountServiceFlagFetcher::TOKEN_ERROR,
+                                    std::vector<std::string>()));
+  token_service_->IssueAllRefreshTokensLoaded();
+}
+
+TEST_F(AccountServiceFlagFetcherTest, GetTokenFailure) {
+  token_service_->UpdateCredentials(kAccountId, "refresh_token");
+
+  AccountServiceFlagFetcher fetcher(
+      kAccountId,
+      token_service_.get(),
+      request_context_.get(),
+      base::Bind(&AccountServiceFlagFetcherTest::OnFlagsFetched,
+                 base::Unretained(this)));
+
+  // On failure to get an access token we expect a token error.
+  EXPECT_CALL(*this, OnFlagsFetched(AccountServiceFlagFetcher::TOKEN_ERROR,
+                                    std::vector<std::string>()));
+  token_service_->IssueErrorForAllPendingRequestsForAccount(
+      kAccountId,
+      GoogleServiceAuthError(GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS));
+}
+
+TEST_F(AccountServiceFlagFetcherTest, ClientLoginFailure) {
+  token_service_->UpdateCredentials(kAccountId, "refresh_token");
+  SetFailedLoginResponse();
+
+  AccountServiceFlagFetcher fetcher(
+      kAccountId,
+      token_service_.get(),
+      request_context_.get(),
+      base::Bind(&AccountServiceFlagFetcherTest::OnFlagsFetched,
+                 base::Unretained(this)));
+
+  token_service_->IssueAllTokensForAccount(
+      kAccountId,
+      "access_token",
+      base::Time::Now() + base::TimeDelta::FromHours(1));
+
+  // Login failure should result in a service error.
+  EXPECT_CALL(*this, OnFlagsFetched(AccountServiceFlagFetcher::SERVICE_ERROR,
+                                    std::vector<std::string>()));
+  message_loop_.RunUntilIdle();
+}
+
+TEST_F(AccountServiceFlagFetcherTest, GetUserInfoInvalidResponse) {
+  token_service_->UpdateCredentials(kAccountId, "refresh_token");
+  SetValidLoginResponse();
+  SetInvalidGetUserInfoResponse();
+
+  AccountServiceFlagFetcher fetcher(
+      kAccountId,
+      token_service_.get(),
+      request_context_.get(),
+      base::Bind(&AccountServiceFlagFetcherTest::OnFlagsFetched,
+                 base::Unretained(this)));
+
+  token_service_->IssueAllTokensForAccount(
+      kAccountId,
+      "access_token",
+      base::Time::Now() + base::TimeDelta::FromHours(1));
+
+  // Invalid response data from GetUserInfo should result in a service error.
+  EXPECT_CALL(*this, OnFlagsFetched(AccountServiceFlagFetcher::SERVICE_ERROR,
+                                    std::vector<std::string>()));
+  message_loop_.RunUntilIdle();
+}
+
+TEST_F(AccountServiceFlagFetcherTest, GetUserInfoFailure) {
+  token_service_->UpdateCredentials(kAccountId, "refresh_token");
+  SetValidLoginResponse();
+  SetFailedGetUserInfoResponse();
+
+  AccountServiceFlagFetcher fetcher(
+      kAccountId,
+      token_service_.get(),
+      request_context_.get(),
+      base::Bind(&AccountServiceFlagFetcherTest::OnFlagsFetched,
+                 base::Unretained(this)));
+
+  token_service_->IssueAllTokensForAccount(
+      kAccountId,
+      "access_token",
+      base::Time::Now() + base::TimeDelta::FromHours(1));
+
+  // Failed GetUserInfo call should result in a service error.
+  EXPECT_CALL(*this, OnFlagsFetched(AccountServiceFlagFetcher::SERVICE_ERROR,
+                                    std::vector<std::string>()));
+  message_loop_.RunUntilIdle();
+}

[Bernhard Bauer, 2014/05/28 16:32:43]

Would it also make sense to add a test that checks that the callback is not
called when the AccountServiceFlagFetcher is destroyed? You can ping me if you
want tips on how to test that.

[Marc Treib, 2014/06/02 09:14:41]

I've added a test for that. Is there more to it than a .Times(0)?

[Bernhard Bauer, 2014/06/02 10:25:42]

Yes and no. *In principle* you can do it with .Times(0) and RunUntilIdle(). The
problem is that it's surprisingly subtle to get RunUntilIdle right. For example,
in the latest patch set, you set up all the responses, but then destroy the
AccountServiceFlagFetcher before running the run loop, which means there is a
task in the message loop that would end up calling OnFlagsFetched. If something
in the implementation would start calling callbacks synchronously instead of
posting them to the message loop, the test would start failing. A more robust
solution would be to wait with e.g. the GetUserInfo response until the
AccountServiceFlagFetcher has been destroyed.

In more complicated tests, it could happen that RunUntilIdle will quit too soon,
or that it won't quit at all, which will cause flakiness and timeouts. Getting
these things right then requires knowing implementation details of the class
you're testing, which will in turn make the tests brittle.

If you use RunLoop::Run() with a quit closure, you can create a closure that
will fail the test if it is run, and causes the run loop to quit if it is
destroyed (i.e. the last reference to it is dropped) :)

[Marc Treib, 2014/06/02 16:19:56]

As discussed offline: The URLFetcher is now stubbed out differently, so I can
control when the results are returned.