Refactor ChromeFraudulentCertReporter for code reuse by SSL reporting

chrome/browser/net/certificate_error_reporter_unittest.cc

+// Copyright 2015 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/net/certificate_error_reporter.h"
+
+#include "base/files/file_util.h"
+#include "chrome/browser/net/cert_logger.pb.h"
+#include "content/public/test/test_browser_thread.h"
+#include "net/base/test_data_directory.h"
+#include "net/test/cert_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using content::BrowserThread;
+using net::SSLInfo;
+
+namespace chrome_browser_net {
+
+const std::string kHostname = "test.mail.google.com";

[Ryan Sleevi, 2015/03/06 19:23:13]

You should put all of these (including the test) in an unnamed namespace, so
that you don't run into any ODR issues

[estark, 2015/03/06 19:52:06]

If I put the test in an unnamed namespace, I can't FRIEND_TEST_ALL_PREFIXES it,
right?

[estark, 2015/03/06 20:10:02]

Done, except for the test for above reason.

[Ryan Sleevi, 2015/03/06 21:32:46]

Ah, right, because the test is friended you can't.

+
+static SSLInfo GetSSLInfo() {

[Ryan Sleevi, 2015/03/06 19:23:13]

/static/ is not needed here (and perversely, undesirable) once you move it into
an unnamed namespace

[estark, 2015/03/06 20:10:02]

Done.

+  SSLInfo info;
+  info.cert = net::ImportCertFromFile(net::GetTestCertsDirectory(),
+                                      "test_mail_google_com.pem");
+  info.is_issued_by_known_root = true;
+  info.pinning_failure_log = "dummy failure log";
+  return info;
+}
+
+static std::string GetPEMEncodedChain() {
+  base::FilePath cert_path =
+      net::GetTestCertsDirectory().AppendASCII("test_mail_google_com.pem");
+  std::string cert_data;
+  EXPECT_TRUE(base::ReadFileToString(cert_path, &cert_data));
+  return cert_data;
+}
+
+// Test that a built report has the correct data.
+TEST(CertificateErrorReporterTest, ReportIsBuiltCorrectly) {

[Ryan Sleevi, 2015/03/06 19:23:13]

This test is mostly testing the implementation detail, rather than testing the
public API.

That is, it'd be good to have a test that SendReport is working as documented.

For example, if you create a reporter with a URL for an endpoint, does it create
a new URLRequest when you SendReport()? Does it do the right thing if that URL
fails to load? What happens if you have an outstanding request and you delete
the Reporter - does it properly cancel the request?

(Yes, these are all tests that should have been written for the original code)

Because this is refactoring existing code, I'm inclined to give you a short-term
pass on the testing, but it would be great to address these soon.

(Yes, this is the cost of touching code; someone inevitably asks for moar tests
even when you just Want To Land Things)

[estark, 2015/03/06 19:28:18]

I had a question about writing those kinds of tests in the first patch set that
you might have missed:
https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce...

+  SSLInfo info = GetSSLInfo();
+  CertLoggerRequest request;
+  CertificateErrorReporter::BuildReport(kHostname, info, request);
+
+  EXPECT_EQ(request.hostname(), kHostname);
+  EXPECT_EQ(request.cert_chain(), GetPEMEncodedChain());
+  EXPECT_EQ(request.pin().size(), 1);
+  EXPECT_EQ(request.pin().Get(0), "dummy failure log");
+}
+
+}  // namespace chrome_browser_net