Obfuscated Gaia ID fetcher

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

+// 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 "chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h"
+#include "chrome/browser/extensions/api/push_messaging/stub_url_fetcher.h"

[Munjal (Google), 2012/08/15 23:04:19]

I think I commented about this before. There are already a few URL fetcher
classes for testing purposes. Let us not create our own.

[Pete Williamson, 2012/08/19 22:05:14]

I replied to your comment before with some reasons why I though having the
stub_url_fetcher class was a good idea.  I'll discuss this in person with you.

+#include "chrome/browser/extensions/api/push_messaging/push_messaging_api.h"
+
+namespace {
+
+static const char kGoodData[] = "{ \"id\" : \"My-channel-id\" }";
+static const char kBadJsonData[] = "I am not a JSON string";
+static const char kDictionaryMissingIdData[] = "{ \"foo\" : \"bar\" }";
+static const char kNonDictionaryJsonData[] = "{ 0.5 }";
+
+// Delegate class for catching notifications from the ObfuscatedGaiaIdFetcher.
+class TestDelegate : public extensions::PushMessagingGetChannelIdFunction {
+ public:
+  // inherited methods
+  virtual void OnObfuscatedGaiaIdFetchSuccess(
+      const std::string& access_token) {
+    succeeded_ = true;
+  }
+  virtual void OnObfuscatedGaiaIdFetchFailure(
+      const GoogleServiceAuthError& error) {
+    failed_ = true;
+  }
+  TestDelegate() {
+    succeeded_ = false;
+    failed_ = false;
+  }
+
+  bool Succeeded() { return succeeded_; }
+  bool Failed() { return failed_; }
+
+ private:
+virtual ~TestDelegate() {}

[Munjal (Google), 2012/08/15 23:04:19]

Indentation.

[Pete Williamson, 2012/08/19 22:05:14]

Done.

+  bool succeeded_;
+  bool failed_;

[dcheng, 2012/08/16 19:12:43]

DISALLOW_COPY_AND_ASSIGN.

[Pete Williamson, 2012/08/19 22:05:14]

Done, but why?  It seems like this class is not in any danger of being copied,
and since it is in a private namespace already, it is not in any danger of being
used in other files.  Does the extra safety really justify the extra code
complexity (admittedly minor) of adding this line, or is this a coding standard
that I missed reading somewhere?

[dcheng, 2012/08/19 22:38:35]

Unless a class needs to be copyable or assignable, the style convention is to
explicitly disallow it.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Copy_Constructors

+};
+
+}  // namespace
+
+namespace extensions {
+
+TEST(ObfuscatedGaiaIdFetcherTest, ParseResponse) {
+  std::string channel_id_out1;
+  std::string channel_id_out2;
+  std::string channel_id_out3;
+  std::string channel_id_out4;

[Munjal (Google), 2012/08/15 23:04:19]

I generalyl prefer separate scopes since even if you have different variables,
you could forget to use the right one. Having a different scope means compiler
catches all errors for us and compiler makes sure we start with a clean state.

But I leave it to you to decide.

[dcheng, 2012/08/16 19:12:43]

I don't care about scoped vs non-scoped. However, please move variables nearer
to where they are used.

[Pete Williamson, 2012/08/19 22:05:14]

I generally prefer not to use scopes unless the code absolutely requires it,
since when I read code, every time I see a new scope, it is a speed bump to my
understanding the code as I need to wonder why it is there, so I prefer to make
the tests independent through other means.

[Pete Williamson, 2012/08/19 22:05:14]

Done, but why?  These unit tests are really fast already, and further
micro-optimization won't gain us much.  Do you think it makes the code more
readable to have variables closer to the point of use instead of at the top? 
(It's OK if you do, I'm just trying to understand the reason behind the
suggestion.)  Is it just a matter of chrome coding style to declare variables
close to use always?

[dcheng, 2012/08/19 22:38:35]

This isn't for optimization reasons, just readability.

+  TestDelegate* delegate = new TestDelegate();

[dcheng, 2012/08/16 19:12:43]

Do we leak this? In general, all "new" allocations should be wrapped in a smart
pointer.

[Pete Williamson, 2012/08/19 22:05:14]

Nope, it deletes itself with the current design.  If we keep that design, I'll
add a comment here (and in the other tests where we allocate it).

+  std::string refresh_token;
+  ObfuscatedGaiaIdFetcher fetcher(NULL, delegate, refresh_token);
+
+  // Try a good response string.
+  bool ret1 = fetcher.ParseResponse(kGoodData, &channel_id_out1);
+  ASSERT_EQ(channel_id_out1, "My-channel-id");

[dcheng, 2012/08/16 19:12:43]

Prefer EXPECT instead of ASSERT. ASSERT() should be used for things where a test
cannot possibly continue (for example, you call Foo::CreateInstance() and
CreateInstance() fails for whatever reason).

ASSERT terminates the current test while EXPECT will mark a failure but allow
the rest of the test case to run, which is typically what you want.

[Pete Williamson, 2012/08/19 22:05:14]

Done, changed all ASSERTs in this file to EXPECT.  FYI - I've actually received
the opposite guidance in the past, but I'm happy with either way, and I'll be
happy to use EXPECT if that is the way we do it here.  (The theory is that unit
tests are important, and you shouldn't keep running them if one fails until that
one is fixed.  The counter-argument goes that you sometimes fix more than one
failing test per run.  In practice, I find that I normally only fix one anyway,
so it doesn't make so much difference to me.)

[dcheng, 2012/08/19 22:38:35]

This is just the general convention with googletest:
http://googletesting.blogspot.com/2008/07/tott-expect-vs-assert.html

+  ASSERT_TRUE(ret1);
+
+  // Try badly formattedf JSON.
+  bool ret2 = fetcher.ParseResponse(kBadJsonData, &channel_id_out2);
+  ASSERT_NE(channel_id_out2, "My-channel-id");
+  ASSERT_FALSE(ret2);
+
+  // Try a JSON dictionary with no "id" value.
+  bool ret3 = fetcher.ParseResponse(kDictionaryMissingIdData, &channel_id_out3);
+  ASSERT_NE(channel_id_out3, "My-channel-id");

[dcheng, 2012/08/16 19:12:43]

If channel_id_out3 is supposed to be empty, then we should say
EXPECT_TRUE(channel_id_out3.empty());

+  ASSERT_FALSE(ret3);
+
+  // Try JSON, but not a dictionary.
+  bool ret4 = fetcher.ParseResponse(kNonDictionaryJsonData, &channel_id_out4);
+  ASSERT_NE(channel_id_out4, "My-channel-id");
+  ASSERT_FALSE(ret4);
+}
+
+TEST(ObfuscatedGaiaIdFetcherTest, ProcessApiCallSuccess) {
+  TestDelegate* delegate = new TestDelegate();

[Munjal (Google), 2012/08/15 23:04:19]

Let us not use bare pointers. Use scoped_ptr or scoped_refptr. Here and
everywhere.

[Pete Williamson, 2012/08/19 22:05:14]

As noted earlier, this is an artifact of using a self deleting reference.  I'll
fix this when you, Daniel, and I agree on the best approach.

+  std::string refresh_token;
+  ObfuscatedGaiaIdFetcher* fetcher =
+      new ObfuscatedGaiaIdFetcher(NULL, delegate, refresh_token);
+  const net::StubURLFetcher goodFetcher(kGoodData, true);
+
+  // Test the happy path.
+
+  // Keep the delegate alive, since the callback would make its refcount go to 0
+  delegate->AddRef();

[Munjal (Google), 2012/08/15 23:04:19]

Again, don't reuse delegates, or any such objects, across tests. It is very easy
to introduce a false positive or other bugs. Just declare a new delegate to use
for different tests.

[Pete Williamson, 2012/08/19 22:05:14]

We aren't re-using the delegate here.  This is to keep the delegate alive during
the test, due to the current design of self deleting delegates that I will talk
to You and Daniel about in person.

+  fetcher->ProcessApiCallSuccess(&goodFetcher);
+  ASSERT_TRUE(delegate->Succeeded());
+  delegate->Release();
+}
+
+TEST(ObfuscatedGaiaIdFetcherTest, ProcessApiCallFailure) {
+  TestDelegate* delegate = new TestDelegate();
+  std::string refresh_token;
+  ObfuscatedGaiaIdFetcher* fetcher =
+      new ObfuscatedGaiaIdFetcher(NULL, delegate, refresh_token);
+  const net::StubURLFetcher badFetcher(kDictionaryMissingIdData, true);
+
+  // Test with bad data, ensure it fails.
+  // Keep the delegate alive, since the callback would make its refcount go to 0
+  delegate->AddRef();
+  fetcher->ProcessApiCallSuccess(&badFetcher);
+  ASSERT_TRUE(delegate->Failed());
+  delegate->Release();
+
+  // TODO: add case for when the base class fails and calls
+  // ProcessApiCallFailure
+}
+
+}  // namespace extensions