Add tests for PushMessagingAppIdentifier

chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc

 // 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 "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(
       const GURL& origin,
       int64_t service_worker_registration_id) {
     // To bypass DCHECK in PushMessagingAppIdentifier::Generate, we just use it
     // to generate app_id, and then use private constructor.
     std::string app_id = PushMessagingAppIdentifier::Generate(
         GURL("https://www.example.com/"), 1).app_id();
     return PushMessagingAppIdentifier(app_id, origin,
@@ skipping 13 matching lines @@
   // The following one is invalid and will DCHECK in Generate and be null:
   EXPECT_TRUE(GenerateId(GURL("https://www.example.com/"), -1).is_null());
 }
 
 TEST_F(PushMessagingAppIdentifierTest, UniqueGuids) {
   EXPECT_NE(PushMessagingAppIdentifier::Generate(
                 GURL("https://www.example.com/"), 1).app_id(),
             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);
+}