Address NetworkIsolationKey::Todo() in MockPersistentNelStore tests.

Also remove MockPersistentNelStore::GetDebugString(). It's only used
in a single LOG statement in a single test.

Bug: 993805
Change-Id: I03e52c5a327d6ec1c9e7c6913e3869a106664202
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2505939
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#821902}

net/network_error_logging/mock_persistent_nel_store.cc

@@ -132,33 +132,4 @@ MockPersistentNelStore::CommandList MockPersistentNelStore::GetAllCommands()
   return command_list_;
 }
 
-std::string MockPersistentNelStore::GetDebugString() const {
-  std::ostringstream s;
-
-  for (const Command& command : command_list_) {
-    switch (command.type) {
-      case Command::Type::LOAD_NEL_POLICIES:
-        s << "LOAD; ";
-        break;
-      case Command::Type::ADD_NEL_POLICY:
-        s << "ADD(" << command.key.network_isolation_key.ToDebugString() << ", "
-          << command.key.origin.Serialize() << "); ";
-        break;
-      case Command::Type::UPDATE_NEL_POLICY:
-        s << "UPDATE(" << command.key.network_isolation_key.ToDebugString()
-          << ", " << command.key.origin.Serialize() << "); ";
-        break;
-      case Command::Type::DELETE_NEL_POLICY:
-        s << "DELETE(" << command.key.network_isolation_key.ToDebugString()
-          << ", " << command.key.origin.Serialize() << "); ";
-        break;
-      case Command::Type::FLUSH:
-        s << "FLUSH; ";
-        break;
-    }
-  }
-
-  return s.str();
-}
-
 }  // namespace net

net/network_error_logging/mock_persistent_nel_store.h

@@ -94,9 +94,6 @@ class MockPersistentNelStore
   // this were a real store.
   int StoredPoliciesCount() const { return policy_count_; }
 
-  // Generates a string with the list of commands, for ease of debugging.
-  std::string GetDebugString() const;
-
  private:
   // List of commands that we have received so far.
   CommandList command_list_;

net/network_error_logging/mock_persistent_nel_store_unittest.cc

@@ -7,6 +7,7 @@
 #include "base/location.h"
 #include "base/strings/strcat.h"
 #include "base/test/bind_test_util.h"
+#include "net/base/network_isolation_key.h"
 #include "net/network_error_logging/mock_persistent_nel_store.h"
 #include "net/network_error_logging/network_error_logging_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -17,15 +18,12 @@ namespace net {
 
 namespace {
 
-// Serialized string of NetworkIsolationKey::Todo() which is used in
-// NelPolicyKeys for now.
-const char kTodoNikString[] = "null null";
-
-// TODO(chlily): Populate NIK.
-NetworkErrorLoggingService::NelPolicy MakePolicyForOrigin(url::Origin origin) {
+NetworkErrorLoggingService::NelPolicy MakePolicy(
+    const url::Origin& origin,
+    const net::NetworkIsolationKey& network_isolation_key) {
   NetworkErrorLoggingService::NelPolicy policy;
-  policy.key = NetworkErrorLoggingService::NelPolicyKey(
-      NetworkIsolationKey::Todo(), origin);
+  policy.key =
+      NetworkErrorLoggingService::NelPolicyKey(network_isolation_key, origin);
   policy.expires = base::Time();
   policy.last_used = base::Time();
 
@@ -50,8 +48,23 @@ MakeExpectedRunNelPoliciesLoadedCallback(
                         policies_out);
 }
 
+class MockPersistentNelStoreTest : public testing::Test {
+ public:
+  MockPersistentNelStoreTest() = default;
+  ~MockPersistentNelStoreTest() override = default;
+
+ protected:
+  const url::Origin origin_ =
+      url::Origin::Create(GURL("https://example.test/"));
+  const NetworkIsolationKey network_isolation_key_ =
+      NetworkIsolationKey(url::Origin::Create(GURL("https://foo.test/")),
+                          url::Origin::Create(GURL("https://bar.test/")));
+  const NetworkErrorLoggingService::NelPolicy nel_policy_ =
+      MakePolicy(origin_, network_isolation_key_);
+};
+
 // Test that FinishLoading() runs the callback.
-TEST(MockPersistentNelStoreTest, FinishLoading) {
+TEST_F(MockPersistentNelStoreTest, FinishLoading) {
   MockPersistentNelStore store;
   MockPersistentNelStore::CommandList expected_commands;
   std::vector<NetworkErrorLoggingService::NelPolicy> loaded_policies;
@@ -66,13 +79,12 @@ TEST(MockPersistentNelStoreTest, FinishLoading) {
 
   EXPECT_EQ(1u, store.GetAllCommands().size());
   EXPECT_TRUE(store.VerifyCommands(expected_commands));
-  EXPECT_EQ("LOAD; ", store.GetDebugString());
 
   // Test should not crash because the callback has been run.
 }
 
-TEST(MockPersistentNelStoreTest, PreStoredPolicies) {
-  const url::Origin kOrigin =
+TEST_F(MockPersistentNelStoreTest, PreStoredPolicies) {
+  const url::Origin origin_ =
       url::Origin::Create(GURL("https://example.test/"));
 
   MockPersistentNelStore store;
@@ -80,7 +92,7 @@ TEST(MockPersistentNelStoreTest, PreStoredPolicies) {
   std::vector<NetworkErrorLoggingService::NelPolicy> loaded_policies;
 
   std::vector<NetworkErrorLoggingService::NelPolicy> prestored_policies = {
-      MakePolicyForOrigin(kOrigin)};
+      nel_policy_};
   store.SetPrestoredPolicies(std::move(prestored_policies));
   EXPECT_EQ(1, store.StoredPoliciesCount());
 
@@ -90,21 +102,22 @@ TEST(MockPersistentNelStoreTest, PreStoredPolicies) {
       MockPersistentNelStore::Command::Type::LOAD_NEL_POLICIES);
   store.FinishLoading(true /* load_success */);
   ASSERT_EQ(1u, loaded_policies.size());
-  EXPECT_EQ(kOrigin, loaded_policies[0].key.origin);
+  EXPECT_EQ(origin_, loaded_policies[0].key.origin);
+  EXPECT_EQ(network_isolation_key_,
+            loaded_policies[0].key.network_isolation_key);
 
   EXPECT_EQ(1u, store.GetAllCommands().size());
   EXPECT_TRUE(store.VerifyCommands(expected_commands));
-  EXPECT_EQ("LOAD; ", store.GetDebugString());
 }
 
 // Failed load should yield empty vector of policies.
-TEST(MockPersistentNelStoreTest, FailedLoad) {
+TEST_F(MockPersistentNelStoreTest, FailedLoad) {
   MockPersistentNelStore store;
   MockPersistentNelStore::CommandList expected_commands;
   std::vector<NetworkErrorLoggingService::NelPolicy> loaded_policies;
 
   std::vector<NetworkErrorLoggingService::NelPolicy> prestored_policies = {
-      MakePolicyForOrigin(url::Origin::Create(GURL("https://example.test/")))};
+      nel_policy_};
   store.SetPrestoredPolicies(std::move(prestored_policies));
   EXPECT_EQ(1, store.StoredPoliciesCount());
 
@@ -118,13 +131,9 @@ TEST(MockPersistentNelStoreTest, FailedLoad) {
 
   EXPECT_EQ(1u, store.GetAllCommands().size());
   EXPECT_TRUE(store.VerifyCommands(expected_commands));
-  EXPECT_EQ("LOAD; ", store.GetDebugString());
 }
 
-TEST(MockPersistentNelStoreTest, Add) {
-  const url::Origin kOrigin =
-      url::Origin::Create(GURL("https://example.test/"));
-
+TEST_F(MockPersistentNelStoreTest, Add) {
   MockPersistentNelStore store;
   MockPersistentNelStore::CommandList expected_commands;
   std::vector<NetworkErrorLoggingService::NelPolicy> loaded_policies;
@@ -138,7 +147,7 @@ TEST(MockPersistentNelStoreTest, Add) {
   store.FinishLoading(true /* load_success */);
   EXPECT_EQ(0u, loaded_policies.size());
 
-  NetworkErrorLoggingService::NelPolicy policy = MakePolicyForOrigin(kOrigin);
+  NetworkErrorLoggingService::NelPolicy policy = nel_policy_;
   store.AddNelPolicy(policy);
   expected_commands.emplace_back(
       MockPersistentNelStore::Command::Type::ADD_NEL_POLICY, policy);
@@ -152,15 +161,9 @@ TEST(MockPersistentNelStoreTest, Add) {
 
   EXPECT_EQ(3u, store.GetAllCommands().size());
   EXPECT_TRUE(store.VerifyCommands(expected_commands));
-  EXPECT_EQ(base::StrCat({"LOAD; ADD(", kTodoNikString, ", ",
-                          kOrigin.Serialize(), "); FLUSH; "}),
-            store.GetDebugString());
 }
 
-TEST(MockPersistentNelStoreTest, AddThenDelete) {
-  const url::Origin kOrigin =
-      url::Origin::Create(GURL("https://example.test/"));
-
+TEST_F(MockPersistentNelStoreTest, AddThenDelete) {
   MockPersistentNelStore store;
   MockPersistentNelStore::CommandList expected_commands;
   std::vector<NetworkErrorLoggingService::NelPolicy> loaded_policies;
@@ -174,7 +177,7 @@ TEST(MockPersistentNelStoreTest, AddThenDelete) {
   store.FinishLoading(true /* load_success */);
   EXPECT_EQ(0u, loaded_policies.size());
 
-  NetworkErrorLoggingService::NelPolicy policy = MakePolicyForOrigin(kOrigin);
+  NetworkErrorLoggingService::NelPolicy policy = nel_policy_;
   store.AddNelPolicy(policy);
   expected_commands.emplace_back(
       MockPersistentNelStore::Command::Type::ADD_NEL_POLICY, policy);
@@ -191,16 +194,9 @@ TEST(MockPersistentNelStoreTest, AddThenDelete) {
   EXPECT_EQ(4u, store.GetAllCommands().size());
 
   EXPECT_TRUE(store.VerifyCommands(expected_commands));
-  EXPECT_EQ(base::StrCat({"LOAD; ADD(", kTodoNikString, ", ",
-                          kOrigin.Serialize(), "); DELETE(", kTodoNikString,
-                          ", ", kOrigin.Serialize(), "); FLUSH; "}),
-            store.GetDebugString());
 }
 
-TEST(MockPersistentNelStoreTest, AddFlushThenDelete) {
-  const url::Origin kOrigin =
-      url::Origin::Create(GURL("https://example.test/"));
-
+TEST_F(MockPersistentNelStoreTest, AddFlushThenDelete) {
   MockPersistentNelStore store;
   MockPersistentNelStore::CommandList expected_commands;
   std::vector<NetworkErrorLoggingService::NelPolicy> loaded_policies;
@@ -214,7 +210,7 @@ TEST(MockPersistentNelStoreTest, AddFlushThenDelete) {
   store.FinishLoading(true /* load_success */);
   EXPECT_EQ(0u, loaded_policies.size());
 
-  NetworkErrorLoggingService::NelPolicy policy = MakePolicyForOrigin(kOrigin);
+  NetworkErrorLoggingService::NelPolicy policy = nel_policy_;
   store.AddNelPolicy(policy);
   expected_commands.emplace_back(
       MockPersistentNelStore::Command::Type::ADD_NEL_POLICY, policy);
@@ -236,17 +232,9 @@ TEST(MockPersistentNelStoreTest, AddFlushThenDelete) {
   EXPECT_EQ(5u, store.GetAllCommands().size());
 
   EXPECT_TRUE(store.VerifyCommands(expected_commands));
-  EXPECT_EQ(
-      base::StrCat({"LOAD; ADD(", kTodoNikString, ", ", kOrigin.Serialize(),
-                    "); FLUSH; DELETE(", kTodoNikString, ", ",
-                    kOrigin.Serialize(), "); FLUSH; "}),
-      store.GetDebugString());
 }
 
-TEST(MockPersistentNelStoreTest, AddThenUpdate) {
-  const url::Origin kOrigin =
-      url::Origin::Create(GURL("https://example.test/"));
-
+TEST_F(MockPersistentNelStoreTest, AddThenUpdate) {
   MockPersistentNelStore store;
   MockPersistentNelStore::CommandList expected_commands;
   std::vector<NetworkErrorLoggingService::NelPolicy> loaded_policies;
@@ -259,7 +247,7 @@ TEST(MockPersistentNelStoreTest, AddThenUpdate) {
 
   store.FinishLoading(true /* load_success */);
 
-  NetworkErrorLoggingService::NelPolicy policy = MakePolicyForOrigin(kOrigin);
+  NetworkErrorLoggingService::NelPolicy policy = nel_policy_;
   store.AddNelPolicy(policy);
   expected_commands.emplace_back(
       MockPersistentNelStore::Command::Type::ADD_NEL_POLICY, policy);
@@ -276,10 +264,6 @@ TEST(MockPersistentNelStoreTest, AddThenUpdate) {
   EXPECT_EQ(4u, store.GetAllCommands().size());
 
   EXPECT_TRUE(store.VerifyCommands(expected_commands));
-  EXPECT_EQ(base::StrCat({"LOAD; ADD(", kTodoNikString, ", ",
-                          kOrigin.Serialize(), "); UPDATE(", kTodoNikString,
-                          ", ", kOrigin.Serialize(), "); FLUSH; "}),
-            store.GetDebugString());
 }
 
 }  // namespace

net/network_error_logging/network_error_logging_service_unittest.cc

@@ -1604,7 +1604,6 @@ TEST_P(NetworkErrorLoggingServiceTest,
   expected_commands.emplace_back(
       MockPersistentNelStore::Command::Type::ADD_NEL_POLICY, policy1);
   EXPECT_TRUE(store()->VerifyCommands(expected_commands));
-  LOG(INFO) << store()->GetDebugString();
 
   service()->OnRequest(
       MakeRequestDetails(kNik_, kOrigin_.GetURL(), ERR_CONNECTION_REFUSED));