Add chrome channel to cert logger reports

components/certificate_reporting/error_report_unittest.cc

Index: components/certificate_reporting/error_report_unittest.cc
diff --git a/components/certificate_reporting/error_report_unittest.cc b/components/certificate_reporting/error_report_unittest.cc
index 03667c5b973ed262cbb8d99cb6095a1b4518336a..84f98be7cf32245064959c622b4973037fd63fb6 100644
--- a/components/certificate_reporting/error_report_unittest.cc
+++ b/components/certificate_reporting/error_report_unittest.cc
@@ -18,6 +18,7 @@
 #include "components/certificate_reporting/cert_logger.pb.h"
 #include "components/network_time/network_time_test_utils.h"
 #include "components/prefs/testing_pref_service.h"
+#include "components/version_info/version_info.h"
 #include "net/cert/cert_status_flags.h"
 #include "net/ssl/ssl_info.h"
 #include "net/test/cert_test_util.h"
@@ -247,6 +248,29 @@ TEST(ErrorReportTest, NetworkTimeQueryingFeatureInfo) {
                 .network_time_query_behavior());
 }
 
+TEST(ErrorReportTest, TestChromeChannelIncluded) {
+  base::Thread io_thread("IO thread");

[Lei Zhang, 2017/06/30 22:32:06]

Does your test case need an IO thread?

[sperigo, 2017/06/30 23:37:28]

Done.

[sperigo, 2017/06/30 23:37:29]

I guess not! Thanks.

+  base::Thread::Options thread_options;
+  thread_options.message_loop_type = base::MessageLoop::TYPE_IO;
+  EXPECT_TRUE(io_thread.StartWithOptions(thread_options));
+
+  SSLInfo ssl_info;
+  ASSERT_NO_FATAL_FAILURE(
+      GetTestSSLInfo(INCLUDE_UNVERIFIED_CERT_CHAIN, &ssl_info, kCertStatus));
+  ErrorReport report(kDummyHostname, ssl_info);
+
+  // Add a chrome channel to the report.
+  report.AddChromeChannel(version_info::Channel::DEV);
+  std::string serialized_report;
+  ASSERT_TRUE(report.Serialize(&serialized_report));
+
+  // Check that the parsed report contains the chrome channel information.
+  CertLoggerRequest parsed;
+  ASSERT_TRUE(parsed.ParseFromString(serialized_report));
+  EXPECT_TRUE(parsed.chrome_channel());

[Lei Zhang, 2017/06/30 22:32:06]

It is a bit weird to check chrome_channel() twice. Once as a boolean and once
with the expected enum value.

[sperigo, 2017/06/30 23:37:29]

Done.

+  EXPECT_EQ(CertLoggerRequest::DEV, parsed.chrome_channel());

[meacer, 2017/06/30 22:24:46]

A bit of an overkill but you might want to test all of the enum values here. You
can do that by creating a test struct that contains both the tested value and
the expected value, make an array of all enum values and loop through them.

Something like this:

const struct TestCase {
  version_info::Channel channel;
  CertLoggerRequest::ChromeChannel expected_channel;
} kTestCases[] = {
  {version_info::Channel::UNKNOWN, CertLoggerRequest::UNKNOWN},
  {version_info::Channel::DEV, CertLoggerRequest::DEV},
  ...
}

for (const TestCase& testCase: kTestCases) {
  // Create a report, set its channel value and check
  // if you get back testCase.expected_channel.
}

This would catch the bug with the break statements :)

[sperigo, 2017/06/30 23:37:29]

Done.

[sperigo, 2017/06/30 23:37:29]

I like this! I'm a fan of over testing over under testing.

+}
+
 #if defined(OS_ANDROID)
 // Tests that information about the Android AIA fetching feature is included in
 // the report.