Allow a callback to populate mock DNS responses.

Allow a callback to populate mock DNS responses.

This improves the flexibility of the DNS mocking framework.
It will be useful when testing the Certificate Transparency DNS client.

BUG=612439

[Rob Percival, 2016-06-02 19:13:47]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-02 19:15:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2030783003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2030783003/1

[Rob Percival, 2016-06-02 19:18:03]

robpercival@chromium.org changed reviewers:
+ mmenke@chromium.org

[Rob Percival, 2016-06-02 19:18:04]

PTAL

[commit-bot: I haz the power, 2016-06-02 19:33:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-02 19:33:24]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[mmenke, 2016-06-02 19:46:05]

https://codereview.chromium.org/2030783003/diff/1/net/dns/dns_test_util.h
File net/dns/dns_test_util.h (right):

https://codereview.chromium.org/2030783003/diff/1/net/dns/dns_test_util.h#new...
net/dns/dns_test_util.h:177: base::BigEndianWriter* answer_writer)>;
Can you just use mock UDP sockets for this?

Reasons:

* That makes for integration-y tests, making sure DnsTransactions can handle
whever new requests you throw at it.
* That's more flexible.  Makes it easier to send malformed packets containing
whatever you want, so we don't end up having to add yet another API for that, if
we discover we need it.
* It's also less new code.

[Rob Percival, 2016-06-02 21:20:04]

On 2016/06/02 19:46:05, mmenke wrote:
> https://codereview.chromium.org/2030783003/diff/1/net/dns/dns_test_util.h
> File net/dns/dns_test_util.h (right):
> 
>
https://codereview.chromium.org/2030783003/diff/1/net/dns/dns_test_util.h#new...
> net/dns/dns_test_util.h:177: base::BigEndianWriter* answer_writer)>;
> Can you just use mock UDP sockets for this?
> 
> Reasons:
> 
> * That makes for integration-y tests, making sure DnsTransactions can handle
> whever new requests you throw at it.
> * That's more flexible.  Makes it easier to send malformed packets containing
> whatever you want, so we don't end up having to add yet another API for that,
if
> we discover we need it.
> * It's also less new code.

Are these the mock UDP sockets you're talking about:
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/socket_...
While that does seem better suited for sending malformed packets, it's arguably
less well suited to my current use case. I have a class that uses DnsClient, and
I'd like to test that it's sending the correct requests and parsing the
responses correctly. From what I can tell, using mock UDP sockets would make
that considerably more complicated. With this solution, I just have to create
some MockDnsClientRules and provide a single callback that knows how to write
the appropriate TXT record answers. I've only skimmed socket_test_util.h though,
so if I'm missing a piece of the puzzle that makes it equally straightforward
with mock sockets, please point that out.

On the topic of new code, this actually doesn't introduce a great deal of new
code. A decent portion of the code has just been extracted from one function
into a set of callbacks and refactored a little to make a reasonable interface.

[mmenke, 2016-06-02 21:45:21]

On 2016/06/02 21:20:04, Rob Percival wrote:
> On 2016/06/02 19:46:05, mmenke wrote:
> > https://codereview.chromium.org/2030783003/diff/1/net/dns/dns_test_util.h
> > File net/dns/dns_test_util.h (right):
> > 
> >
>
https://codereview.chromium.org/2030783003/diff/1/net/dns/dns_test_util.h#new...
> > net/dns/dns_test_util.h:177: base::BigEndianWriter* answer_writer)>;
> > Can you just use mock UDP sockets for this?
> > 
> > Reasons:
> > 
> > * That makes for integration-y tests, making sure DnsTransactions can handle
> > whever new requests you throw at it.
> > * That's more flexible.  Makes it easier to send malformed packets
containing
> > whatever you want, so we don't end up having to add yet another API for
that,
> if
> > we discover we need it.
> > * It's also less new code.
> 
> Are these the mock UDP sockets you're talking about:
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/socket_...
> While that does seem better suited for sending malformed packets, it's
arguably
> less well suited to my current use case. I have a class that uses DnsClient,
and
> I'd like to test that it's sending the correct requests and parsing the
> responses correctly. From what I can tell, using mock UDP sockets would make
> that considerably more complicated. With this solution, I just have to create
> some MockDnsClientRules and provide a single callback that knows how to write
> the appropriate TXT record answers. I've only skimmed socket_test_util.h
though,
> so if I'm missing a piece of the puzzle that makes it equally straightforward
> with mock sockets, please point that out.

Yes, those are the one's I'm talking about.  You'd use the new
DnsClient::CreateClient method I recently added that takes a socket factory, and
returns a DnsClient that uses it.  So it would look something like:

// Could also use a big endian writer or whatever.
const char* expected_request_data = "...";
const char* response_data = "...";

MockWrites writes[] = {MockWrite(expected_request_data , 0)};
MockReads reads[] = {MockRead(response_data , 1)};

SequencedSocketData data(...);
MockClientSocketFactory socket_factory;
socket_factory.AddSocketDataProvider(&data);

unique_ptr<DnsClient> client = CreateClient(&socket_factory);

And then use the client to issue the transaction.

> On the topic of new code, this actually doesn't introduce a great deal of new
> code. A decent portion of the code has just been extracted from one function
> into a set of callbacks and refactored a little to make a reasonable
interface.

The API is also a little weird (A "BigEndianWriter" is a somewhat weird object
to be exposing).  Reusing classes also means there less test fixture code to
learn (And most people working on net/ will know about mock sockets, since
they're used in a huge number of tests).  Anyhow, don't want to push back too
hard on this, but was wondering if existing tools met your needs.

(On a side note, make sure you check plenty of cases where you get weird data
from the server, and you may want to add a fuzzer as well)

[Rob Percival, 2016-06-02 22:34:43]

On 2016/06/02 21:45:21, mmenke wrote:
> On 2016/06/02 21:20:04, Rob Percival wrote:
> > On 2016/06/02 19:46:05, mmenke wrote:
> > > https://codereview.chromium.org/2030783003/diff/1/net/dns/dns_test_util.h
> > > File net/dns/dns_test_util.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2030783003/diff/1/net/dns/dns_test_util.h#new...
> > > net/dns/dns_test_util.h:177: base::BigEndianWriter* answer_writer)>;
> > > Can you just use mock UDP sockets for this?
> > > 
> > > Reasons:
> > > 
> > > * That makes for integration-y tests, making sure DnsTransactions can
handle
> > > whever new requests you throw at it.
> > > * That's more flexible.  Makes it easier to send malformed packets
> containing
> > > whatever you want, so we don't end up having to add yet another API for
> that,
> > if
> > > we discover we need it.
> > > * It's also less new code.
> > 
> > Are these the mock UDP sockets you're talking about:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/socket_...
> > While that does seem better suited for sending malformed packets, it's
> arguably
> > less well suited to my current use case. I have a class that uses DnsClient,
> and
> > I'd like to test that it's sending the correct requests and parsing the
> > responses correctly. From what I can tell, using mock UDP sockets would make
> > that considerably more complicated. With this solution, I just have to
create
> > some MockDnsClientRules and provide a single callback that knows how to
write
> > the appropriate TXT record answers. I've only skimmed socket_test_util.h
> though,
> > so if I'm missing a piece of the puzzle that makes it equally
straightforward
> > with mock sockets, please point that out.
> 
> Yes, those are the one's I'm talking about.  You'd use the new
> DnsClient::CreateClient method I recently added that takes a socket factory,
and
> returns a DnsClient that uses it.  So it would look something like:
> 
> // Could also use a big endian writer or whatever.
> const char* expected_request_data = "...";
> const char* response_data = "...";
> 
> MockWrites writes[] = {MockWrite(expected_request_data , 0)};
> MockReads reads[] = {MockRead(response_data , 1)};
> 
> SequencedSocketData data(...);
> MockClientSocketFactory socket_factory;
> socket_factory.AddSocketDataProvider(&data);
> 
> unique_ptr<DnsClient> client = CreateClient(&socket_factory);
> 
> And then use the client to issue the transaction.
> 
> > On the topic of new code, this actually doesn't introduce a great deal of
new
> > code. A decent portion of the code has just been extracted from one function
> > into a set of callbacks and refactored a little to make a reasonable
> interface.
> 
> The API is also a little weird (A "BigEndianWriter" is a somewhat weird object
> to be exposing).  Reusing classes also means there less test fixture code to
> learn (And most people working on net/ will know about mock sockets, since
> they're used in a huge number of tests).  Anyhow, don't want to push back too
> hard on this, but was wondering if existing tools met your needs.
> 
> (On a side note, make sure you check plenty of cases where you get weird data
> from the server, and you may want to add a fuzzer as well)

That's more succinct than I was expecting, it's just a shame that the expected
reads and writes will be opaque blobs rather than the more self-documenting
MockDnsClientRules. That is, assuming there's no DNS code that I can easily
reuse for building the expected reads/writes (I haven't noticed any). I suppose
a bunch of comments explaining what each blob consists of would suffice.

As for the API, the first iteration had it just expose the DnsResponse, which
was cleaner but insufficient. That class lacks some essentials, like pointers to
where the different sections begin, so a callback couldn't just make its own
BigEndianWriter and write to the correct location. This ended up being the
"nicest", complete solution that I could come up with. I *thought* I was
(mostly) reusing classes when I came across MockDnsClientRule, especially since
the name makes it sound like it's exactly the thing to use when mocking
DnsClient! I'll give your approach a try though and see how it compares.

RE side note: Fortunately, the Certificate Transparency DNS responses are very
simple - one is a TXT record containing a single number, the other a TXT record
containing one or more 32 byte hashes - so there's not much opportunity for
weirdness that could be mistaken for a valid response or do something horrible.
DnsClient is responsible for parsing practically all of the dangerous parts of
the response (field lengths, etc.). Still, I'll be sure to test all the oddities
I can think of. Those mock sockets would certainly be helpful for fuzzing.

[Rob Percival, 2016-06-02 22:49:35]

On 2016/06/02 22:34:43, Rob Percival wrote:
> On 2016/06/02 21:45:21, mmenke wrote:
> > On 2016/06/02 21:20:04, Rob Percival wrote:
> > > On 2016/06/02 19:46:05, mmenke wrote:
> > > >
https://codereview.chromium.org/2030783003/diff/1/net/dns/dns_test_util.h
> > > > File net/dns/dns_test_util.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2030783003/diff/1/net/dns/dns_test_util.h#new...
> > > > net/dns/dns_test_util.h:177: base::BigEndianWriter* answer_writer)>;
> > > > Can you just use mock UDP sockets for this?
> > > > 
> > > > Reasons:
> > > > 
> > > > * That makes for integration-y tests, making sure DnsTransactions can
> handle
> > > > whever new requests you throw at it.
> > > > * That's more flexible.  Makes it easier to send malformed packets
> > containing
> > > > whatever you want, so we don't end up having to add yet another API for
> > that,
> > > if
> > > > we discover we need it.
> > > > * It's also less new code.
> > > 
> > > Are these the mock UDP sockets you're talking about:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/socket_...
> > > While that does seem better suited for sending malformed packets, it's
> > arguably
> > > less well suited to my current use case. I have a class that uses
DnsClient,
> > and
> > > I'd like to test that it's sending the correct requests and parsing the
> > > responses correctly. From what I can tell, using mock UDP sockets would
make
> > > that considerably more complicated. With this solution, I just have to
> create
> > > some MockDnsClientRules and provide a single callback that knows how to
> write
> > > the appropriate TXT record answers. I've only skimmed socket_test_util.h
> > though,
> > > so if I'm missing a piece of the puzzle that makes it equally
> straightforward
> > > with mock sockets, please point that out.
> > 
> > Yes, those are the one's I'm talking about.  You'd use the new
> > DnsClient::CreateClient method I recently added that takes a socket factory,
> and
> > returns a DnsClient that uses it.  So it would look something like:
> > 
> > // Could also use a big endian writer or whatever.
> > const char* expected_request_data = "...";
> > const char* response_data = "...";
> > 
> > MockWrites writes[] = {MockWrite(expected_request_data , 0)};
> > MockReads reads[] = {MockRead(response_data , 1)};
> > 
> > SequencedSocketData data(...);
> > MockClientSocketFactory socket_factory;
> > socket_factory.AddSocketDataProvider(&data);
> > 
> > unique_ptr<DnsClient> client = CreateClient(&socket_factory);
> > 
> > And then use the client to issue the transaction.
> > 
> > > On the topic of new code, this actually doesn't introduce a great deal of
> new
> > > code. A decent portion of the code has just been extracted from one
function
> > > into a set of callbacks and refactored a little to make a reasonable
> > interface.
> > 
> > The API is also a little weird (A "BigEndianWriter" is a somewhat weird
object
> > to be exposing).  Reusing classes also means there less test fixture code to
> > learn (And most people working on net/ will know about mock sockets, since
> > they're used in a huge number of tests).  Anyhow, don't want to push back
too
> > hard on this, but was wondering if existing tools met your needs.
> > 
> > (On a side note, make sure you check plenty of cases where you get weird
data
> > from the server, and you may want to add a fuzzer as well)
> 
> That's more succinct than I was expecting, it's just a shame that the expected
> reads and writes will be opaque blobs rather than the more self-documenting
> MockDnsClientRules. That is, assuming there's no DNS code that I can easily
> reuse for building the expected reads/writes (I haven't noticed any). I
suppose
> a bunch of comments explaining what each blob consists of would suffice.
> 
> As for the API, the first iteration had it just expose the DnsResponse, which
> was cleaner but insufficient. That class lacks some essentials, like pointers
to
> where the different sections begin, so a callback couldn't just make its own
> BigEndianWriter and write to the correct location. This ended up being the
> "nicest", complete solution that I could come up with. I *thought* I was
> (mostly) reusing classes when I came across MockDnsClientRule, especially
since
> the name makes it sound like it's exactly the thing to use when mocking
> DnsClient! I'll give your approach a try though and see how it compares.
> 
> RE side note: Fortunately, the Certificate Transparency DNS responses are very
> simple - one is a TXT record containing a single number, the other a TXT
record
> containing one or more 32 byte hashes - so there's not much opportunity for
> weirdness that could be mistaken for a valid response or do something
horrible.
> DnsClient is responsible for parsing practically all of the dangerous parts of
> the response (field lengths, etc.). Still, I'll be sure to test all the
oddities
> I can think of. Those mock sockets would certainly be helpful for fuzzing.

Wow, when you said to use the "DnsClient::CreateClient method I recently added",
I didn't expect "recently" meant 6 hours ago! I feel better about not having
considered that option, given that it didn't exist :)

[Rob Percival, 2016-06-02 22:50:22]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-02 22:51:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2030783003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2030783003/20001

[mmenke, 2016-06-03 00:31:27]

On 2016/06/02 22:49:35, Rob Percival wrote:
> On 2016/06/02 22:34:43, Rob Percival wrote:
> > On 2016/06/02 21:45:21, mmenke wrote:
> > > On 2016/06/02 21:20:04, Rob Percival wrote:
> > > > On 2016/06/02 19:46:05, mmenke wrote:
> > > > >
> https://codereview.chromium.org/2030783003/diff/1/net/dns/dns_test_util.h
> > > > > File net/dns/dns_test_util.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2030783003/diff/1/net/dns/dns_test_util.h#new...
> > > > > net/dns/dns_test_util.h:177: base::BigEndianWriter* answer_writer)>;
> > > > > Can you just use mock UDP sockets for this?
> > > > > 
> > > > > Reasons:
> > > > > 
> > > > > * That makes for integration-y tests, making sure DnsTransactions can
> > handle
> > > > > whever new requests you throw at it.
> > > > > * That's more flexible.  Makes it easier to send malformed packets
> > > containing
> > > > > whatever you want, so we don't end up having to add yet another API
for
> > > that,
> > > > if
> > > > > we discover we need it.
> > > > > * It's also less new code.
> > > > 
> > > > Are these the mock UDP sockets you're talking about:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/socket_...
> > > > While that does seem better suited for sending malformed packets, it's
> > > arguably
> > > > less well suited to my current use case. I have a class that uses
> DnsClient,
> > > and
> > > > I'd like to test that it's sending the correct requests and parsing the
> > > > responses correctly. From what I can tell, using mock UDP sockets would
> make
> > > > that considerably more complicated. With this solution, I just have to
> > create
> > > > some MockDnsClientRules and provide a single callback that knows how to
> > write
> > > > the appropriate TXT record answers. I've only skimmed socket_test_util.h
> > > though,
> > > > so if I'm missing a piece of the puzzle that makes it equally
> > straightforward
> > > > with mock sockets, please point that out.
> > > 
> > > Yes, those are the one's I'm talking about.  You'd use the new
> > > DnsClient::CreateClient method I recently added that takes a socket
factory,
> > and
> > > returns a DnsClient that uses it.  So it would look something like:
> > > 
> > > // Could also use a big endian writer or whatever.
> > > const char* expected_request_data = "...";
> > > const char* response_data = "...";
> > > 
> > > MockWrites writes[] = {MockWrite(expected_request_data , 0)};
> > > MockReads reads[] = {MockRead(response_data , 1)};
> > > 
> > > SequencedSocketData data(...);
> > > MockClientSocketFactory socket_factory;
> > > socket_factory.AddSocketDataProvider(&data);
> > > 
> > > unique_ptr<DnsClient> client = CreateClient(&socket_factory);
> > > 
> > > And then use the client to issue the transaction.
> > > 
> > > > On the topic of new code, this actually doesn't introduce a great deal
of
> > new
> > > > code. A decent portion of the code has just been extracted from one
> function
> > > > into a set of callbacks and refactored a little to make a reasonable
> > > interface.
> > > 
> > > The API is also a little weird (A "BigEndianWriter" is a somewhat weird
> object
> > > to be exposing).  Reusing classes also means there less test fixture code
to
> > > learn (And most people working on net/ will know about mock sockets, since
> > > they're used in a huge number of tests).  Anyhow, don't want to push back
> too
> > > hard on this, but was wondering if existing tools met your needs.
> > > 
> > > (On a side note, make sure you check plenty of cases where you get weird
> data
> > > from the server, and you may want to add a fuzzer as well)
> > 
> > That's more succinct than I was expecting, it's just a shame that the
expected
> > reads and writes will be opaque blobs rather than the more self-documenting
> > MockDnsClientRules. That is, assuming there's no DNS code that I can easily
> > reuse for building the expected reads/writes (I haven't noticed any). I
> suppose
> > a bunch of comments explaining what each blob consists of would suffice.

Seems like the code to generate the mock response would be the same, either way
- you could still use the BigEndianWriter, though I suppose you would need to
generate the request as well as the response.

One thing nice about static strings, though, hideous though they be, is that
you're not relying on classes used in production code to deal with the data, so
it may be a bit more robust against some types of breakage (Though uglier as
well).

> > As for the API, the first iteration had it just expose the DnsResponse,
which
> > was cleaner but insufficient. That class lacks some essentials, like
pointers
> to
> > where the different sections begin, so a callback couldn't just make its own
> > BigEndianWriter and write to the correct location. This ended up being the
> > "nicest", complete solution that I could come up with. I *thought* I was
> > (mostly) reusing classes when I came across MockDnsClientRule, especially
> since
> > the name makes it sound like it's exactly the thing to use when mocking
> > DnsClient! I'll give your approach a try though and see how it compares.
> > 
> > RE side note: Fortunately, the Certificate Transparency DNS responses are
very
> > simple - one is a TXT record containing a single number, the other a TXT
> record
> > containing one or more 32 byte hashes - so there's not much opportunity for
> > weirdness that could be mistaken for a valid response or do something
> horrible.
> > DnsClient is responsible for parsing practically all of the dangerous parts
of
> > the response (field lengths, etc.). Still, I'll be sure to test all the
> oddities
> > I can think of. Those mock sockets would certainly be helpful for fuzzing.
> 
> Wow, when you said to use the "DnsClient::CreateClient method I recently
added",
> I didn't expect "recently" meant 6 hours ago! I feel better about not having
> considered that option, given that it didn't exist :)

:)

If you want to write a fuzzer, that CL also, purely coincidentally, added the
ability to fuzz UDP sockets to "FuzzedSocketFactory".  So just use one of those
instead of a MockSocketFactory, and you're pretty much done, in terms of fuzzing
your response parsing code.

[commit-bot: I haz the power, 2016-06-03 00:36:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 00:36:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Rob Percival, 2016-06-13 16:04:41]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-13 16:04:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2030783003/40001

[commit-bot: I haz the power, 2016-06-13 17:04:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-13 17:04:33]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-06-15 16:03:49]

This isn't blocked on me, right?  Just want to make sure I'm not holding this up
without realizing it.

[chromium-reviews, 2016-06-15 16:20:53]

No it's not, I'm still evaluating using mock sockets as an alternative. The
only reason this is getting updated is because I've been using it to test
my code in the meantime. The mock socket tests have been problematic, with
odd issues like DnsClient claiming it can't connect (while using mock UDP),
but hopefully I'll figure that out soon.

On Wed, 15 Jun 2016, 17:03 , <mmenke@chromium.org> wrote:

> This isn't blocked on me, right? Just want to make sure I'm not holding
> this up
> without realizing it.
>
> https://codereview.chromium.org/2030783003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Rob Percival, 2016-06-27 19:21:45]

Description was changed from

==========
Allow a callback to populate mock DNS responses.

This improves the flexibility of the DNS mocking framework.
It will be useful when testing the Certificate Transparency DNS client.

BUG=612439
==========

to

==========
Allow a callback to populate mock DNS responses.

This improves the flexibility of the DNS mocking framework.
It will be useful when testing the Certificate Transparency DNS client.

BUG=612439
==========

[Rob Percival, 2016-07-06 16:39:53]

On 2016/06/15 16:20:53, chromium-reviews wrote:
> No it's not, I'm still evaluating using mock sockets as an alternative. The
> only reason this is getting updated is because I've been using it to test
> my code in the meantime. The mock socket tests have been problematic, with
> odd issues like DnsClient claiming it can't connect (while using mock UDP),
> but hopefully I'll figure that out soon.
> 
> On Wed, 15 Jun 2016, 17:03 , <mailto:mmenke@chromium.org> wrote:
> 
> > This isn't blocked on me, right? Just want to make sure I'm not holding
> > this up
> > without realizing it.
> >
> > https://codereview.chromium.org/2030783003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

There's renewed call for this change now (from Eran Messeri). While LogDnsClient
no longer requires it, anything that uses a LogDnsClient needs a way to mock out
the socket operations. They could use mock sockets, but that's considerably more
complicated than using MockDnsClient. The options are either:
A) Land this change and then users of LogDnsClient can pass a MockDnsClient to
LogDnsClient in tests.
B) Refactor the mock socket code out of the LogDnsClient tests and into some
test utilities so it can be re-used in tests that depend on LogDnsClient.
C) Add an interface that LogDnsClient implements and then users can mock out the
entire LogDnsClient.

Eran prefers option A, but I'm interested to hear your take on it. Option B is
certainly viable, though I'm not sure quite how easily that mock socket code can
be separated from the test cases. Option C seems like it would be in direct
opposition to your preference for "integration-y" tests.

[Eran Messeri, 2016-07-08 11:53:16]

To be clear, all I'm after is an easy way to test classes that use the
LogDnsClient that in turn uses net::DnsClient.
It seems to me there is value in testing the LogDnsClient by using a real
DnsClient and fake socket data (so we test the contract between the LogDnsClient
and the real DnsClient). However, it seems like there's less value in using a
real DnsClient when the contract tested is between a user of the DnsClient and a
user of a user of the DnsClient.

There are ways to work around this, but having a way to specify clearly (i.e.
setting expectations on a mock, compared to providing raw socket data) what the
LogDnsClient should receive makes testing usage of the LogDnsClient much
simpler.

[Rob Percival, 2016-07-15 16:30:21]

On 2016/07/08 11:53:16, Eran Messeri wrote:
> To be clear, all I'm after is an easy way to test classes that use the
> LogDnsClient that in turn uses net::DnsClient.
> It seems to me there is value in testing the LogDnsClient by using a real
> DnsClient and fake socket data (so we test the contract between the
LogDnsClient
> and the real DnsClient). However, it seems like there's less value in using a
> real DnsClient when the contract tested is between a user of the DnsClient and
a
> user of a user of the DnsClient.
> 
> There are ways to work around this, but having a way to specify clearly (i.e.
> setting expectations on a mock, compared to providing raw socket data) what
the
> LogDnsClient should receive makes testing usage of the LogDnsClient much
> simpler.

Here's another option I've put together - a reusable version of the test code
for LogDnsClient, powered by the mock sockets mmenke recommended:
https://codereview.chromium.org/2149973003/