MockLogDnsTraffic now returns bool instead of CHECKing bad arguments

As suggested by Ryan Sleevi (https://codereview.chromium.org/2348393002/diff/80001/components/certificate_transparency/mock_log_dns_traffic.cc#newcode47), MockLogDnsTraffic will no longer CHECK-fail when given invalid test expectations. Instead, it will return false and the tests using it can ASSERT_TRUE.

BUG=624894
Committed: https://crrev.com/7038ecd8f58dc8fbd3201ad84b3875a6edc3ce3c
Cr-Commit-Position: refs/heads/master@{#426510}

[Rob Percival, 2016-10-04 15:24:36]

Patchset #1 (id:1) has been deleted

[Rob Percival, 2016-10-04 15:24:50]

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

[commit-bot: I haz the power, 2016-10-04 15:25:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 16:11:43]

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

[commit-bot: I haz the power, 2016-10-04 16:11:44]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2016-10-07 14:28:29]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2016-10-07 14:28:29]

The CL description feels a little passive/aggressive - it sounds like you don't
agree, or perhaps I didn't explain well, so I'll try to explain more the
reasoning and you can see how you feel:

1) DCHECKs cause unit tests to crash when DCHECKs are enabled (which they are,
in both debug & release, on Chromium's bots). When a test crashes, all tests run
in that instance are considered failing. On the waterfall, this will take one
swarming batch (~10 tests) out, which will then result in test retries to try to
isolate the failing tests, causing more tests to fail. When run on a local
machine, if the developer is running them as a single process test (which is
highly recommended for net and component tests, because the process spawn
overhead) there is no retry capability, so it results in a 'fast failure' that
can mask other failures.
2) ASSERT failures don't propagate from void subroutines (c.f.
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
) so if you go that route, it requires propagating EXPECT_NO_FATAL_FAILURE -
which is another approach you could use for this code (as opposed to DCHECK)

While I'm supportive of the argument that we should DCHECK during "test setup"
aspects, to fast-fail to the developer that they're "holding the API wrong"
(much like we do in production code), it becomes particularly difficult in test
code to separate out when it's "test expectations" and when it's "holding the
API wrong".

Basically, the DCHECK approach is similar to our discussion of ASSERT vs EXPECT,
but applied at a 'per-unittest' level - the DCHECKs cause the first test to fail
and subsequent tests not to run, while the ASSERT/EXPECT approach just causes
the test to fail.

This LGTM (although I'd appreciate it without the second sentence in the CL
description), but I'm also happy to discuss further the context/concerns here.

[Rob Percival, 2016-10-11 21:04:49]

On 2016/10/07 14:28:29, Ryan Sleevi (slow) wrote:
> The CL description feels a little passive/aggressive - it sounds like you
don't
> agree, or perhaps I didn't explain well, so I'll try to explain more the
> reasoning and you can see how you feel:
> 
> 1) DCHECKs cause unit tests to crash when DCHECKs are enabled (which they are,
> in both debug & release, on Chromium's bots). When a test crashes, all tests
run
> in that instance are considered failing. On the waterfall, this will take one
> swarming batch (~10 tests) out, which will then result in test retries to try
to
> isolate the failing tests, causing more tests to fail. When run on a local
> machine, if the developer is running them as a single process test (which is
> highly recommended for net and component tests, because the process spawn
> overhead) there is no retry capability, so it results in a 'fast failure' that
> can mask other failures.
> 2) ASSERT failures don't propagate from void subroutines (c.f.
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> ) so if you go that route, it requires propagating EXPECT_NO_FATAL_FAILURE -
> which is another approach you could use for this code (as opposed to DCHECK)
> 
> While I'm supportive of the argument that we should DCHECK during "test setup"
> aspects, to fast-fail to the developer that they're "holding the API wrong"
> (much like we do in production code), it becomes particularly difficult in
test
> code to separate out when it's "test expectations" and when it's "holding the
> API wrong".
> 
> Basically, the DCHECK approach is similar to our discussion of ASSERT vs
EXPECT,
> but applied at a 'per-unittest' level - the DCHECKs cause the first test to
fail
> and subsequent tests not to run, while the ASSERT/EXPECT approach just causes
> the test to fail.
> 
> This LGTM (although I'd appreciate it without the second sentence in the CL
> description), but I'm also happy to discuss further the context/concerns here.

Sorry, I hadn't meant it to come across that way - I was just documenting the
justification for this change (e.g. for Eran). Perhaps just linking to your
comments would have been better. I do understand the arguments in favour of
this, but the comprehensive description is appreciated nonetheless. I think the
DCHECKs are reasonable, as they'll only fail when you make impossible
expectations (e.g. data too large to fit in a DNS response). This syntax (having
to ASSERT that an expectation could be set) also seems like it could surprise
people (hence my addition of WARN_UNUSED_RESULT). However, if you think it's an
improvement, I'm willing to go along with that.

[Rob Percival, 2016-10-11 21:05:37]

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

[Rob Percival, 2016-10-11 21:05:38]

PTAL Eran.

[Eran Messeri, 2016-10-20 10:29:13]

lgtm

[Rob Percival, 2016-10-20 14:49:36]

Description was changed from

==========
MockLogDnsTraffic now returns bool instead of ASSERT/EXPECT/CHECKing

Ryan Sleevi has advised that test helpers should not ASSERT/EXPECT, nor
should they CHECK and crash a test.

BUG=624894
==========

to

==========
As suggested by Ryan Sleevi
(https://codereview.chromium.org/2348393002/diff/80001/components/certificate_...),
MockLogDnsTraffic will no longer CHECK-fail when given invalid test
expectations. Instead, it will return false and the tests using it can ASSERT on
this being true.

BUG=624894
==========

[Rob Percival, 2016-10-20 14:50:18]

Description was changed from

==========
As suggested by Ryan Sleevi
(https://codereview.chromium.org/2348393002/diff/80001/components/certificate_...),
MockLogDnsTraffic will no longer CHECK-fail when given invalid test
expectations. Instead, it will return false and the tests using it can ASSERT on
this being true.

BUG=624894
==========

to

==========
As suggested by Ryan Sleevi
(https://codereview.chromium.org/2348393002/diff/80001/components/certificate_...),
MockLogDnsTraffic will no longer CHECK-fail when given invalid test
expectations. Instead, it will return false and the tests using it can
ASSERT_TRUE.

BUG=624894
==========

[Rob Percival, 2016-10-20 15:46:53]

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

[commit-bot: I haz the power, 2016-10-20 15:47:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 16:42:50]

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

[commit-bot: I haz the power, 2016-10-20 16:42:51]

Dry run: This issue passed the CQ dry run.

[Rob Percival, 2016-10-20 16:46:37]

The CQ bit was checked by robpercival@chromium.org

[Rob Percival, 2016-10-20 16:46:38]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2389993003/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-20 16:46:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 17:06:20]

Description was changed from

==========
As suggested by Ryan Sleevi
(https://codereview.chromium.org/2348393002/diff/80001/components/certificate_...),
MockLogDnsTraffic will no longer CHECK-fail when given invalid test
expectations. Instead, it will return false and the tests using it can
ASSERT_TRUE.

BUG=624894
==========

to

==========
As suggested by Ryan Sleevi
(https://codereview.chromium.org/2348393002/diff/80001/components/certificate_...),
MockLogDnsTraffic will no longer CHECK-fail when given invalid test
expectations. Instead, it will return false and the tests using it can
ASSERT_TRUE.

BUG=624894
==========

[commit-bot: I haz the power, 2016-10-20 17:06:21]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-10-21 13:19:22]

Description was changed from

==========
As suggested by Ryan Sleevi
(https://codereview.chromium.org/2348393002/diff/80001/components/certificate_...),
MockLogDnsTraffic will no longer CHECK-fail when given invalid test
expectations. Instead, it will return false and the tests using it can
ASSERT_TRUE.

BUG=624894
==========

to

==========
As suggested by Ryan Sleevi
(https://codereview.chromium.org/2348393002/diff/80001/components/certificate_...),
MockLogDnsTraffic will no longer CHECK-fail when given invalid test
expectations. Instead, it will return false and the tests using it can
ASSERT_TRUE.

BUG=624894

Committed: https://crrev.com/7038ecd8f58dc8fbd3201ad84b3875a6edc3ce3c
Cr-Commit-Position: refs/heads/master@{#426510}
==========

[commit-bot: I haz the power, 2016-10-21 13:19:22]

Patchset 2 (id:??) landed as
https://crrev.com/7038ecd8f58dc8fbd3201ad84b3875a6edc3ce3c
Cr-Commit-Position: refs/heads/master@{#426510}