Domain Reliability: Don't send proxy address, other fixes

components/domain_reliability/monitor_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 "components/domain_reliability/monitor.h"
 
 #include <map>
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "components/domain_reliability/baked_in_configs.h"
 #include "components/domain_reliability/beacon.h"
 #include "components/domain_reliability/config.h"
 #include "components/domain_reliability/test_util.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "net/base/load_flags.h"
+#include "net/http/http_response_headers.h"
+#include "net/http/http_util.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_status.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace domain_reliability {
 
+namespace {
+
 typedef std::vector<DomainReliabilityBeacon> BeaconVector;
 
+scoped_refptr<net::HttpResponseHeaders> MakeHttpResponseHeaders(
+    const std::string& headers) {
+  return scoped_refptr<net::HttpResponseHeaders>(
+      new net::HttpResponseHeaders(net::HttpUtil::AssembleRawHeaders(
+          headers.c_str(), headers.length())));
+}
+
+static scoped_ptr<const DomainReliabilityConfig> MakeConfig() {
+  DomainReliabilityConfig* config = new DomainReliabilityConfig();
+
+  DomainReliabilityConfig::Resource* resource;
+
+  resource = new DomainReliabilityConfig::Resource();
+  resource->name = "always_report";
+  resource->url_patterns.push_back(
+      new std::string("http://example/always_report"));
+  resource->success_sample_rate = 1.0;
+  resource->failure_sample_rate = 1.0;
+  EXPECT_TRUE(resource->IsValid());
+  config->resources.push_back(resource);
+
+  resource = new DomainReliabilityConfig::Resource();
+  resource->name = "never_report";
+  resource->url_patterns.push_back(
+      new std::string("http://example/never_report"));
+  resource->success_sample_rate = 0.0;
+  resource->failure_sample_rate = 0.0;
+  EXPECT_TRUE(resource->IsValid());
+  config->resources.push_back(resource);
+
+  DomainReliabilityConfig::Collector* collector;
+  collector = new DomainReliabilityConfig::Collector();
+  collector->upload_url = GURL("https://example/upload");
+  EXPECT_TRUE(collector->IsValid());
+  config->collectors.push_back(collector);
+
+  config->version = "1";
+  config->valid_until = 1234567890.0;
+  config->domain = "example";
+  EXPECT_TRUE(config->IsValid());
+
+  return scoped_ptr<const DomainReliabilityConfig>(config);
+}
+
+}  // namespace
+
 class DomainReliabilityMonitorTest : public testing::Test {
  protected:
   typedef DomainReliabilityMonitor::RequestInfo RequestInfo;
 
   DomainReliabilityMonitorTest()
       : bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
         url_request_context_getter_(new net::TestURLRequestContextGetter(
             base::MessageLoopProxy::current())),
         time_(new MockTime()),
         monitor_(url_request_context_getter_->GetURLRequestContext(),
                  "test-reporter",
                  scoped_ptr<MockableTime>(time_)),
-        context_(monitor_.AddContextForTesting(CreateConfig())) {}
+        context_(monitor_.AddContextForTesting(MakeConfig())) {}
 
-  static scoped_ptr<const DomainReliabilityConfig> CreateConfig() {
-    DomainReliabilityConfig* config = new DomainReliabilityConfig();
-
-    DomainReliabilityConfig::Resource* resource;
-
-    resource = new DomainReliabilityConfig::Resource();
-    resource->name = "always_report";
-    resource->url_patterns.push_back(
-        new std::string("http://example/always_report"));
-    resource->success_sample_rate = 1.0;
-    resource->failure_sample_rate = 1.0;
-    EXPECT_TRUE(resource->IsValid());
-    config->resources.push_back(resource);
-
-    resource = new DomainReliabilityConfig::Resource();
-    resource->name = "never_report";
-    resource->url_patterns.push_back(
-        new std::string("http://example/never_report"));
-    resource->success_sample_rate = 0.0;
-    resource->failure_sample_rate = 0.0;
-    EXPECT_TRUE(resource->IsValid());
-    config->resources.push_back(resource);
-
-    DomainReliabilityConfig::Collector* collector;
-    collector = new DomainReliabilityConfig::Collector();
-    collector->upload_url = GURL("https://example/upload");
-    EXPECT_TRUE(collector->IsValid());
-    config->collectors.push_back(collector);
-
-    config->version = "1";
-    config->valid_until = 1234567890.0;
-    config->domain = "example";
-    EXPECT_TRUE(config->IsValid());
-
-    return scoped_ptr<const DomainReliabilityConfig>(config);
-  }
-
-  RequestInfo MakeRequestInfo() {
+  static RequestInfo MakeRequestInfo() {
     RequestInfo request;
     request.status = net::URLRequestStatus();
-    request.response_code = 200;
-    request.was_cached = false;
+    request.status.set_status(net::URLRequestStatus::SUCCESS);
+    request.status.set_error(net::OK);
+    request.response_info.headers = MakeHttpResponseHeaders(
+        "HTTP/1.1 200 OK\n\n");
+    request.response_info.network_accessed = true;
+    request.response_info.was_fetched_via_proxy = false;
     request.load_flags = 0;
     request.is_upload = false;
     return request;
   }
 
   bool CheckNoBeacons(size_t index) {
     BeaconVector beacons;
     unsigned successful, failed;
     context_->GetQueuedDataForTesting(index, &beacons, &successful, &failed);
     return beacons.empty() && successful == 0 && failed == 0;
   }

[Ryan Sleevi, 2014/05/02 23:00:07]

Interestingly, your test for beacons being sent ignores successful/failed

I'm wondering whether this would make more sense if structured as

size_t GetPendingBeacons(size_t index) {
  BeaconVector beacons;
  context_->GetQueuedDataForTesting(index, &beacons, NULL, NULL);
  return beacons.size();
}

And then in your tests, lines like 147-149, 194-196 just become

EXPECT_EQ(1U, GetPendingBeacons(kAlwaysReportIndex));

[Deprecated (see juliatuttle), 2014/05/06 18:52:05]

Done.

 
   void OnRequestLegComplete(const RequestInfo& info) {
     monitor_.OnRequestLegComplete(info);
   }
 
   content::TestBrowserThreadBundle bundle_;
   scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
   MockTime* time_;
   DomainReliabilityMonitor monitor_;
   DomainReliabilityContext* context_;
   DomainReliabilityMonitor::RequestInfo request_;
 };
 
 TEST_F(DomainReliabilityMonitorTest, Create) {
   EXPECT_TRUE(CheckNoBeacons(0));

[Ryan Sleevi, 2014/05/02 23:00:07]

style nit: Having read this file a few times, the (0) and (1) still surprise me.

Any concerns with adding some constants, like
static const size_t kAlwaysReportIndex = 0u;
static const size_t kNeverReportIndex = 1u;

EXPECT_TRUE(CheckNoBeacons(kAlwaysReportIndex));
EXPECT_TRUE(CheckNoBeacons(kNeverReportIndex));

[Deprecated (see juliatuttle), 2014/05/06 18:52:05]

Done.

   EXPECT_TRUE(CheckNoBeacons(1));
 }
 
-TEST_F(DomainReliabilityMonitorTest, NoContextRequest) {
+TEST_F(DomainReliabilityMonitorTest, NoRequest) {
   RequestInfo request = MakeRequestInfo();
   request.url = GURL("http://no-context/");
   OnRequestLegComplete(request);
 
   EXPECT_TRUE(CheckNoBeacons(0));
   EXPECT_TRUE(CheckNoBeacons(1));
 }
 
-TEST_F(DomainReliabilityMonitorTest, ContextRequest) {
+namespace {
+

[Ryan Sleevi, 2014/05/02 23:00:07]

Move this to line 125?

[Deprecated (see juliatuttle), 2014/05/06 18:52:05]

Done.

+TEST_F(DomainReliabilityMonitorTest, Request) {
   RequestInfo request = MakeRequestInfo();
   request.url = GURL("http://example/always_report");
   OnRequestLegComplete(request);
 
   BeaconVector beacons;
   context_->GetQueuedDataForTesting(0, &beacons, NULL, NULL);
   EXPECT_EQ(1u, beacons.size());
   EXPECT_TRUE(CheckNoBeacons(1));
 }
 
-TEST_F(DomainReliabilityMonitorTest, ContextRequestWithDoNotSendCookies) {
+// Make sure the monitor does not log requests that did not access the network.
+TEST_F(DomainReliabilityMonitorTest, DidNotAccessNetwork) {
+  RequestInfo request = MakeRequestInfo();
+  request.url = GURL("http://example/always_report");
+  request.response_info.network_accessed = false;
+  OnRequestLegComplete(request);
+
+  EXPECT_TRUE(CheckNoBeacons(0));
+  EXPECT_TRUE(CheckNoBeacons(1));
+}
+
+// Make sure the monitor does not log requests that don't send cookies.
+TEST_F(DomainReliabilityMonitorTest, DoNotSendCookies) {
   RequestInfo request = MakeRequestInfo();
   request.url = GURL("http://example/always_report");
   request.load_flags = net::LOAD_DO_NOT_SEND_COOKIES;
   OnRequestLegComplete(request);
 
   EXPECT_TRUE(CheckNoBeacons(0));
   EXPECT_TRUE(CheckNoBeacons(1));
 }
 
-TEST_F(DomainReliabilityMonitorTest, ContextRequestThatIsUpload) {
+// Make sure the monitor does not log upload requests.
+TEST_F(DomainReliabilityMonitorTest, IsUpload) {
   RequestInfo request = MakeRequestInfo();
   request.url = GURL("http://example/always_report");
   request.is_upload = true;
   OnRequestLegComplete(request);
 
   EXPECT_TRUE(CheckNoBeacons(0));
   EXPECT_TRUE(CheckNoBeacons(1));
 }
 
+// Make sure the monitor logs a legitimate error.
+TEST_F(DomainReliabilityMonitorTest, ReportedError) {
+  RequestInfo request = MakeRequestInfo();
+  request.url = GURL("http://example/always_report");
+  request.status.set_status(net::URLRequestStatus::FAILED);
+  request.status.set_error(net::ERR_CONNECTION_RESET);
+  OnRequestLegComplete(request);
+
+  BeaconVector beacons;
+  context_->GetQueuedDataForTesting(0, &beacons, NULL, NULL);
+  EXPECT_EQ(1u, beacons.size());
+  EXPECT_TRUE(CheckNoBeacons(1));
+}
+
+// Make sure the monitor does not log a network-local error.
+TEST_F(DomainReliabilityMonitorTest, UnreportedError) {
+  RequestInfo request = MakeRequestInfo();
+  request.url = GURL("http://example/always_report");
+  request.status.set_status(net::URLRequestStatus::FAILED);
+  request.status.set_error(net::ERR_PROXY_CONNECTION_FAILED);
+  OnRequestLegComplete(request);
+
+  EXPECT_TRUE(CheckNoBeacons(0));
+  EXPECT_TRUE(CheckNoBeacons(1));
+}
+
+// Make sure the monitor does not log the proxy's IP if one was used.
+TEST_F(DomainReliabilityMonitorTest, WasFetchedViaProxy) {
+  RequestInfo request = MakeRequestInfo();
+  request.url = GURL("http://example/always_report");
+  request.response_info.was_fetched_via_proxy = true;
+  OnRequestLegComplete(request);
+
+  BeaconVector beacons;
+  context_->GetQueuedDataForTesting(0, &beacons, NULL, NULL);
+  EXPECT_EQ(1u, beacons.size());
+  EXPECT_EQ("", beacons[0].server_ip);

[Ryan Sleevi, 2014/05/02 23:00:07]

AFAICT, you never actually set server_ip, so it's unclear if this test is
passing because you're successful testing, or if it's because you failed to set
server_ip

It would be good if either MakeRequestInfo or your test harness here (214-216)
actually ensured things were configured properly.

[Deprecated (see juliatuttle), 2014/05/06 18:52:05]

Done.

+  EXPECT_TRUE(CheckNoBeacons(1));
+}
+
 TEST_F(DomainReliabilityMonitorTest, AddBakedInConfigs) {
   // AddBakedInConfigs DCHECKs that the baked-in configs parse correctly, so
   // this unittest will fail if someone tries to add an invalid config to the
   // source tree.
   monitor_.AddBakedInConfigs();
 
   // Count the number of baked-in configs.
   size_t num_baked_in_configs = 0;
   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());
 }
 
+}  // namespace
+
 }  // namespace domain_reliability