Obfuscated Gaia ID fetcher

chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc

Index: chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc
diff --git a/chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc b/chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b041136c7982ed78b71d96e9626d7e36e216a5b4
--- /dev/null
+++ b/chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher_unittest.cc
@@ -0,0 +1,116 @@
+// Copyright (c) 2012 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 "testing/gtest/include/gtest/gtest.h"
+
+#include <string>
+
+#include "obfuscated_gaia_id_fetcher.h"
+#include "stub_url_fetcher.h"

[Munjal (Google), 2012/08/09 21:44:29]

Include paths should be relative to src/

[Pete Williamson, 2012/08/13 21:21:14]

Done.

+
+namespace extensions {
+
+const std::string fakeGoodData("{ \"id\" : \"My-channel-id\" }");
+const std::string fakeBadData1("I am not a JSON string");
+const std::string fakeBadData2("{ \"foo\" : \"bar\" }");
+const std::string fakeBadData3("{ 0.5 }");

[Munjal (Google), 2012/08/09 21:44:29]

Put all of these as static const char[] in anonymous namespace.

[Pete Williamson, 2012/08/13 21:21:14]

Done.

+std::string channelIdOut;
+net::URLFetcher* fakeSource;

[Munjal (Google), 2012/08/09 21:44:29]

Do not use non-const global variables.

[Pete Williamson, 2012/08/13 21:21:14]

Done.

+
+// delegate class for catching notifications from teh ObfuscatedGaiaIdFetcher

[Munjal (Google), 2012/08/09 21:44:29]

Nit teh -> the

[Pete Williamson, 2012/08/13 21:21:14]

Done.

+class FetcherDelegate : public ObfuscatedGaiaIdFetcher::Delegate {

[Munjal (Google), 2012/08/09 21:44:29]

Name this TestDelegate and put it in an anonymous namespace.

[Pete Williamson, 2012/08/13 21:21:14]

Done.

+ public:
+  // inherited methods
+  virtual void OnObfuscatedGaiaIdFetchSuccess(
+      const std::string& access_token) {
+    succeeded_ = true;
+  }
+  virtual void OnObfuscatedGaiaIdFetchFailure(
+      const GoogleServiceAuthError& error) {
+    failed_ = true;
+  }
+  FetcherDelegate() {
+    Clear();
+  }
+  virtual ~FetcherDelegate() {}
+  void Clear() {
+    succeeded_ = false;
+    failed_ = false;
+  }
+  bool Succeeded() { return succeeded_; }
+  bool Failed() { return failed_; }
+
+ private:
+  bool succeeded_;
+  bool failed_;
+};
+
+class ObfuscatedGaiaIdFetcherTest : public testing::Test {
+ public:
+  virtual void SetUp() {
+    // Create a fetcher that we can use for the tests, and initialize it well
+    // enough for the tests to pass.
+    ObfuscatedGaiaIdFetcher::Parameters params;
+    fetcher_ = new ObfuscatedGaiaIdFetcher(NULL, &delegate_, params);
+
+    // set up mocks
+    goodFetcher_ = new net::StubURLFetcher(fakeGoodData, true);
+    badFetcher1_ = new net::StubURLFetcher(fakeBadData1, false);
+    badFetcher2_ = new net::StubURLFetcher(fakeBadData2, true);
+    badFetcher3_ = new net::StubURLFetcher(fakeBadData3, true);

[Munjal (Google), 2012/08/09 21:44:29]

It is better for each individual test to create their own fetchers.

[Pete Williamson, 2012/08/13 21:21:14]

Done.

+  }
+
+  ObfuscatedGaiaIdFetcher* GetFetcher() { return fetcher_; }
+
+ protected:
+  // TODO: use a scoped pointer to release these at dtor time

[Munjal (Google), 2012/08/09 21:44:29]

You missed doing this TODO. I think it is anyway better to define all of this
localy in individual tests.

[Pete Williamson, 2012/08/13 21:21:14]

Done.

+  ObfuscatedGaiaIdFetcher* fetcher_;
+  FetcherDelegate delegate_;
+  net::StubURLFetcher* goodFetcher_;
+  net::StubURLFetcher* badFetcher1_;
+  net::StubURLFetcher* badFetcher2_;
+  net::StubURLFetcher* badFetcher3_;
+
+};
+
+TEST_F( ObfuscatedGaiaIdFetcherTest, ParseResponse) {
+  ObfuscatedGaiaIdFetcher* fetcher = GetFetcher();
+  bool ret = false;
+  ret = fetcher->ParseResponse(fakeGoodData, &channelIdOut);
+  ASSERT_TRUE(channelIdOut == "My-channel-id" && ret);

[Munjal (Google), 2012/08/09 21:44:29]

Split the compound check into multiple asserts so that you get better output
when the test fails.
So do
ASSERT_EQ("My-channel-id", channelIdOut);
ASSERT_FALSE(ret);

Same with rest of the tests.

[Pete Williamson, 2012/08/13 21:21:14]

Done.

+  channelIdOut.clear();

[Munjal (Google), 2012/08/09 21:44:29]

I generally don't link reusing the variables like this. If you forget to clear,
some tests might silently pass when they should fail. It is very hard to catch
such false positivies.

use a new scope to define fresh variables every time. So something like:
{  // Good response string.
  std::string channelIdOut;
  ASSERT_TRUE(fetcher->ParseResponse(fakeGoodData, &channelIdOut));
  ASSERT_EQ("My-channel-id", channelIdOut);
}
{  // Invalid JSON.
  std::string channelIdOut;
  ASSERT_FALSE(fetcher->ParseResponse(fakeBadData1, &channelIdOut));
  ASSERT_EQ("", channelIdOut);
}

[Pete Williamson, 2012/08/13 21:21:14]

Done.

+  ret = fetcher->ParseResponse(fakeBadData1, &channelIdOut);

[Munjal (Google), 2012/08/09 21:44:29]

Either replace fakeData1 with "I am not a JSON string" or name fakeData1
something better for readability of tests.

[Pete Williamson, 2012/08/13 21:21:14]

Done.

+  ASSERT_TRUE(channelIdOut != "My-channel-id" && !ret);
+  channelIdOut.clear();
+  ret = fetcher->ParseResponse(fakeBadData2, &channelIdOut);
+  ASSERT_TRUE(channelIdOut != "My-channel-id" && !ret);
+  channelIdOut.clear();
+  ret = fetcher->ParseResponse(fakeBadData3, &channelIdOut);
+  ASSERT_TRUE(channelIdOut != "My-channel-id" && !ret);
+}
+
+TEST_F( ObfuscatedGaiaIdFetcherTest, ProcessApiCallSuccess) {
+  ObfuscatedGaiaIdFetcher* fetcher = GetFetcher();
+
+  // happy path
+  delegate_.Clear();

[Munjal (Google), 2012/08/09 21:44:29]

Create fetcher and delegate locally to eliminate the chance of false positives
(and false negatives).

[Pete Williamson, 2012/08/13 21:21:14]

Done.

+  fetcher->ProcessApiCallSuccess(goodFetcher_);
+  ASSERT_TRUE(delegate_.Succeeded());
+}
+
+TEST_F( ObfuscatedGaiaIdFetcherTest, ProcessApiCallFailure) {
+  ObfuscatedGaiaIdFetcher* fetcher = GetFetcher();
+  // several varieties of failures should all return failure
+  delegate_.Clear();
+  fetcher->ProcessApiCallSuccess(badFetcher1_);

[Munjal (Google), 2012/08/09 21:44:29]

Since you tested ParseResponse thoroughly with each different type of bad
response, it is redundant to test all those combinations here also. Just test
with one bad response.

Also, there are other failure cases - when the API call request fails (i.e. when
the base class fails and calls ProcessApiCallFailure).

[Pete Williamson, 2012/08/13 21:21:14]

Done.

+  ASSERT_TRUE(delegate_.Failed());
+  delegate_.Clear();
+  fetcher->ProcessApiCallSuccess(badFetcher2_);
+  ASSERT_TRUE(delegate_.Failed());
+  delegate_.Clear();
+  fetcher->ProcessApiCallSuccess(badFetcher3_);
+  ASSERT_TRUE(delegate_.Failed());
+}
+
+} // namespace extensions