Minor improvements to MockLogDnsTraffic

components/certificate_transparency/mock_log_dns_traffic.h

 // Copyright 2016 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.
 
 #ifndef COMPONENTS_CERTIFICATE_TRANSPARENCY_MOCK_LOG_DNS_TRAFFIC_H_
 #define COMPONENTS_CERTIFICATE_TRANSPARENCY_MOCK_LOG_DNS_TRAFFIC_H_
 
 #include <stdint.h>
 
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "base/macros.h"
 #include "base/strings/string_piece.h"
 #include "net/dns/dns_client.h"
 #include "net/dns/dns_config_service.h"
 #include "net/socket/socket_test_util.h"
 
 namespace certificate_transparency {
 
 namespace internal {
 
 // A container for all of the data we need to keep alive for a mock socket.

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

https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis...

// A container for the data needed for simulating a mock socket.

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

Done.

 // This is useful because Mock{Read,Write}, SequencedSocketData and
 // MockClientSocketFactory all do not take ownership of or copy their arguments,
 // so we have to manage the lifetime of those arguments ourselves. Wrapping all

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

// so it is necessary to manage the lifetime of those arguments.

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

Done.

 // of that up in a single class simplifies this.
 // This cannot be forward declared because MockLogDnsTraffic has a
 // vector<unique_ptr<MockSocketData>> member, which requires MockSocketData be

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

Both vector and unique_ptr should allow you to forward declare, so long as the
constructors are out-of-line.

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

Both vector and unique_ptr are templates, so they can't possibly have
out-of-line constructors. I assume I'm misunderstanding you - could you clarify
please?

[Rob Percival, 2016/10/03 13:50:19]

Never mind, fixed.

 // defined.
 class MockSocketData {
  public:
   // A socket that expects one write and one read operation.
   MockSocketData(const std::vector<char>& write, const std::vector<char>& read);

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

Isn't base::StringPiece the preferred data structure here?

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

One of the main purposes of this class is to manage the lifetime of these
arguments. If I change these to be StringPiece, I'll need to immediately convert
them into std::vector<char> or std::string. Since they're built from
std::vector<char> in the first place, it seems logical to keep them this way.

   // A socket that expects one write and a read error.
   MockSocketData(const std::vector<char>& write, int net_error);
   // A socket that expects one write and no response.
   explicit MockSocketData(const std::vector<char>& write);
 
   ~MockSocketData();
 
   void SetWriteMode(net::IoMode mode) { expected_write_.mode = mode; }
   void SetReadMode(net::IoMode mode) { expected_reads_[0].mode = mode; }
 
   void AddToFactory(net::MockClientSocketFactory* socket_factory);
 
  private:
   // Prevents read overruns and makes a socket timeout the default behaviour.
   static const net::MockRead no_more_data_;

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

class-level private statics do not provide any advantages over file-local
statics in an unnamed namespace.

That is, no one can reach into it, it's still a global (just by another name),
but at least that way, your implementation details don't leak into your header.

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

Done.

 
   // This class only supports one write and one read, so just need to store one
   // payload each.
   const std::vector<char> expected_write_payload_;
   const std::vector<char> expected_read_payload_;
   // Encapsulates the data that is expected to be written to a socket.
   net::MockWrite expected_write_;

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

Line breaks between 55-56, 57-58, 64-65 will all help readability.

Consider this code review interface, which doesn't support any syntax
highlighting. Unless actively focused on the text, the structure is not visible,
and the ability to rest the eyes on a particular section requires more effort.

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

Ah I forget that there's so syntax highlighting in Rietveld by default. You
might want to install the Rietveld Usability Toolkit
(https://chrome.google.com/webstore/detail/rietveld-usability-toolki/nmljjlfbn...),
which adds syntax highlighting and some other nice features too. I've added the
suggested line breaks anyway though, as it's a good idea.

   // Encapsulates the data/error that should be returned when reading from a
   // socket. The second "expected" read is always |no_more_data_|, which
   // causes the socket read to hang until it times out. This results in better
   // test failure messages (rather than a CHECK-fail due to a socket read
   // overrunning the MockRead array) and behaviour more like a real socket when
   // an unexpected second socket read occurs.
   net::MockRead expected_reads_[2];
   // Holds pointers to |expected_write_| and |expected_reads_|. This is what is
   // added to net::MockClientSocketFactory to prepare a mock socket.
   net::SequencedSocketData socket_data_;
 
   DISALLOW_COPY_AND_ASSIGN(MockSocketData);
 };
 
 }  // namespace internal
 
 // 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();
 
   // Expect a CT DNS request for the domain |qname|.
   // Such a request will receive a DNS response indicating that the error
   // specified by |rcode| occurred. See RFC1035, Section 4.1.1 for |rcode|
   // values.
   void ExpectRequestAndErrorResponse(base::StringPiece qname, uint8_t rcode);
   // Expect a CT DNS request for the domain |qname|.

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

Linebreak between 110-111

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

Done.

   // Such a request will trigger a socket error of type |net_error|.
   // |net_error| can be any net:Error value.
   void ExpectRequestAndSocketError(base::StringPiece qname, int net_error);
   // Expect a CT DNS request for the domain |qname|.

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

Linebreak

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

Done.

   // Such a request will timeout.
   // This will reduce the DNS timeout to minimize test duration.
   void ExpectRequestAndTimeout(base::StringPiece qname);

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

Line break

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

Done.

   // Expect a CT DNS request for the domain |qname|.
   // Such a request will receive a DNS response containing |leaf_index|.
   // A description of such a request and response can be seen here:
   // https://github.com/google/certificate-transparency-rfcs/blob/c8844de6bd0b5d3d16bac79865e6edef533d760b/dns/draft-ct-over-dns.md#hash-query-hashquery
   void ExpectLeafIndexRequestAndResponse(base::StringPiece qname,
                                          base::StringPiece leaf_index);
   // Expect a CT DNS request for the domain |qname|.

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

Line break

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

Done.

   // Such a request will receive a DNS response containing the inclusion proof
   // nodes between |audit_path_start| and |audit_path_end|.
   // A description of such a request and response can be seen here:
   // https://github.com/google/certificate-transparency-rfcs/blob/c8844de6bd0b5d3d16bac79865e6edef533d760b/dns/draft-ct-over-dns.md#tree-query-treequery
   void ExpectAuditProofRequestAndResponse(
       base::StringPiece qname,
       std::vector<std::string>::const_iterator audit_path_start,
       std::vector<std::string>::const_iterator audit_path_end);
 
   // Sets the initial DNS config appropriate for testing.
   // Requires that net::NetworkChangeNotifier is initialized first.
   // The DNS config is propogated to NetworkChangeNotifier::DNSObservers
   // asynchronously.
   void InitializeDnsConfig();
 
   // Sets the DNS config to |config|.
   // Requires that net::NetworkChangeNotifier is initialized first.
   // The DNS config is propogated to NetworkChangeNotifier::DNSObservers
   // asynchronously.
   void SetDnsConfig(const net::DnsConfig& config);
 
   // 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:
   // Expect A CT DNS request for the domain |qname|.
   // Such a request will receive a DNS response containing |answer|.
   void ExpectRequestAndResponse(base::StringPiece qname,
                                 base::StringPiece answer);
 
@@ skipping 14 matching lines @@
   net::MockClientSocketFactory socket_factory_;
   // Controls whether mock socket reads are asynchronous.
   net::IoMode socket_read_mode_;
 
   DISALLOW_COPY_AND_ASSIGN(MockLogDnsTraffic);
 };
 
 }  // namespace certificate_transparency
 
 #endif  // COMPONENTS_CERTIFICATE_TRANSPARENCY_MOCK_LOG_DNS_TRAFFIC_H_