Minor improvements to MockLogDnsTraffic

components/certificate_transparency/mock_log_dns_traffic.h

Index: components/certificate_transparency/mock_log_dns_traffic.h
diff --git a/components/certificate_transparency/mock_log_dns_traffic.h b/components/certificate_transparency/mock_log_dns_traffic.h
index f91e66824de64b4a0b1d63914ca54da1977eb80a..d5965b5024e811109e7900a5a1fb165a56b30d55 100644
--- a/components/certificate_transparency/mock_log_dns_traffic.h
+++ b/components/certificate_transparency/mock_log_dns_traffic.h
@@ -73,10 +73,33 @@ class MockSocketData {
 
 // Mocks DNS requests and responses for a Certificate Transparency (CT) log.
 // This is implemented using mock sockets. Call the CreateDnsClient() method to
 // get a net::DnsClient wired up to these mock sockets.
 // The Expect*() methods must be called from within a GTest test case.
+//
+// Example Usage:
+// // Create a mock NetworkChangeNotifier to propogate DNS config.

[Ryan Sleevi, 2016/09/23 21:29:47]

spelling: propagate

[Rob Percival, 2016/10/03 13:35:45]

Done.

+// std::unique_ptr<net::NetworkChangeNotifier> net_change_notifier =
+//     net::NetworkChangeNotifier::CreateMock();

[Ryan Sleevi, 2016/09/23 21:29:47]

pedantic nit: ( ) over = here since CreateMock returns a raw pointer

[Rob Percival, 2016/10/03 13:35:45]

Done.

+//
+// // net::DnsClient requires an I/O message loop for async operations.
+// base::MessageLoopForIO message_loop;
+//
+// MockLogDnsTraffic mock_dns;

[Ryan Sleevi, 2016/09/23 21:29:47]

This is a fake, not a mock.

[Rob Percival, 2016/10/03 13:35:45]

Based on most definitions I can find, it seems more like a mock. You set
expected inputs and it verifies that they are provided, then passes back some
predefined output.

+// mock_dns.InitializeDnsConfig();
+// // Use the Expect* methods to define expected DNS requests and responses.

[Ryan Sleevi, 2016/09/23 21:29:47]

Naming wise, this feels uncomfortable with //net paradigms, and suggests a
pattern that is like GTest/Gmock, but very much isn't. That is, it doesn't setup
any test expectations, AFAICT.

The use of EXPECT_TRUE in mock_log_dns_traffic.cc is something I would have
flagged and pushed back on; I believe that EXPECT/ASSERT should be used in
tests, not test helpers.

[Rob Percival, 2016/10/03 13:35:45]

It actually does setup test expectations, if you look deep inside
socket_test_util.cc
(https://cs.chromium.org/chromium/src/net/socket/socket_test_util.cc?rcl=14752...).
That's probably something you'd have pushed back on equally hard though. I'm
happy to change the EXPECT/ASSERTs in mock_log_dns_traffic.cc into DCHECKs, as
they're really only things that'll happen as the result of setting up
expectations incorrectly (e.g. expected inputs/outputs that can't fit into a
packet).

+// mock_dns.ExpectLeafIndexRequestAndResponse(
+//     "D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
+//     "123456");
+//
+// LogDnsClient log_client(mock_dns.CreateDnsClient(), ...);
+// LogDnsClient::QueryLeafIndexCallback callback = base::Bind(...);

[Ryan Sleevi, 2016/09/23 21:29:47]

As mentioned to Eran in a recent CL, I'm very much opposed to these sorts of
local-callbacks.

log_client.QueryLeafIndex("ct.test", ..., base::Bind(...));

would suffice.

[Rob Percival, 2016/10/03 13:35:45]

Done.

+// log_client.QueryLeafIndex("ct.test", ..., callback);
+//
+// // Pump the message loop. This will lead to |callback| being invoked with
+// // the result of the leaf index query.

[Ryan Sleevi, 2016/09/23 21:29:47]

You're making an API contract that RunUntilIdle is sufficient, but I don't
believe it is.

I would instead remove all of 98-100.

[Rob Percival, 2016/10/03 13:35:45]

Done.

+// base::RunLoop().RunUntilIdle();
 class MockLogDnsTraffic {
  public:
   MockLogDnsTraffic();
   ~MockLogDnsTraffic();
 
@@ -124,10 +147,12 @@ class MockLogDnsTraffic {
   // Creates a DNS client that uses mock sockets.
   // It is this DNS client that the expectations will be tested against.
   std::unique_ptr<net::DnsClient> CreateDnsClient();
 
   // Sets whether mock reads should complete synchronously or asynchronously.
+  // By default, they complete asynchronously. The only reason to change this
+  // is to test that LogDnsClient handles both modes in the same way.

[Ryan Sleevi, 2016/09/23 21:29:47]

DESIGN: I would expect that this would be private, with friended for the tests.

[Rob Percival, 2016/10/03 13:35:45]

Done.

   void SetSocketReadMode(net::IoMode read_mode) {
     socket_read_mode_ = read_mode;
   }
 
  private: