Domain Reliability: Simplify configs and reports

components/domain_reliability/monitor_unittest.cc

Index: components/domain_reliability/monitor_unittest.cc
diff --git a/components/domain_reliability/monitor_unittest.cc b/components/domain_reliability/monitor_unittest.cc
index ffa284982631979bab17e928db2522526d6d1351..c1f8b858f51394430106e8e0a8d9e1b4dd654b0e 100644
--- a/components/domain_reliability/monitor_unittest.cc
+++ b/components/domain_reliability/monitor_unittest.cc
@@ -14,6 +14,7 @@
 #include "components/domain_reliability/baked_in_configs.h"
 #include "components/domain_reliability/beacon.h"
 #include "components/domain_reliability/config.h"
+#include "components/domain_reliability/google_configs.h"
 #include "components/domain_reliability/test_util.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/load_flags.h"
@@ -28,10 +29,7 @@ namespace domain_reliability {
 
 namespace {
 
-typedef std::vector<DomainReliabilityBeacon> BeaconVector;
-
-static const size_t kAlwaysReportIndex = 0u;
-static const size_t kNeverReportIndex = 1u;
+typedef std::vector<const DomainReliabilityBeacon*> BeaconVector;
 
 scoped_refptr<net::HttpResponseHeaders> MakeHttpResponseHeaders(
     const std::string& headers) {
@@ -40,6 +38,12 @@ scoped_refptr<net::HttpResponseHeaders> MakeHttpResponseHeaders(
           headers.c_str(), headers.length())));
 }
 
+size_t CountQueuedBeacons(DomainReliabilityContext* context) {
+  BeaconVector beacons;
+  context->GetQueuedBeaconsForTesting(&beacons);
+  return beacons.size();
+}
+
 }  // namespace
 
 class DomainReliabilityMonitorTest : public testing::Test {
@@ -55,12 +59,10 @@ class DomainReliabilityMonitorTest : public testing::Test {
         monitor_("test-reporter",
                  pref_task_runner_,
                  network_task_runner_,
-                 scoped_ptr<MockableTime>(time_)),
-        context_(nullptr) {
+                 scoped_ptr<MockableTime>(time_)) {
     monitor_.MoveToNetworkThread();
     monitor_.InitURLRequestContext(url_request_context_getter_);
     monitor_.SetDiscardUploads(false);
-    context_ = monitor_.AddContextForTesting(MakeTestConfig());
   }
 
   static RequestInfo MakeRequestInfo() {
@@ -82,34 +84,16 @@ class DomainReliabilityMonitorTest : public testing::Test {
     monitor_.OnRequestLegComplete(info);
   }
 
-  size_t CountPendingBeacons() {
-    BeaconVector beacons;
-    context_->GetQueuedBeaconsForTesting(&beacons);
-    return beacons.size();
+  DomainReliabilityContext* CreateAndAddContext() {
+    return monitor_.AddContextForTesting(MakeTestConfig());
   }
 
-  bool CheckRequestCounts(size_t index,
-                          uint32 expected_successful,
-                          uint32 expected_failed) {
-    return CheckRequestCounts(context_,
-                              index,
-                              expected_successful,
-                              expected_failed);
-  }
-
-  bool CheckRequestCounts(DomainReliabilityContext* context,
-                          size_t index,
-                          uint32 expected_successful,
-                          uint32 expected_failed) {
-    uint32 successful, failed;
-    context->GetRequestCountsForTesting(index, &successful, &failed);
-    EXPECT_EQ(expected_successful, successful);
-    EXPECT_EQ(expected_failed, failed);
-    return expected_successful == successful && expected_failed == failed;
-  }
-
-  DomainReliabilityContext* CreateAndAddContext(const std::string& domain) {
-    return monitor_.AddContextForTesting(MakeTestConfigWithDomain(domain));
+  DomainReliabilityContext* CreateAndAddContext(const GURL& origin,

[Randy Smith (Not in Mondays), 2015/11/03 21:48:12]

Don't we have a style guide rule against any but the most obvious function
overloads? 
(https://google.github.io/styleguide/cppguide.html#Function_Overloading )

[Deprecated (see juliatuttle), 2015/11/06 17:22:29]

Done.

+                                                bool wildcard) {
+    scoped_ptr<DomainReliabilityConfig> config(
+        MakeTestConfigWithOrigin(origin));
+    config->include_subdomains = wildcard;
+    return monitor_.AddContextForTesting(config.Pass());
   }
 
   scoped_refptr<base::TestSimpleTaskRunner> pref_task_runner_;
@@ -117,169 +101,151 @@ class DomainReliabilityMonitorTest : public testing::Test {
   scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
   MockTime* time_;
   DomainReliabilityMonitor monitor_;
-  DomainReliabilityContext* context_;
   DomainReliabilityMonitor::RequestInfo request_;
 };
 
 namespace {
 
 TEST_F(DomainReliabilityMonitorTest, Create) {
-  EXPECT_EQ(0u, CountPendingBeacons());
-  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 0u));
-  EXPECT_TRUE(CheckRequestCounts(kNeverReportIndex, 0u, 0u));
 }
 
 TEST_F(DomainReliabilityMonitorTest, NoContext) {
-  RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://no-context/");
-  OnRequestLegComplete(request);
-
-  EXPECT_EQ(0u, CountPendingBeacons());
-  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 0u));
-  EXPECT_TRUE(CheckRequestCounts(kNeverReportIndex, 0u, 0u));
-}
+  DomainReliabilityContext* context = CreateAndAddContext();
 
-TEST_F(DomainReliabilityMonitorTest, NotReported) {

[Randy Smith (Not in Mondays), 2015/11/03 21:48:12]

Hmmm.  I'm sure it's my lack of completely groking this CL, but why is this test
no longer useful?

[Deprecated (see juliatuttle), 2015/11/06 17:22:29]

It's testing a resource with 0 failure_sample_rate, but we don't have resources
anymore.

[Randy Smith (Not in Mondays), 2015/11/09 21:23:24]

Acknowledged.

   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://example/never_report");
+  request.url = GURL("http://no-context/");
   OnRequestLegComplete(request);
 
-  EXPECT_EQ(0u, CountPendingBeacons());
-  EXPECT_TRUE(CheckRequestCounts(kNeverReportIndex, 1u, 0u));
+  EXPECT_EQ(0u, CountQueuedBeacons(context));
 }
 
 TEST_F(DomainReliabilityMonitorTest, NetworkFailure) {
+  DomainReliabilityContext* context = CreateAndAddContext();
+
   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://example/always_report");
+  request.url = GURL("http://example/");
   request.status = net::URLRequestStatus::FromError(net::ERR_CONNECTION_RESET);
   request.response_info.headers = nullptr;
   OnRequestLegComplete(request);
 
-  EXPECT_EQ(1u, CountPendingBeacons());
-  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 1u));
+  EXPECT_EQ(1u, CountQueuedBeacons(context));
 }
 
 TEST_F(DomainReliabilityMonitorTest, ServerFailure) {
+  DomainReliabilityContext* context = CreateAndAddContext();
+
   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://example/always_report");
+  request.url = GURL("http://example/");
   request.response_info.headers =
       MakeHttpResponseHeaders("HTTP/1.1 500 :(\n\n");
   OnRequestLegComplete(request);
 
-  EXPECT_EQ(1u, CountPendingBeacons());
-  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 1u));
-}
-
-TEST_F(DomainReliabilityMonitorTest, NotReportedFailure) {
-  RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://example/never_report");
-  request.status = net::URLRequestStatus::FromError(net::ERR_CONNECTION_RESET);
-  OnRequestLegComplete(request);
-
-  EXPECT_EQ(0u, CountPendingBeacons());
-  EXPECT_TRUE(CheckRequestCounts(kNeverReportIndex, 0u, 1u));
-}
-
-TEST_F(DomainReliabilityMonitorTest, Request) {
-  RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://example/always_report");
-  OnRequestLegComplete(request);
-
-  EXPECT_EQ(1u, CountPendingBeacons());
-  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 1u, 0u));
+  EXPECT_EQ(1u, CountQueuedBeacons(context));
 }
 
 // Make sure the monitor does not log requests that did not access the network.
 TEST_F(DomainReliabilityMonitorTest, DidNotAccessNetwork) {
+  DomainReliabilityContext* context = CreateAndAddContext();
+
   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://example/always_report");
+  request.url = GURL("http://example/");
   request.response_info.network_accessed = false;
   OnRequestLegComplete(request);
 
-  EXPECT_EQ(0u, CountPendingBeacons());
-  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 0u));
+  EXPECT_EQ(0u, CountQueuedBeacons(context));
 }
 
 // Make sure the monitor does not log requests that don't send cookies.
 TEST_F(DomainReliabilityMonitorTest, DoNotSendCookies) {
+  DomainReliabilityContext* context = CreateAndAddContext();
+
   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://example/always_report");
+  request.url = GURL("http://example/");
   request.load_flags = net::LOAD_DO_NOT_SEND_COOKIES;
   OnRequestLegComplete(request);
 
-  EXPECT_EQ(0u, CountPendingBeacons());
-  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 0u));
+  EXPECT_EQ(0u, CountQueuedBeacons(context));
 }
 
 // Make sure the monitor does not log upload requests.
 TEST_F(DomainReliabilityMonitorTest, IsUpload) {
+  DomainReliabilityContext* context = CreateAndAddContext();
+
   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://example/always_report");
+  request.url = GURL("http://example/");
   request.is_upload = true;
   OnRequestLegComplete(request);
 
-  EXPECT_EQ(0u, CountPendingBeacons());
-  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 0u));
+  EXPECT_EQ(0u, CountQueuedBeacons(context));
 }
 
 // Make sure the monitor does not log a network-local error.
 TEST_F(DomainReliabilityMonitorTest, LocalError) {
+  DomainReliabilityContext* context = CreateAndAddContext();
+
   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://example/always_report");
+  request.url = GURL("http://example/");
   request.status =
       net::URLRequestStatus::FromError(net::ERR_PROXY_CONNECTION_FAILED);
   OnRequestLegComplete(request);
 
-  EXPECT_EQ(0u, CountPendingBeacons());
-  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 0u));
+  EXPECT_EQ(0u, CountQueuedBeacons(context));
 }
 
 // Make sure the monitor does not log the proxy's IP if one was used.
 TEST_F(DomainReliabilityMonitorTest, WasFetchedViaProxy) {
+  DomainReliabilityContext* context = CreateAndAddContext();
+
   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://example/always_report");
+  request.url = GURL("http://example/");
+  request.status = net::URLRequestStatus::FromError(net::ERR_CONNECTION_RESET);
   request.response_info.socket_address =
       net::HostPortPair::FromString("127.0.0.1:3128");
   request.response_info.was_fetched_via_proxy = true;
   OnRequestLegComplete(request);
 
   BeaconVector beacons;
-  context_->GetQueuedBeaconsForTesting(&beacons);
+  context->GetQueuedBeaconsForTesting(&beacons);
   EXPECT_EQ(1u, beacons.size());
-  EXPECT_TRUE(beacons[0].server_ip.empty());
-
-  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 1u, 0u));
+  EXPECT_TRUE(beacons[0]->server_ip.empty());
 }
 
 // Make sure the monitor does not log the cached IP returned after a successful
 // cache revalidation request.
 TEST_F(DomainReliabilityMonitorTest,
        NoCachedIPFromSuccessfulRevalidationRequest) {
+  scoped_ptr<DomainReliabilityConfig> config = MakeTestConfig();
+  config->success_sample_rate = 1.0;
+  DomainReliabilityContext* context =
+      monitor_.AddContextForTesting(config.Pass());
+
   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://example/always_report");
+  request.url = GURL("http://example/");
   request.response_info.was_cached = true;
   OnRequestLegComplete(request);
 
   BeaconVector beacons;
-  context_->GetQueuedBeaconsForTesting(&beacons);
+  context->GetQueuedBeaconsForTesting(&beacons);
   EXPECT_EQ(1u, beacons.size());
-  EXPECT_TRUE(beacons[0].server_ip.empty());
+  EXPECT_TRUE(beacons[0]->server_ip.empty());
 }
 
 // Make sure the monitor does not log the cached IP returned with a failed
 // cache revalidation request.
 TEST_F(DomainReliabilityMonitorTest, NoCachedIPFromFailedRevalidationRequest) {
+  DomainReliabilityContext* context = CreateAndAddContext();
+
   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://example/always_report");
+  request.url = GURL("http://example/");
   request.response_info.was_cached = true;
   request.status =
       net::URLRequestStatus::FromError(net::ERR_NAME_RESOLUTION_FAILED);
   OnRequestLegComplete(request);
 
   BeaconVector beacons;
-  context_->GetQueuedBeaconsForTesting(&beacons);
+  context->GetQueuedBeaconsForTesting(&beacons);
   EXPECT_EQ(1u, beacons.size());
-  EXPECT_TRUE(beacons[0].server_ip.empty());
+  EXPECT_TRUE(beacons[0]->server_ip.empty());
 }
 
 TEST_F(DomainReliabilityMonitorTest, AtLeastOneBakedInConfig) {
@@ -299,37 +265,43 @@ TEST_F(DomainReliabilityMonitorTest, AddBakedInConfigs) {
   for (const char* const* p = kBakedInJsonConfigs; *p; ++p)
     ++num_baked_in_configs;
 
-  // The monitor should have contexts for all of the baked-in configs, plus the
-  // test one added in the test constructor.
-  EXPECT_EQ(num_baked_in_configs + 1, monitor_.contexts_size_for_testing());
+  // Also count the Google configs stored in abbreviated form.
+  std::vector<DomainReliabilityConfig*> google_configs;
+  GetAllGoogleConfigs(&google_configs);
+  size_t num_google_configs = google_configs.size();
+  STLDeleteElements(&google_configs);
+
+  // The monitor should have contexts for all of the baked-in configs.
+  EXPECT_EQ(num_baked_in_configs + num_google_configs,
+            monitor_.contexts_size_for_testing());
 }
 
 TEST_F(DomainReliabilityMonitorTest, ClearBeacons) {
+  DomainReliabilityContext* context = CreateAndAddContext();
+
   // Initially the monitor should have just the test context, with no beacons.
   EXPECT_EQ(1u, monitor_.contexts_size_for_testing());
-  EXPECT_EQ(0u, CountPendingBeacons());
-  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 0u));
-  EXPECT_TRUE(CheckRequestCounts(kNeverReportIndex, 0u, 0u));
+  EXPECT_EQ(0u, CountQueuedBeacons(context));
 
   // Add a beacon.
   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://example/always_report");
+  request.url = GURL("http://example/");
+  request.status = net::URLRequestStatus::FromError(net::ERR_CONNECTION_RESET);
   OnRequestLegComplete(request);
 
   // Make sure it was added.
-  EXPECT_EQ(1u, CountPendingBeacons());
-  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 1u, 0u));
+  EXPECT_EQ(1u, CountQueuedBeacons(context));
 
   monitor_.ClearBrowsingData(CLEAR_BEACONS);
 
   // Make sure the beacon was cleared, but not the contexts.
   EXPECT_EQ(1u, monitor_.contexts_size_for_testing());
-  EXPECT_EQ(0u, CountPendingBeacons());
-  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 0u));
-  EXPECT_TRUE(CheckRequestCounts(kNeverReportIndex, 0u, 0u));
+  EXPECT_EQ(0u, CountQueuedBeacons(context));
 }
 
 TEST_F(DomainReliabilityMonitorTest, ClearContexts) {
+  CreateAndAddContext();
+
   // Initially the monitor should have just the test context.
   EXPECT_EQ(1u, monitor_.contexts_size_for_testing());
 
@@ -340,67 +312,85 @@ TEST_F(DomainReliabilityMonitorTest, ClearContexts) {
 }
 
 TEST_F(DomainReliabilityMonitorTest, WildcardMatchesSelf) {
-  DomainReliabilityContext* context = CreateAndAddContext("*.wildcard");
+  DomainReliabilityContext* context =
+      CreateAndAddContext(GURL("https://wildcard/"), true);
 
   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://wildcard/always_report");
+  request.url = GURL("http://wildcard/");
+  request.status = net::URLRequestStatus::FromError(net::ERR_CONNECTION_RESET);
   OnRequestLegComplete(request);
-  EXPECT_TRUE(CheckRequestCounts(context, kAlwaysReportIndex, 1u, 0u));
+
+  EXPECT_EQ(1u, CountQueuedBeacons(context));
 }
 
 TEST_F(DomainReliabilityMonitorTest, WildcardMatchesSubdomain) {
-  DomainReliabilityContext* context = CreateAndAddContext("*.wildcard");
+  DomainReliabilityContext* context =
+      CreateAndAddContext(GURL("https://wildcard/"), true);
 
   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://test.wildcard/always_report");
+  request.url = GURL("http://test.wildcard/");
+  request.status = net::URLRequestStatus::FromError(net::ERR_CONNECTION_RESET);
   OnRequestLegComplete(request);
-  EXPECT_TRUE(CheckRequestCounts(context, kAlwaysReportIndex, 1u, 0u));
+
+  EXPECT_EQ(1u, CountQueuedBeacons(context));
 }
 
 TEST_F(DomainReliabilityMonitorTest, WildcardDoesntMatchSubsubdomain) {
-  DomainReliabilityContext* context = CreateAndAddContext("*.wildcard");
+  DomainReliabilityContext* context =
+      CreateAndAddContext(GURL("https://wildcard/"), true);
 
   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://test.test.wildcard/always_report");
+  request.url = GURL("http://test.test.wildcard/");
+  request.status = net::URLRequestStatus::FromError(net::ERR_CONNECTION_RESET);
   OnRequestLegComplete(request);
-  EXPECT_TRUE(CheckRequestCounts(context, kAlwaysReportIndex, 0u, 0u));
+
+  EXPECT_EQ(0u, CountQueuedBeacons(context));
 }
 
 TEST_F(DomainReliabilityMonitorTest, WildcardPrefersSelfToSelfWildcard) {
-  DomainReliabilityContext* context1 = CreateAndAddContext("wildcard");
-  DomainReliabilityContext* context2 = CreateAndAddContext("*.wildcard");
+  DomainReliabilityContext* context1 =
+      CreateAndAddContext(GURL("https://wildcard/"), false);
+  DomainReliabilityContext* context2 =
+      CreateAndAddContext(GURL("https://wildcard/"), true);
 
   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://wildcard/always_report");
+  request.url = GURL("http://wildcard/");
+  request.status = net::URLRequestStatus::FromError(net::ERR_CONNECTION_RESET);
   OnRequestLegComplete(request);
 
-  EXPECT_TRUE(CheckRequestCounts(context1, kAlwaysReportIndex, 1u, 0u));
-  EXPECT_TRUE(CheckRequestCounts(context2, kAlwaysReportIndex, 0u, 0u));
+  EXPECT_EQ(1u, CountQueuedBeacons(context1));
+  EXPECT_EQ(0u, CountQueuedBeacons(context2));
 }
 
 TEST_F(DomainReliabilityMonitorTest, WildcardPrefersSelfToParentWildcard) {
-  DomainReliabilityContext* context1 = CreateAndAddContext("test.wildcard");
-  DomainReliabilityContext* context2 = CreateAndAddContext("*.wildcard");
+  DomainReliabilityContext* context1 =
+      CreateAndAddContext(GURL("https://test.wildcard/"), false);
+  DomainReliabilityContext* context2 =
+      CreateAndAddContext(GURL("https://wildcard/"), true);
 
   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://test.wildcard/always_report");
+  request.url = GURL("http://test.wildcard/");
+  request.status = net::URLRequestStatus::FromError(net::ERR_CONNECTION_RESET);
   OnRequestLegComplete(request);
 
-  EXPECT_TRUE(CheckRequestCounts(context1, kAlwaysReportIndex, 1u, 0u));
-  EXPECT_TRUE(CheckRequestCounts(context2, kAlwaysReportIndex, 0u, 0u));
+  EXPECT_EQ(1u, CountQueuedBeacons(context1));
+  EXPECT_EQ(0u, CountQueuedBeacons(context2));
 }
 
 TEST_F(DomainReliabilityMonitorTest,
     WildcardPrefersSelfWildcardToParentWildcard) {
-  DomainReliabilityContext* context1 = CreateAndAddContext("*.test.wildcard");
-  DomainReliabilityContext* context2 = CreateAndAddContext("*.wildcard");
+  DomainReliabilityContext* context1 =
+      CreateAndAddContext(GURL("https://test.wildcard/"), true);
+  DomainReliabilityContext* context2 =
+      CreateAndAddContext(GURL("https://wildcard/"), true);
 
   RequestInfo request = MakeRequestInfo();
-  request.url = GURL("http://test.wildcard/always_report");
+  request.url = GURL("http://test.wildcard/");
+  request.status = net::URLRequestStatus::FromError(net::ERR_CONNECTION_RESET);
   OnRequestLegComplete(request);
 
-  EXPECT_TRUE(CheckRequestCounts(context1, kAlwaysReportIndex, 1u, 0u));
-  EXPECT_TRUE(CheckRequestCounts(context2, kAlwaysReportIndex, 0u, 0u));
+  EXPECT_EQ(1u, CountQueuedBeacons(context1));
+  EXPECT_EQ(0u, CountQueuedBeacons(context2));
 }
 
 }  // namespace