Replace use of ASSERT in test code.

webrtc/p2p/base/dtlstransportchannel_unittest.cc

Index: webrtc/p2p/base/dtlstransportchannel_unittest.cc
diff --git a/webrtc/p2p/base/dtlstransportchannel_unittest.cc b/webrtc/p2p/base/dtlstransportchannel_unittest.cc
index bff2e7da5d27ff18285a402d1f65c78a0a54ea0a..de9c9ed4aa46df6db98fc91df8915e54faef0f5f 100644
--- a/webrtc/p2p/base/dtlstransportchannel_unittest.cc
+++ b/webrtc/p2p/base/dtlstransportchannel_unittest.cc
@@ -14,6 +14,7 @@
 #include "webrtc/p2p/base/dtlstransportchannel.h"
 #include "webrtc/p2p/base/faketransportcontroller.h"
 #include "webrtc/p2p/base/packettransportinterface.h"
+#include "webrtc/base/checks.h"
 #include "webrtc/base/common.h"
 #include "webrtc/base/dscp.h"
 #include "webrtc/base/gunit.h"
@@ -78,7 +79,7 @@ class DtlsTestClient : public sigslot::has_slots<> {
     return certificate_;
   }
   void SetupSrtp() {
-    ASSERT(certificate_);
+    RTC_CHECK(certificate_);
     use_dtls_srtp_ = true;
   }
   void SetupMaxProtocolVersion(rtc::SSLProtocolVersion version) {
@@ -300,7 +301,7 @@ class DtlsTestClient : public sigslot::has_slots<> {
   }
 
   void SendPackets(size_t channel, size_t size, size_t count, bool srtp) {
-    ASSERT(channel < channels_.size());
+    RTC_CHECK(channel < channels_.size());

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

Since ASSERT_* is used elsewhere in this function, it's presumably safe to do so
here as well.

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

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.

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

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.

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

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.

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

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...)

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

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:33]

Acknowledged.

     std::unique_ptr<char[]> packet(new char[size]);
     size_t sent = 0;
     do {
@@ -324,7 +325,7 @@ class DtlsTestClient : public sigslot::has_slots<> {
   }
 
   int SendInvalidSrtpPacket(size_t channel, size_t size) {
-    ASSERT(channel < channels_.size());
+    RTC_CHECK(channel < channels_.size());
     std::unique_ptr<char[]> packet(new char[size]);
     // Fill the packet with 0 to form an invalid SRTP packet.
     memset(packet.get(), 0, size);