Replace use of ASSERT in test code.

Replace use of ASSERT in test code.

In top level test functions, replaced with gtest ASSERT_*. In helper
methods in main test files, replaced with EXPECT_* or RTC_DCHECK on a
case-by-case basis.

In separate mock/fake classes used by tests (which might be of some 
use also in tests of third-party applications), ASSERT was replaced
with RTC_CHECK, using 

  git grep -l ' ASSERT(' | grep -v common.h | \
    xargs sed -i 's/ ASSERT(/ RTC_CHECK(/'

followed by additional includes of base/checks.h in affected files,
and git cl format.

BUG=webrtc:6424
Review-Url: https://codereview.webrtc.org/2622413005
Cr-Commit-Position: refs/heads/master@{#16150}
Committed: https://chromium.googlesource.com/external/webrtc/+/c8ee882753f5240018445a1ce5761fd1c3196b4e

[nisse-webrtc, 2017-01-13 15:32:14]

nisse@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[nisse-webrtc, 2017-01-13 15:32:14]

Next step. It seems appropriate with RTC_CHECK (not RTC_DCHECK) in test code.

[the sun, 2017-01-13 15:35:57]

On 2017/01/13 15:32:14, nisse-webrtc wrote:
> Next step. It seems appropriate with RTC_CHECK (not RTC_DCHECK) in test code.

Aren't logic errors the same regardless if it is test code or not?

[nisse-webrtc, 2017-01-13 15:44:25]

On 2017/01/13 15:35:57, the sun wrote:
> On 2017/01/13 15:32:14, nisse-webrtc wrote:
> > Next step. It seems appropriate with RTC_CHECK (not RTC_DCHECK) in test
code.
> 
> Aren't logic errors the same regardless if it is test code or not?

I'd be happy to use RTC_CHECK (i.e., always do the check and crash on
violations)
everywhere, but it seems that's not the current convention in the code.

As far as I understand there are two reasons one might want to use RTC_DCHECK,
and skip checks in a release build:

1. Performance (including binary size).

2. Hoping that keeping running despite serious errors somehow makes the
user happier than crashing at the spot.

And I think it's easy to agree that neither reason apply to test code.

[nisse-webrtc, 2017-01-16 15:22:07]

nisse@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[nisse-webrtc, 2017-01-16 15:22:08]

Ping?

[the sun, 2017-01-16 18:20:48]

Description was changed from

==========
Replace ASSERT by RTC_CHECK in test code.

Bulk of the changes were produced using

  git grep -l ' ASSERT(' | grep -v common.h | \
    xargs sed -i 's/ ASSERT(/ RTC_CHECK(/'

followed by additional includes of base/checks.h in affected files,
and git cl format.

BUG=webrtc:6424
==========

to

==========
Replace ASSERT by RTC_CHECK in test code.

Bulk of the changes were produced using

  git grep -l ' ASSERT(' | grep -v common.h | \
    xargs sed -i 's/ ASSERT(/ RTC_CHECK(/'

followed by additional includes of base/checks.h in affected files,
and git cl format.

BUG=webrtc:6424
==========

[the sun, 2017-01-16 18:20:48]

solenberg@webrtc.org changed reviewers:
- solenberg@webrtc.org

[the sun, 2017-01-16 18:21:49]

On 2017/01/16 15:22:08, nisse-webrtc wrote:
> Ping?

I didn't intend to add myself as reviewer. Karl has looked at the previous CLs
for this change so it's better he reviews this one as well.

[kwiberg-webrtc, 2017-01-17 09:39:25]

I've made a number of comments that apply to more than just the line they're
attached to---I've tried to put the comment on the first instance in each file.
Holler if you're having trouble extrapolating...

In general, in tests you want to use EXPECT_* if the test can reasonably
continue in case the assumption fails, and ASSERT_* if you need immediate
termination. However, ASSERT_* can only be used in the top-level test functions
(and some other places; see the docs), so sometimes you have to use CHECK
instead. Try not to do this unless you have to, though, since CHECK will kill
the whole test process, not just the current test. If EXPECT_* would work
instead, prefer it to CHECK---but e.g. for testing that a pointer is non-null
before dereferencing it, you need CHECK or ASSERT.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/rtcstatscollector_...
File webrtc/api/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/rtcstatscollector_...
webrtc/api/rtcstatscollector_unittest.cc:690:
RTC_CHECK(report->Get(expected_inbound_audio_codec.id()));
I'm pretty sure you should be using ASSERT_TRUE here.

(You *can* use ASSERT_* here because it's the top level function of a
test---ASSERT_* sets some flag and then does a return---and you *should* use
ASSERT_* rather than CHECK because the former will fail just the current test,
whereas the latter will kill the entire process, possibly making other tests
collateral damage.)

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/statscollector_uni...
File webrtc/api/statscollector_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/statscollector_uni...
webrtc/api/statscollector_unittest.cc:613: RTC_CHECK((voice_sender_info == NULL)
^ (voice_receiver_info == NULL));
You may not be able to use ASSERT_* here. Will EXPECT_* do? (CHECK is a bad
choice for reasons stated in an earlier comment.)

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/statscollector_uni...
webrtc/api/statscollector_unittest.cc:1310: RTC_CHECK(c.id().length() > 0);
ASSERT_* or EXPECT_*

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/test/fakeaudiocapt...
File webrtc/api/test/fakeaudiocapturemodule.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/test/fakeaudiocapt...
webrtc/api/test/fakeaudiocapturemodule.cc:642:
RTC_CHECK(process_thread_->IsCurrent());
CHECK may be the correct choice here, if it is the case that a failing CHECK is
a sign that the test itself is broken rather than that the thing being tested is
broken. Otherwise, I'd think EXPECT_* would be the right choice---unless of
course this code is ever used in a non-test context.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/test/fakedatachann...
File webrtc/api/test/fakedatachannelprovider.h (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/test/fakedatachann...
webrtc/api/test/fakedatachannelprovider.h:29: RTC_CHECK(ready_to_send_ &&
transport_available_);
Same situation here as in fakeaudiocapturemodule.cc.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/test/mockpeerconne...
File webrtc/api/test/mockpeerconnectionobservers.h (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/test/mockpeerconne...
webrtc/api/test/mockpeerconnectionobservers.h:113: RTC_CHECK(!called_);
Same situation here as in fakeaudiocapturemodule.cc.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:304: RTC_CHECK(channel <
channels_.size());
Since ASSERT_* is used elsewhere in this function, it's presumably safe to do so
here as well.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/port_unittest.cc
File webrtc/p2p/base/port_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/port_unittest...
webrtc/p2p/base/port_unittest.cc:1304: RTC_CHECK(conn != nullptr);
This is a top-level test function, so you can use ASSERT_*.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/turnport_unit...
File webrtc/p2p/base/turnport_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/turnport_unit...
webrtc/p2p/base/turnport_unittest.cc:277: RTC_CHECK(server_address.proto ==
PROTO_UDP);
ASSERT_* is used elsewhere in this function.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/pc/channel_unittest.cc
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/pc/channel_unittest.cc...
webrtc/pc/channel_unittest.cc:1273: RTC_CHECK((flags2_in & ~(RTCP_MUX | DTLS |
GCM_CIPHER)) == 0);
EXPECT_* ?

[kwiberg-webrtc, 2017-01-17 09:39:50]

On 2017/01/16 18:21:49, the sun wrote:
> On 2017/01/16 15:22:08, nisse-webrtc wrote:
> > Ping?
> 
> I didn't intend to add myself as reviewer. Karl has looked at the previous CLs
> for this change so it's better he reviews this one as well.

Yes, sorry, I was OOO yesterday.

[nisse-webrtc, 2017-01-17 12:19:21]

I've updated the simple things: Top-level test functions should use EXPECT_* or
ASSERT_*, not RTC_CHECK. For the remaining issues, this is my view:

1. I think only source files defining top-level tests (i.e., use the TEST or
TEST_F macros) should include gtest.h. Various fake/mock classes shouldn't have
that dependency. They should be useable in tests of third-party code, using some
other testing framework.

2. The compiler accepts ASSERT_* in any function with a void return value. Its
behavior is the same as EXPECT_*, plus an immediate return from the current
function on failure. When used anywhere but in a top-level test function, it
will *not* abort the current test on failure, and I suspect most such uses in
the current code are bugs.

3. When it's not possible or appropriate to use ASSERT_* or EXPECT_*, I think
RTC_CHECK is the right choice in all code which is intended to be used only from
test code. I don't have a very strong opinion on this point, though, I'll change
some or all to RTC_DCHECK if you think that's better.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/rtcstatscollector_...
File webrtc/api/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/rtcstatscollector_...
webrtc/api/rtcstatscollector_unittest.cc:690:
RTC_CHECK(report->Get(expected_inbound_audio_codec.id()));
On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> I'm pretty sure you should be using ASSERT_TRUE here.
> 
> (You *can* use ASSERT_* here because it's the top level function of a
> test---ASSERT_* sets some flag and then does a return---and you *should* use
> ASSERT_* rather than CHECK because the former will fail just the current test,
> whereas the latter will kill the entire process, possibly making other tests
> collateral damage.)

Done. Applied to all places in this file.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/statscollector_uni...
File webrtc/api/statscollector_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/statscollector_uni...
webrtc/api/statscollector_unittest.cc:613: RTC_CHECK((voice_sender_info == NULL)
^ (voice_receiver_info == NULL));
On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> You may not be able to use ASSERT_* here. Will EXPECT_* do? (CHECK is a bad
> choice for reasons stated in an earlier comment.)

EXPECT_TRUE seems to work.
 
But I think RTC_CHECK may be appropriate here. This is a helper function used in
4 places in this file, and the RTC_CHECK enforces that those callers use it
correctly. I.e., if this ever fails, that's a bug in the test code, not in the
code under test.

Your call.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/statscollector_uni...
webrtc/api/statscollector_unittest.cc:1310: RTC_CHECK(c.id().length() > 0);
On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> ASSERT_* or EXPECT_*

Changed to EXPECT_GT.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/test/fakeaudiocapt...
File webrtc/api/test/fakeaudiocapturemodule.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/test/fakeaudiocapt...
webrtc/api/test/fakeaudiocapturemodule.cc:642:
RTC_CHECK(process_thread_->IsCurrent());
On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> CHECK may be the correct choice here, if it is the case that a failing CHECK
is
> a sign that the test itself is broken rather than that the thing being tested
is
> broken. Otherwise, I'd think EXPECT_* would be the right choice---unless of
> course this code is ever used in a non-test context.

I don't quite like depending on gtest here. I don't think fake classes should
depend on any particular testing framework, in particular those that live in
api.

Might make sense to switch to RTC_DCHECK in this class, since (i) that's closest
to ASSERT, and (ii) for consistency with typical use of ThreadChecker. 

But on the other hand, I still think RTC_CHECK is, in general, more appropriate
than RTC_DCHECK in test code.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/test/fakedatachann...
File webrtc/api/test/fakedatachannelprovider.h (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/test/fakedatachann...
webrtc/api/test/fakedatachannelprovider.h:29: RTC_CHECK(ready_to_send_ &&
transport_available_);
On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> Same situation here as in fakeaudiocapturemodule.cc.

Agree both situations should be handled in the same way.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:304: RTC_CHECK(channel <
channels_.size());
On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> Since ASSERT_* is used elsewhere in this function, it's presumably safe to do
so
> here as well.

I'm a bit puzzled. I can use ASSERT_TRUE and ASSERT_LT in the methods SetupSrtp
and SendPackets, but when I try to use it also in SendInvalidSrtpPacket, I get a
compilation error 

../../webrtc/p2p/base/dtlstransportchannel_unittest.cc:327:5: error: cannot
initialize return object of type 'int' with an rvalue of type 'void'
    ASSERT_LT(channel, channels_.size());
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This makes me think that ASSERT_* actually does *not* work as desired in the
other functions, even though the compiler accepts it. My guess is that on
failure it will exit the helper function only, but still execute the rest of the
calling test.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/port_unittest.cc
File webrtc/p2p/base/port_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/port_unittest...
webrtc/p2p/base/port_unittest.cc:1304: RTC_CHECK(conn != nullptr);
On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> This is a top-level test function, so you can use ASSERT_*.

Done.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/turnport_unit...
File webrtc/p2p/base/turnport_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/turnport_unit...
webrtc/p2p/base/turnport_unittest.cc:277: RTC_CHECK(server_address.proto ==
PROTO_UDP);
On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> ASSERT_* is used elsewhere in this function.

And that use is probably broken...

https://codereview.webrtc.org/2622413005/diff/1/webrtc/pc/channel_unittest.cc
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/pc/channel_unittest.cc...
webrtc/pc/channel_unittest.cc:1273: RTC_CHECK((flags2_in & ~(RTCP_MUX | DTLS |
GCM_CIPHER)) == 0);
On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> EXPECT_* ?

I'll do that if you prefer. But these checks are also enforcing that the test
code itself uses this function correctly.

[kwiberg-webrtc, 2017-01-17 13:52:47]

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/statscollector_uni...
File webrtc/api/statscollector_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/statscollector_uni...
webrtc/api/statscollector_unittest.cc:613: RTC_CHECK((voice_sender_info == NULL)
^ (voice_receiver_info == NULL));
On 2017/01/17 12:19:21, nisse-webrtc wrote:
> On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> > You may not be able to use ASSERT_* here. Will EXPECT_* do? (CHECK is a bad
> > choice for reasons stated in an earlier comment.)
> 
> EXPECT_TRUE seems to work.
>  
> But I think RTC_CHECK may be appropriate here. This is a helper function used
in
> 4 places in this file, and the RTC_CHECK enforces that those callers use it
> correctly. I.e., if this ever fails, that's a bug in the test code, not in the
> code under test.
> 
> Your call.

Sounds reasonable.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/test/fakeaudiocapt...
File webrtc/api/test/fakeaudiocapturemodule.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/api/test/fakeaudiocapt...
webrtc/api/test/fakeaudiocapturemodule.cc:642:
RTC_CHECK(process_thread_->IsCurrent());
On 2017/01/17 12:19:21, nisse-webrtc wrote:
> On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> > CHECK may be the correct choice here, if it is the case that a failing CHECK
> is
> > a sign that the test itself is broken rather than that the thing being
tested
> is
> > broken. Otherwise, I'd think EXPECT_* would be the right choice---unless of
> > course this code is ever used in a non-test context.
> 
> I don't quite like depending on gtest here. I don't think fake classes should
> depend on any particular testing framework, in particular those that live in
> api.
> 
> Might make sense to switch to RTC_DCHECK in this class, since (i) that's
closest
> to ASSERT, and (ii) for consistency with typical use of ThreadChecker. 
> 
> But on the other hand, I still think RTC_CHECK is, in general, more
appropriate
> than RTC_DCHECK in test code.

Hmm. OK, CHECK is probably fine. (Since this is test code, there should be no
reason to prefer DCHECK to CHECK, I think.)

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:304: RTC_CHECK(channel <
channels_.size());
On 2017/01/17 12:19:21, nisse-webrtc wrote:
> On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> > Since ASSERT_* is used elsewhere in this function, it's presumably safe to
do
> so
> > here as well.
> 
> I'm a bit puzzled. I can use ASSERT_TRUE and ASSERT_LT in the methods
SetupSrtp
> and SendPackets, but when I try to use it also in SendInvalidSrtpPacket, I get
a
> compilation error 
> 
> ../../webrtc/p2p/base/dtlstransportchannel_unittest.cc:327:5: error: cannot
> initialize return object of type 'int' with an rvalue of type 'void'
>     ASSERT_LT(channel, channels_.size());
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> This makes me think that ASSERT_* actually does *not* work as desired in the
> other functions, even though the compiler accepts it. My guess is that on
> failure it will exit the helper function only, but still execute the rest of
the
> calling test.

Yes, on failure ASSERT_* will set some flag and return void; the compiler will
allow that in any function that returns void. If it's not a top-level test
function, the caller needs to explicitly check for fatal failures. See
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

And yes, I do believe it's not uncommon for tests to forget to do this, since
the compiler can't catch it, so it's almost always best to use ASSERT_* only in
the top-level test functions.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/turnport_unit...
File webrtc/p2p/base/turnport_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/turnport_unit...
webrtc/p2p/base/turnport_unittest.cc:277: RTC_CHECK(server_address.proto ==
PROTO_UDP);
On 2017/01/17 12:19:21, nisse-webrtc wrote:
> On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> > ASSERT_* is used elsewhere in this function.
> 
> And that use is probably broken...

In that case, EXPECT_* may be better.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/pc/channel_unittest.cc
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/pc/channel_unittest.cc...
webrtc/pc/channel_unittest.cc:1273: RTC_CHECK((flags2_in & ~(RTCP_MUX | DTLS |
GCM_CIPHER)) == 0);
On 2017/01/17 12:19:21, nisse-webrtc wrote:
> On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> > EXPECT_* ?
> 
> I'll do that if you prefer. But these checks are also enforcing that the test
> code itself uses this function correctly.

OK, use CHECK then.

[kwiberg-webrtc, 2017-01-17 13:55:34]

On 2017/01/17 12:19:21, nisse-webrtc wrote:
> I've updated the simple things: Top-level test functions should use EXPECT_*
or
> ASSERT_*, not RTC_CHECK. For the remaining issues, this is my view:
> 
> 1. I think only source files defining top-level tests (i.e., use the TEST or
> TEST_F macros) should include gtest.h. Various fake/mock classes shouldn't
have
> that dependency. They should be useable in tests of third-party code, using
some
> other testing framework.

OK. That doesn't sound unreasonable.

> 2. The compiler accepts ASSERT_* in any function with a void return value. Its
> behavior is the same as EXPECT_*, plus an immediate return from the current
> function on failure. When used anywhere but in a top-level test function, it
> will *not* abort the current test on failure, and I suspect most such uses in
> the current code are bugs.

Yes, see previous message.

> 3. When it's not possible or appropriate to use ASSERT_* or EXPECT_*, I think
> RTC_CHECK is the right choice in all code which is intended to be used only
from
> test code. I don't have a very strong opinion on this point, though, I'll
change
> some or all to RTC_DCHECK if you think that's better.

No, I agree. You don't want test code to fail to fail...

[nisse-webrtc, 2017-01-17 15:00:04]

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:304: RTC_CHECK(channel <
channels_.size());
On 2017/01/17 13:52:47, kwiberg-webrtc wrote:
> On 2017/01/17 12:19:21, nisse-webrtc wrote:
> > On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> > > Since ASSERT_* is used elsewhere in this function, it's presumably safe to
> do
> > so
> > > here as well.
> > 
> > I'm a bit puzzled. I can use ASSERT_TRUE and ASSERT_LT in the methods
> SetupSrtp
> > and SendPackets, but when I try to use it also in SendInvalidSrtpPacket, I
get
> a
> > compilation error 
> > 
> > ../../webrtc/p2p/base/dtlstransportchannel_unittest.cc:327:5: error: cannot
> > initialize return object of type 'int' with an rvalue of type 'void'
> >     ASSERT_LT(channel, channels_.size());
> >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > This makes me think that ASSERT_* actually does *not* work as desired in the
> > other functions, even though the compiler accepts it. My guess is that on
> > failure it will exit the helper function only, but still execute the rest of
> the
> > calling test.
> 
> Yes, on failure ASSERT_* will set some flag and return void; the compiler will
> allow that in any function that returns void. If it's not a top-level test
> function, the caller needs to explicitly check for fatal failures. See
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> 
> And yes, I do believe it's not uncommon for tests to forget to do this, since
> the compiler can't catch it, so it's almost always best to use ASSERT_* only
in
> the top-level test functions.

What do you suggest that I do? 

For the old (and mostly broken) use of ASSERT_*, I could replace by EXPECT_*
where reasonable, or add error propagation in callers. But it seems a bit
independent of the rest of this cl.

For the ASSERT calls I replace, I can either leave them as RTC_CHECK, or use
ASSERT_* + error propagation where they are used.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/turnport_unit...
File webrtc/p2p/base/turnport_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/turnport_unit...
webrtc/p2p/base/turnport_unittest.cc:132: ASSERT_TRUE(conn == connection_);
This could just as well be an EXPECT_TRUE (or EXPECT_EQ).

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/turnport_unit...
webrtc/p2p/base/turnport_unittest.cc:282: ASSERT_TRUE(socket_ != NULL);
While this actually looks like a possibly working use of ASSERT_*, exiting the
current function on failure should probably be enough to not crash the process.

[kwiberg-webrtc, 2017-01-18 09:40:35]

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:304: RTC_CHECK(channel <
channels_.size());
On 2017/01/17 15:00:03, nisse-webrtc wrote:
> On 2017/01/17 13:52:47, kwiberg-webrtc wrote:
> > On 2017/01/17 12:19:21, nisse-webrtc wrote:
> > > On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> > > > Since ASSERT_* is used elsewhere in this function, it's presumably safe
to
> > do
> > > so
> > > > here as well.
> > > 
> > > I'm a bit puzzled. I can use ASSERT_TRUE and ASSERT_LT in the methods
> > SetupSrtp
> > > and SendPackets, but when I try to use it also in SendInvalidSrtpPacket, I
> get
> > a
> > > compilation error 
> > > 
> > > ../../webrtc/p2p/base/dtlstransportchannel_unittest.cc:327:5: error:
cannot
> > > initialize return object of type 'int' with an rvalue of type 'void'
> > >     ASSERT_LT(channel, channels_.size());
> > >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > 
> > > This makes me think that ASSERT_* actually does *not* work as desired in
the
> > > other functions, even though the compiler accepts it. My guess is that on
> > > failure it will exit the helper function only, but still execute the rest
of
> > the
> > > calling test.
> > 
> > Yes, on failure ASSERT_* will set some flag and return void; the compiler
will
> > allow that in any function that returns void. If it's not a top-level test
> > function, the caller needs to explicitly check for fatal failures. See
> >
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> > 
> > And yes, I do believe it's not uncommon for tests to forget to do this,
since
> > the compiler can't catch it, so it's almost always best to use ASSERT_* only
> in
> > the top-level test functions.
> 
> What do you suggest that I do? 
> 
> For the old (and mostly broken) use of ASSERT_*, I could replace by EXPECT_*
> where reasonable, or add error propagation in callers. But it seems a bit
> independent of the rest of this cl.
> 
> For the ASSERT calls I replace, I can either leave them as RTC_CHECK, or use
> ASSERT_* + error propagation where they are used.

My inclination for cases where ASSERT seems like the best choice would be to (1)
use ASSERT if in a top-level test function, (2) use EXCEPT instead when
possible, (3) use CHECK instead if EXCEPT really won't work (e.g. if the test
will crash).

Since the compiler can't enforce the use of (ASSERT|EXPECT)_NO_FATAL_FAILURE, I
would recommend avoiding it entirely; it's too easy to accidentally use ASSERT
in places where the error isn't propagated. (It's irritating that this problem
would have been easy to solve---just make ASSERT_* return a token of some
made-up type instead of void!---but getting that change into gtest is likely to
be hard...)

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/turnport_unit...
File webrtc/p2p/base/turnport_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/turnport_unit...
webrtc/p2p/base/turnport_unittest.cc:132: ASSERT_TRUE(conn == connection_);
On 2017/01/17 15:00:03, nisse-webrtc wrote:
> This could just as well be an EXPECT_TRUE (or EXPECT_EQ).

Acknowledged.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/turnport_unit...
webrtc/p2p/base/turnport_unittest.cc:282: ASSERT_TRUE(socket_ != NULL);
On 2017/01/17 15:00:03, nisse-webrtc wrote:
> While this actually looks like a possibly working use of ASSERT_*, exiting the
> current function on failure should probably be enough to not crash the
process.

Acknowledged.

[nisse-webrtc, 2017-01-18 12:12:31]

Changed the RTC_CHECK(certificate_) check.

If you think this is ok, I'll just revise the description (it's no longer a sed
change) and land.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:304: RTC_CHECK(channel <
channels_.size());
On 2017/01/18 09:40:35, kwiberg-webrtc wrote:
> On 2017/01/17 15:00:03, nisse-webrtc wrote:
> > On 2017/01/17 13:52:47, kwiberg-webrtc wrote:
> > > On 2017/01/17 12:19:21, nisse-webrtc wrote:
> > > > On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> > > > > Since ASSERT_* is used elsewhere in this function, it's presumably
safe
> to
> > > do
> > > > so
> > > > > here as well.
> > > > 
> > > > I'm a bit puzzled. I can use ASSERT_TRUE and ASSERT_LT in the methods
> > > SetupSrtp
> > > > and SendPackets, but when I try to use it also in SendInvalidSrtpPacket,
I
> > get
> > > a
> > > > compilation error 
> > > > 
> > > > ../../webrtc/p2p/base/dtlstransportchannel_unittest.cc:327:5: error:
> cannot
> > > > initialize return object of type 'int' with an rvalue of type 'void'
> > > >     ASSERT_LT(channel, channels_.size());
> > > >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > 
> > > > This makes me think that ASSERT_* actually does *not* work as desired in
> the
> > > > other functions, even though the compiler accepts it. My guess is that
on
> > > > failure it will exit the helper function only, but still execute the
rest
> of
> > > the
> > > > calling test.
> > > 
> > > Yes, on failure ASSERT_* will set some flag and return void; the compiler
> will
> > > allow that in any function that returns void. If it's not a top-level test
> > > function, the caller needs to explicitly check for fatal failures. See
> > >
> >
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> > > 
> > > And yes, I do believe it's not uncommon for tests to forget to do this,
> since
> > > the compiler can't catch it, so it's almost always best to use ASSERT_*
only
> > in
> > > the top-level test functions.
> > 
> > What do you suggest that I do? 
> > 
> > For the old (and mostly broken) use of ASSERT_*, I could replace by EXPECT_*
> > where reasonable, or add error propagation in callers. But it seems a bit
> > independent of the rest of this cl.
> > 
> > For the ASSERT calls I replace, I can either leave them as RTC_CHECK, or use
> > ASSERT_* + error propagation where they are used.
> 
> My inclination for cases where ASSERT seems like the best choice would be to
(1)
> use ASSERT if in a top-level test function, (2) use EXCEPT instead when
> possible, (3) use CHECK instead if EXCEPT really won't work (e.g. if the test
> will crash).
> 
> Since the compiler can't enforce the use of (ASSERT|EXPECT)_NO_FATAL_FAILURE,
I
> would recommend avoiding it entirely; it's too easy to accidentally use ASSERT
> in places where the error isn't propagated. (It's irritating that this problem
> would have been easy to solve---just make ASSERT_* return a token of some
> made-up type instead of void!---but getting that change into gtest is likely
to
> be hard...)

For the channel < channels_.size(), RTC_CHECK then seems appropriate. If the
test fails, the functions will continue with an out-of-bounds access. In
addition, the channel argument originates as a constant in the various test
cases.

For the certificate_ check above, it seems EXPECT works (tested with a hack
setting certificate_ to nullptr). I think we get null certificate passed to
various function, but no crashes.

[kwiberg-webrtc, 2017-01-18 12:35:34]

lgtm, with the commit message changed as you proposed.

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2622413005/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:304: RTC_CHECK(channel <
channels_.size());
On 2017/01/18 12:12:31, nisse-webrtc wrote:
> On 2017/01/18 09:40:35, kwiberg-webrtc wrote:
> > On 2017/01/17 15:00:03, nisse-webrtc wrote:
> > > On 2017/01/17 13:52:47, kwiberg-webrtc wrote:
> > > > On 2017/01/17 12:19:21, nisse-webrtc wrote:
> > > > > On 2017/01/17 09:39:25, kwiberg-webrtc wrote:
> > > > > > Since ASSERT_* is used elsewhere in this function, it's presumably
> safe
> > to
> > > > do
> > > > > so
> > > > > > here as well.
> > > > > 
> > > > > I'm a bit puzzled. I can use ASSERT_TRUE and ASSERT_LT in the methods
> > > > SetupSrtp
> > > > > and SendPackets, but when I try to use it also in
SendInvalidSrtpPacket,
> I
> > > get
> > > > a
> > > > > compilation error 
> > > > > 
> > > > > ../../webrtc/p2p/base/dtlstransportchannel_unittest.cc:327:5: error:
> > cannot
> > > > > initialize return object of type 'int' with an rvalue of type 'void'
> > > > >     ASSERT_LT(channel, channels_.size());
> > > > >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > 
> > > > > This makes me think that ASSERT_* actually does *not* work as desired
in
> > the
> > > > > other functions, even though the compiler accepts it. My guess is that
> on
> > > > > failure it will exit the helper function only, but still execute the
> rest
> > of
> > > > the
> > > > > calling test.
> > > > 
> > > > Yes, on failure ASSERT_* will set some flag and return void; the
compiler
> > will
> > > > allow that in any function that returns void. If it's not a top-level
test
> > > > function, the caller needs to explicitly check for fatal failures. See
> > > >
> > >
> >
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> > > > 
> > > > And yes, I do believe it's not uncommon for tests to forget to do this,
> > since
> > > > the compiler can't catch it, so it's almost always best to use ASSERT_*
> only
> > > in
> > > > the top-level test functions.
> > > 
> > > What do you suggest that I do? 
> > > 
> > > For the old (and mostly broken) use of ASSERT_*, I could replace by
EXPECT_*
> > > where reasonable, or add error propagation in callers. But it seems a bit
> > > independent of the rest of this cl.
> > > 
> > > For the ASSERT calls I replace, I can either leave them as RTC_CHECK, or
use
> > > ASSERT_* + error propagation where they are used.
> > 
> > My inclination for cases where ASSERT seems like the best choice would be to
> (1)
> > use ASSERT if in a top-level test function, (2) use EXCEPT instead when
> > possible, (3) use CHECK instead if EXCEPT really won't work (e.g. if the
test
> > will crash).
> > 
> > Since the compiler can't enforce the use of
(ASSERT|EXPECT)_NO_FATAL_FAILURE,
> I
> > would recommend avoiding it entirely; it's too easy to accidentally use
ASSERT
> > in places where the error isn't propagated. (It's irritating that this
problem
> > would have been easy to solve---just make ASSERT_* return a token of some
> > made-up type instead of void!---but getting that change into gtest is likely
> to
> > be hard...)
> 
> For the channel < channels_.size(), RTC_CHECK then seems appropriate. If the
> test fails, the functions will continue with an out-of-bounds access. In
> addition, the channel argument originates as a constant in the various test
> cases.
> 
> For the certificate_ check above, it seems EXPECT works (tested with a hack
> setting certificate_ to nullptr). I think we get null certificate passed to
> various function, but no crashes.

Acknowledged.

[nisse-webrtc, 2017-01-18 12:49:42]

Description was changed from

==========
Replace ASSERT by RTC_CHECK in test code.

Bulk of the changes were produced using

  git grep -l ' ASSERT(' | grep -v common.h | \
    xargs sed -i 's/ ASSERT(/ RTC_CHECK(/'

followed by additional includes of base/checks.h in affected files,
and git cl format.

BUG=webrtc:6424
==========

to

==========
Replace use of ASSERT by in test code.

In top level test functions, replaced with gtest ASSERT_*. In helper
methods in main test files, replaced with EXPECT_* or RTC_DCHECK on a
case-by-case basis.

In separate mock/fake classes used by tests (which might be of some 
use also in tests of third-party applications), ASSERT was replaced
with RTC_CHECK, using 

  git grep -l ' ASSERT(' | grep -v common.h | \
    xargs sed -i 's/ ASSERT(/ RTC_CHECK(/'

followed by additional includes of base/checks.h in affected files,
and git cl format.

BUG=webrtc:6424
==========

[nisse-webrtc, 2017-01-18 12:50:09]

Description was changed from

==========
Replace use of ASSERT by in test code.

In top level test functions, replaced with gtest ASSERT_*. In helper
methods in main test files, replaced with EXPECT_* or RTC_DCHECK on a
case-by-case basis.

In separate mock/fake classes used by tests (which might be of some 
use also in tests of third-party applications), ASSERT was replaced
with RTC_CHECK, using 

  git grep -l ' ASSERT(' | grep -v common.h | \
    xargs sed -i 's/ ASSERT(/ RTC_CHECK(/'

followed by additional includes of base/checks.h in affected files,
and git cl format.

BUG=webrtc:6424
==========

to

==========
Replace use of ASSERT in test code.

In top level test functions, replaced with gtest ASSERT_*. In helper
methods in main test files, replaced with EXPECT_* or RTC_DCHECK on a
case-by-case basis.

In separate mock/fake classes used by tests (which might be of some 
use also in tests of third-party applications), ASSERT was replaced
with RTC_CHECK, using 

  git grep -l ' ASSERT(' | grep -v common.h | \
    xargs sed -i 's/ ASSERT(/ RTC_CHECK(/'

followed by additional includes of base/checks.h in affected files,
and git cl format.

BUG=webrtc:6424
==========

[nisse-webrtc, 2017-01-18 12:50:29]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2017-01-18 12:50:36]

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

[commit-bot: I haz the power, 2017-01-18 13:02:07]

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

[commit-bot: I haz the power, 2017-01-18 13:02:08]

Try jobs failed on following builders:
  win_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/17777)

[nisse-webrtc, 2017-01-18 14:04:45]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-01-18 14:04:46]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2622413005/#ps80019 (title:
"Fixed another signed/unsigned comparison.")

[commit-bot: I haz the power, 2017-01-18 14:04:50]

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

[commit-bot: I haz the power, 2017-01-18 15:20:56]

CQ is committing da patch.

Bot data: {"patchset_id": 80019, "attempt_start_ts": 1484748285686910,
"parent_rev": "f20dd0014d1cfc8a2e859a9e177e7fe2b21274ca", "commit_rev":
"c8ee882753f5240018445a1ce5761fd1c3196b4e"}

[commit-bot: I haz the power, 2017-01-18 15:21:01]

Description was changed from

==========
Replace use of ASSERT in test code.

In top level test functions, replaced with gtest ASSERT_*. In helper
methods in main test files, replaced with EXPECT_* or RTC_DCHECK on a
case-by-case basis.

In separate mock/fake classes used by tests (which might be of some 
use also in tests of third-party applications), ASSERT was replaced
with RTC_CHECK, using 

  git grep -l ' ASSERT(' | grep -v common.h | \
    xargs sed -i 's/ ASSERT(/ RTC_CHECK(/'

followed by additional includes of base/checks.h in affected files,
and git cl format.

BUG=webrtc:6424
==========

to

==========
Replace use of ASSERT in test code.

In top level test functions, replaced with gtest ASSERT_*. In helper
methods in main test files, replaced with EXPECT_* or RTC_DCHECK on a
case-by-case basis.

In separate mock/fake classes used by tests (which might be of some 
use also in tests of third-party applications), ASSERT was replaced
with RTC_CHECK, using 

  git grep -l ' ASSERT(' | grep -v common.h | \
    xargs sed -i 's/ ASSERT(/ RTC_CHECK(/'

followed by additional includes of base/checks.h in affected files,
and git cl format.

BUG=webrtc:6424

Review-Url: https://codereview.webrtc.org/2622413005
Cr-Commit-Position: refs/heads/master@{#16150}
Committed:
https://chromium.googlesource.com/external/webrtc/+/c8ee882753f5240018445a1ce...
==========

[commit-bot: I haz the power, 2017-01-18 15:21:02]

Committed patchset #6 (id:80019) as
https://chromium.googlesource.com/external/webrtc/+/c8ee882753f5240018445a1ce...