Add tests for PushMessagingAppIdentifier

chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc

Index: chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc
diff --git a/chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc b/chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc
index e79364d22d2b08bbb06beb12f30580f399cb1f57..a7b8aafa3e919aecb470e1ec09189cddf8bef2cf 100644
--- a/chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc
+++ b/chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc
@@ -3,8 +3,14 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/push_messaging/push_messaging_app_identifier.h"
+
+#include "base/message_loop/message_loop.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/test/test_browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using content::BrowserThread;
+
 class PushMessagingAppIdentifierTest : public testing::Test {
  protected:
   PushMessagingAppIdentifier GenerateId(
@@ -38,3 +44,127 @@ TEST_F(PushMessagingAppIdentifierTest, UniqueGuids) {
             PushMessagingAppIdentifier::Generate(
                 GURL("https://www.example.com/"), 1).app_id());
 }
+
+namespace {
+
+void ExpectEq(const PushMessagingAppIdentifier& a,

[Peter Beverloo, 2015/05/12 13:55:40]

I'd slightly prefer to define an (anonymous) equality operator for PMAI:

bool operator==(const PushMessagingAppIdentifier& a,
                const PushMessagingAppIdentifier& b) {}

While this wouldn't tell us exactly which two members aren't equal to each
other, that's OK (and is in scope of reasonable debugging of one who made the
tests fail).

The benefit of this is that we can just use EXPECT_EQ(a, b);

Finally, please have anonymous namespaces at the top of the file.

[johnme, 2015/05/13 13:46:20]

Discussed offline, and we've agreed that it's ok to keep a method with multiple
EXPECT_EQs so failures on trybots etc give enough information to debug the
failure without having to repro locally. But I've renamed the method to
ExpectAppIdentifiersEqual at your suggestion.
Done.

+              const PushMessagingAppIdentifier& b) {
+  EXPECT_EQ(a.app_id(), b.app_id());
+  EXPECT_EQ(a.origin(), b.origin());
+  EXPECT_EQ(a.service_worker_registration_id(),
+            b.service_worker_registration_id());
+}
+
+}  // namespace
+
+TEST_F(PushMessagingAppIdentifierTest, PersistAndDeleteFromPrefs) {
+  base::MessageLoop message_loop;
+  content::TestBrowserThread ui_thread(BrowserThread::UI, &message_loop);

[Peter Beverloo, 2015/05/12 13:55:40]

Since we've already got the PushMessagingAppIdentifierTest base-class in either
case, I'd be OK with just having a |content::TestBrowserThreadBundle| member
that does this for you. Overhead for the two unit tests that don't need it is
minimal.

[johnme, 2015/05/13 13:46:20]

Done.

+  TestingProfile profile;
+
+  PushMessagingAppIdentifier original1 =
+      PushMessagingAppIdentifier::Generate(GURL("https://www.example.com/"), 1);
+  EXPECT_TRUE(PushMessagingAppIdentifier::Get(&profile,
+                                              original1.app_id()).is_null());
+  EXPECT_TRUE(PushMessagingAppIdentifier::Get(&profile, original1.origin(),
+      original1.service_worker_registration_id()).is_null());
+
+  // Test basic PersistToPrefs round trips.

[Peter Beverloo, 2015/05/12 13:55:40]

nit: This can be a separate test.

[johnme, 2015/05/13 13:46:20]

Done.

+  original1.PersistToPrefs(&profile);
+  {
+    PushMessagingAppIdentifier get_app_id_1 =
+        PushMessagingAppIdentifier::Get(&profile, original1.app_id());
+    EXPECT_FALSE(get_app_id_1.is_null());
+    ExpectEq(original1, get_app_id_1);
+  }
+  {
+    PushMessagingAppIdentifier get_origin_and_swr_id_1 =
+        PushMessagingAppIdentifier::Get(&profile,
+            original1.origin(), original1.service_worker_registration_id());
+    EXPECT_FALSE(get_origin_and_swr_id_1.is_null());
+    ExpectEq(original1, get_origin_and_swr_id_1);
+  }
+
+  // Test that PersistToPrefs overwrites (when same origin and Service Worker).

[Peter Beverloo, 2015/05/12 13:55:39]

nit: This can be a separate test.

[johnme, 2015/05/13 13:46:20]

Done.

+  PushMessagingAppIdentifier original2 =
+      PushMessagingAppIdentifier::Generate(GURL("https://www.example.com/"), 1);
+  EXPECT_NE(original1.app_id(), original2.app_id());
+  EXPECT_EQ(original1.origin(), original2.origin());
+  EXPECT_EQ(original1.service_worker_registration_id(),
+            original2.service_worker_registration_id());
+  original2.PersistToPrefs(&profile);
+  {
+    PushMessagingAppIdentifier get_app_id_1 =
+        PushMessagingAppIdentifier::Get(&profile, original1.app_id());
+    EXPECT_TRUE(get_app_id_1.is_null());
+  }
+  {
+    PushMessagingAppIdentifier get_app_id_2 =
+        PushMessagingAppIdentifier::Get(&profile, original2.app_id());
+    EXPECT_FALSE(get_app_id_2.is_null());
+    ExpectEq(original2, get_app_id_2);
+  }
+  {
+    PushMessagingAppIdentifier get_origin_and_swr_id_1 =
+        PushMessagingAppIdentifier::Get(&profile,
+            original1.origin(), original1.service_worker_registration_id());
+    EXPECT_FALSE(get_origin_and_swr_id_1.is_null());
+    ExpectEq(original2, get_origin_and_swr_id_1);
+  }
+
+  // Test that PersistToPrefs doesn't overwrite (when different origin or SW).

[Peter Beverloo, 2015/05/12 13:55:40]

nit: This can be a separate test.

[johnme, 2015/05/13 13:46:20]

Done.

+  PushMessagingAppIdentifier original3 =
+      PushMessagingAppIdentifier::Generate(GURL("https://foo.example.com/"), 1);
+  PushMessagingAppIdentifier original4 =
+      PushMessagingAppIdentifier::Generate(GURL("https://www.example.com/"), 2);
+  EXPECT_NE(original2.app_id(), original3.app_id());
+  EXPECT_NE(original2.app_id(), original4.app_id());
+  original3.PersistToPrefs(&profile);
+  original4.PersistToPrefs(&profile);
+  {
+    PushMessagingAppIdentifier get_app_id_2 =
+        PushMessagingAppIdentifier::Get(&profile, original2.app_id());
+    EXPECT_FALSE(get_app_id_2.is_null());
+    ExpectEq(original2, get_app_id_2);
+  }
+  {
+    PushMessagingAppIdentifier get_origin_and_swr_id_1 =
+        PushMessagingAppIdentifier::Get(&profile,
+            original1.origin(), original1.service_worker_registration_id());
+    EXPECT_FALSE(get_origin_and_swr_id_1.is_null());
+    ExpectEq(original2, get_origin_and_swr_id_1);
+  }
+
+  // Test DeleteFromPrefs. Deleted app identifier should be deleted.

[Peter Beverloo, 2015/05/12 13:55:39]

nit: This can be a separate test.

[johnme, 2015/05/13 13:46:20]

Done.

+  original2.DeleteFromPrefs(&profile);
+  {
+    PushMessagingAppIdentifier get_app_id_2 =
+        PushMessagingAppIdentifier::Get(&profile, original2.app_id());
+    EXPECT_TRUE(get_app_id_2.is_null());
+  }
+  {
+    PushMessagingAppIdentifier get_origin_and_swr_id_1 =
+        PushMessagingAppIdentifier::Get(&profile,
+            original1.origin(), original1.service_worker_registration_id());
+    EXPECT_TRUE(get_origin_and_swr_id_1.is_null());
+  }
+
+  // Test GetAll. Non-deleted app identifiers should all be listed.

[Peter Beverloo, 2015/05/12 13:55:39]

nit: This can be a separate test.

[johnme, 2015/05/13 13:46:20]

Done.

+  std::vector<PushMessagingAppIdentifier> all_app_identifiers =
+      PushMessagingAppIdentifier::GetAll(&profile);
+  EXPECT_EQ(2u, all_app_identifiers.size());
+  // Order is unspecified.
+  bool contained_3 = false;
+  bool contained_4 = false;
+  for (const PushMessagingAppIdentifier& app_identifier : all_app_identifiers) {
+    if (app_identifier.app_id() == original3.app_id()) {
+      ExpectEq(original3, app_identifier);
+      contained_3 = true;
+    } else {
+      ExpectEq(original4, app_identifier);
+      contained_4 = true;
+    }
+  }
+  EXPECT_TRUE(contained_3);
+  EXPECT_TRUE(contained_4);
+}