Improve rtcstats_integrationtest.cc by sanity checking values.

Improve rtcstats_integrationtest.cc by sanity checking values.

TestMemberIsPositive and TestMemberIsNonNegative added and used in test.

BUG=chromium:627816
Review-Url: https://codereview.webrtc.org/2593623002
Cr-Commit-Position: refs/heads/master@{#15866}
Committed: https://chromium.googlesource.com/external/webrtc/+/23351197fb981c7a457991fdb16b1a2f35e4bb65

[hbos, 2016-12-20 13:16:45]

The CQ bit was checked by hbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 13:16:52]

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

[hbos, 2016-12-20 13:16:55]

Patchset #1 (id:1) has been deleted

[hbos, 2016-12-20 13:17:15]

hbos@webrtc.org changed reviewers:
+ hta@webrtc.org

[hbos, 2016-12-20 13:17:16]

Please take a look, hta.

[commit-bot: I haz the power, 2016-12-20 14:13:03]

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

[commit-bot: I haz the power, 2016-12-20 14:13:04]

Dry run: This issue passed the CQ dry run.

[hta-webrtc, 2016-12-20 15:19:18]

Testing specific values seems like a Good Thing.

But I'm skeptical to your chosen implementation; it
seems like a lot of complication for not so much value.
Can you do it simply?

https://codereview.webrtc.org/2593623002/diff/20001/webrtc/api/rtcstats_integ...
File webrtc/api/rtcstats_integrationtest.cc (right):

https://codereview.webrtc.org/2593623002/diff/20001/webrtc/api/rtcstats_integ...
webrtc/api/rtcstats_integrationtest.cc:138: TestMemberSign(member, "positive",
[](int sign) { return sign == 1; });
This seems like a huge amount of machinery for saying

EXPECT_TRUE(member.is_defined())
EXPECT_TRUE(member.<suitable_cast>.value() > 0)

[hbos, 2016-12-21 10:37:16]

Patchset #2 (id:40001) has been deleted

[hbos, 2016-12-21 10:37:21]

The CQ bit was checked by hbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 10:37:35]

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

[hbos, 2016-12-21 10:41:14]

PTAL hta, I simplified the implementation.

https://codereview.webrtc.org/2593623002/diff/20001/webrtc/api/rtcstats_integ...
File webrtc/api/rtcstats_integrationtest.cc (right):

https://codereview.webrtc.org/2593623002/diff/20001/webrtc/api/rtcstats_integ...
webrtc/api/rtcstats_integrationtest.cc:138: TestMemberSign(member, "positive",
[](int sign) { return sign == 1; });
On 2016/12/20 15:19:18, hta-webrtc wrote:
> This seems like a huge amount of machinery for saying
> 
> EXPECT_TRUE(member.is_defined())
> EXPECT_TRUE(member.<suitable_cast>.value() > 0)

Not quite, I have to do MarkMemberTested and EXPECT_BLAH with messages for nice
failure messages, but it can be almost that simple with a template to avoid the
switch! Fixed.

https://codereview.webrtc.org/2593623002/diff/60001/webrtc/api/rtcstats_integ...
File webrtc/api/rtcstats_integrationtest.cc (right):

https://codereview.webrtc.org/2593623002/diff/60001/webrtc/api/rtcstats_integ...
webrtc/api/rtcstats_integrationtest.cc:349:
verifier.TestMemberIsNonNegative<uint64_t>(data_channel.bytes_received);
Here and in other places: I'm doing the check even for unsigned values which
can't fail by definition, but I'm thinking it doesn't hurt for clarity.
Otherwise the unsigned should just be TestMemberIsDefined.

[commit-bot: I haz the power, 2016-12-21 11:17:59]

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

[commit-bot: I haz the power, 2016-12-21 11:18:01]

Dry run: This issue passed the CQ dry run.

[hta-webrtc, 2017-01-02 11:32:00]

lgtm

https://codereview.webrtc.org/2593623002/diff/60001/webrtc/api/rtcstats_integ...
File webrtc/api/rtcstats_integrationtest.cc (right):

https://codereview.webrtc.org/2593623002/diff/60001/webrtc/api/rtcstats_integ...
webrtc/api/rtcstats_integrationtest.cc:144: }
I'm not happy with the fact that this repeats code from TestMemberIsDefined, but
you need to do the "return" here, which makes it complex to just call
TestMemberIsDefined. I'll let it pass.

[hbos, 2017-01-02 11:43:27]

The CQ bit was checked by hbos@webrtc.org

[commit-bot: I haz the power, 2017-01-02 11:43:38]

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-02 12:47:20]

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

[commit-bot: I haz the power, 2017-01-02 12:47:21]

Failed to apply patch for webrtc/api/rtcstats_integrationtest.cc:
While running git apply --index -p1;
  error: patch failed: webrtc/api/rtcstats_integrationtest.cc:355
  error: webrtc/api/rtcstats_integrationtest.cc: patch does not apply

Patch:       webrtc/api/rtcstats_integrationtest.cc
Index: webrtc/api/rtcstats_integrationtest.cc
diff --git a/webrtc/api/rtcstats_integrationtest.cc
b/webrtc/api/rtcstats_integrationtest.cc
index
c28130fe8a692558116b401eb2887ef3149454ae..38f7d7c2a2297ccc024614351c9fb697e1030a95
100644
--- a/webrtc/api/rtcstats_integrationtest.cc
+++ b/webrtc/api/rtcstats_integrationtest.cc
@@ -121,18 +121,50 @@ class RTCStatsVerifier {
 
   void TestMemberIsDefined(const RTCStatsMemberInterface& member) {
     EXPECT_TRUE(member.is_defined()) <<
-      stats_->type() << "." << member.name() << "[" << stats_->id() <<
-      "] was undefined.";
+        stats_->type() << "." << member.name() << "[" << stats_->id() <<
+        "] was undefined.";
     MarkMemberTested(member, member.is_defined());
   }
 
   void TestMemberIsUndefined(const RTCStatsMemberInterface& member) {
     EXPECT_FALSE(member.is_defined()) <<
-      stats_->type() << "." << member.name() << "[" << stats_->id() <<
-      "] was defined (" << member.ValueToString() << ").";
+        stats_->type() << "." << member.name() << "[" << stats_->id() <<
+        "] was defined (" << member.ValueToString() << ").";
     MarkMemberTested(member, !member.is_defined());
   }
 
+  template<typename T>
+  void TestMemberIsPositive(const RTCStatsMemberInterface& member) {
+    EXPECT_TRUE(member.is_defined()) <<
+        stats_->type() << "." << member.name() << "[" << stats_->id() <<
+        "] was undefined.";
+    if (!member.is_defined()) {
+      MarkMemberTested(member, false);
+      return;
+    }
+    bool is_positive = *member.cast_to<RTCStatsMember<T>>() > T(0);
+    EXPECT_TRUE(is_positive) <<
+        stats_->type() << "." << member.name() << "[" << stats_->id() <<
+        "] was not positive (" << member.ValueToString() << ").";
+    MarkMemberTested(member, is_positive);
+  }
+
+  template<typename T>
+  void TestMemberIsNonNegative(const RTCStatsMemberInterface& member) {
+    EXPECT_TRUE(member.is_defined()) <<
+        stats_->type() << "." << member.name() << "[" << stats_->id() <<
+        "] was undefined.";
+    if (!member.is_defined()) {
+      MarkMemberTested(member, false);
+      return;
+    }
+    bool is_non_negative = *member.cast_to<RTCStatsMember<T>>() >= T(0);
+    EXPECT_TRUE(is_non_negative) <<
+        stats_->type() << "." << member.name() << "[" << stats_->id() <<
+        "] was not non-negative (" << member.ValueToString() << ").";
+    MarkMemberTested(member, is_non_negative);
+  }
+
   void TestMemberIsIDReference(
       const RTCStatsMemberInterface& member,
       const char* expected_type) {
@@ -297,7 +329,7 @@ class RTCStatsReportVerifier {
     RTCStatsVerifier verifier(report_, &codec);
     verifier.TestMemberIsDefined(codec.payload_type);
     verifier.TestMemberIsDefined(codec.codec);
-    verifier.TestMemberIsDefined(codec.clock_rate);
+    verifier.TestMemberIsPositive<uint32_t>(codec.clock_rate);
     verifier.TestMemberIsUndefined(codec.channels);
     verifier.TestMemberIsUndefined(codec.parameters);
     verifier.TestMemberIsUndefined(codec.implementation);
@@ -311,10 +343,10 @@ class RTCStatsReportVerifier {
     verifier.TestMemberIsDefined(data_channel.protocol);
     verifier.TestMemberIsDefined(data_channel.datachannelid);
     verifier.TestMemberIsDefined(data_channel.state);
-    verifier.TestMemberIsDefined(data_channel.messages_sent);
-    verifier.TestMemberIsDefined(data_channel.bytes_sent);
-    verifier.TestMemberIsDefined(data_channel.messages_received);
-    verifier.TestMemberIsDefined(data_channel.bytes_received);
+    verifier.TestMemberIsNonNegative<uint32_t>(data_channel.messages_sent);
+    verifier.TestMemberIsNonNegative<uint64_t>(data_channel.bytes_sent);
+    verifier.TestMemberIsNonNegative<uint32_t>(data_channel.messages_received);
+    verifier.TestMemberIsNonNegative<uint64_t>(data_channel.bytes_received);
     return verifier.ExpectAllMembersSuccessfullyTested();
   }
 
@@ -332,20 +364,24 @@ class RTCStatsReportVerifier {
     verifier.TestMemberIsUndefined(candidate_pair.nominated);
     verifier.TestMemberIsDefined(candidate_pair.writable);
     verifier.TestMemberIsUndefined(candidate_pair.readable);
-    verifier.TestMemberIsDefined(candidate_pair.bytes_sent);
-    verifier.TestMemberIsDefined(candidate_pair.bytes_received);
+    verifier.TestMemberIsNonNegative<uint64_t>(candidate_pair.bytes_sent);
+    verifier.TestMemberIsNonNegative<uint64_t>(candidate_pair.bytes_received);
     verifier.TestMemberIsUndefined(candidate_pair.total_round_trip_time);
-    verifier.TestMemberIsDefined(candidate_pair.current_round_trip_time);
+    verifier.TestMemberIsNonNegative<double>(
+        candidate_pair.current_round_trip_time);
     verifier.TestMemberIsUndefined(candidate_pair.available_outgoing_bitrate);
     verifier.TestMemberIsUndefined(candidate_pair.available_incoming_bitrate);
-    verifier.TestMemberIsDefined(candidate_pair.requests_received);
-    verifier.TestMemberIsDefined(candidate_pair.requests_sent);
-    verifier.TestMemberIsDefined(candidate_pair.responses_received);
-    verifier.TestMemberIsDefined(candidate_pair.responses_sent);
+    verifier.TestMemberIsNonNegative<uint64_t>(
+        candidate_pair.requests_received);
+    verifier.TestMemberIsNonNegative<uint64_t>(candidate_pair.requests_sent);
+    verifier.TestMemberIsNonNegative<uint64_t>(
+        candidate_pair.responses_received);
+    verifier.TestMemberIsNonNegative<uint64_t>(candidate_pair.responses_sent);
     verifier.TestMemberIsUndefined(candidate_pair.retransmissions_received);
     verifier.TestMemberIsUndefined(candidate_pair.retransmissions_sent);
     verifier.TestMemberIsUndefined(candidate_pair.consent_requests_received);
-    verifier.TestMemberIsDefined(candidate_pair.consent_requests_sent);
+    verifier.TestMemberIsNonNegative<uint64_t>(
+        candidate_pair.consent_requests_sent);
     verifier.TestMemberIsUndefined(candidate_pair.consent_responses_received);
     verifier.TestMemberIsUndefined(candidate_pair.consent_responses_sent);
     return verifier.ExpectAllMembersSuccessfullyTested();
@@ -355,10 +391,10 @@ class RTCStatsReportVerifier {
       const RTCIceCandidateStats& candidate) {
     RTCStatsVerifier verifier(report_, &candidate);
     verifier.TestMemberIsDefined(candidate.ip);
-    verifier.TestMemberIsDefined(candidate.port);
+    verifier.TestMemberIsNonNegative<int32_t>(candidate.port);
     verifier.TestMemberIsDefined(candidate.protocol);
     verifier.TestMemberIsDefined(candidate.candidate_type);
-    verifier.TestMemberIsDefined(candidate.priority);
+    verifier.TestMemberIsNonNegative<int32_t>(candidate.priority);
     verifier.TestMemberIsUndefined(candidate.url);
     return verifier.ExpectAllMembersSuccessfullyTested();
   }
@@ -393,8 +429,10 @@ class RTCStatsReportVerifier {
     // Video or audio media stream track?
     if (media_stream_track.frame_width.is_defined()) {
       // Video-only members
-      verifier.TestMemberIsDefined(media_stream_track.frame_width);
-      verifier.TestMemberIsDefined(media_stream_track.frame_height);
+      verifier.TestMemberIsNonNegative<uint32_t>(
+          media_stream_track.frame_width);
+      verifier.TestMemberIsNonNegative<uint32_t>(
+          media_stream_track.frame_height);
       verifier.TestMemberIsUndefined(media_stream_track.frames_per_second);
       verifier.TestMemberIsUndefined(media_stream_track.frames_sent);
       verifier.TestMemberIsUndefined(media_stream_track.frames_received);
@@ -436,8 +474,10 @@ class RTCStatsReportVerifier {
   bool VerifyRTCPeerConnectionStats(
       const RTCPeerConnectionStats& peer_connection) {
     RTCStatsVerifier verifier(report_, &peer_connection);
-    verifier.TestMemberIsDefined(peer_connection.data_channels_opened);
-    verifier.TestMemberIsDefined(peer_connection.data_channels_closed);
+    verifier.TestMemberIsNonNegative<uint32_t>(
+        peer_connection.data_channels_opened);
+    verifier.TestMemberIsNonNegative<uint32_t>(
+        peer_connection.data_channels_closed);
     return verifier.ExpectAllMembersSuccessfullyTested();
   }
 
@@ -452,9 +492,9 @@ class RTCStatsReportVerifier {
         stream.transport_id, RTCTransportStats::kType);
     verifier->TestMemberIsIDReference(stream.codec_id, RTCCodecStats::kType);
     if (stream.media_type.is_defined() && *stream.media_type == "video") {
-      verifier->TestMemberIsDefined(stream.fir_count);
-      verifier->TestMemberIsDefined(stream.pli_count);
-      verifier->TestMemberIsDefined(stream.nack_count);
+      verifier->TestMemberIsNonNegative<uint32_t>(stream.fir_count);
+      verifier->TestMemberIsNonNegative<uint32_t>(stream.pli_count);
+      verifier->TestMemberIsNonNegative<uint32_t>(stream.nack_count);
     } else {
       verifier->TestMemberIsUndefined(stream.fir_count);
       verifier->TestMemberIsUndefined(stream.pli_count);
@@ -467,16 +507,16 @@ class RTCStatsReportVerifier {
       const RTCInboundRTPStreamStats& inbound_stream) {
     RTCStatsVerifier verifier(report_, &inbound_stream);
     VerifyRTCRTPStreamStats(inbound_stream, &verifier);
-    verifier.TestMemberIsDefined(inbound_stream.packets_received);
-    verifier.TestMemberIsDefined(inbound_stream.bytes_received);
-    verifier.TestMemberIsDefined(inbound_stream.packets_lost);
+   
verifier.TestMemberIsNonNegative<uint32_t>(inbound_stream.packets_received);
+    verifier.TestMemberIsNonNegative<uint64_t>(inbound_stream.bytes_received);
+    verifier.TestMemberIsNonNegative<uint32_t>(inbound_stream.packets_lost);
     if (inbound_stream.media_type.is_defined() &&
         *inbound_stream.media_type == "video") {
       verifier.TestMemberIsUndefined(inbound_strea…
(message too large)

[hbos, 2017-01-02 13:48:25]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2017-01-02 13:48:25]

The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2593623002/#ps80001 (title:
"Rebase with master")

[commit-bot: I haz the power, 2017-01-02 13:48:35]

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-02 14:52:20]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483364905206470,
"parent_rev": "9baddf25b9cb4b8c4d0813c9982dae9425d6fa98", "commit_rev":
"23351197fb981c7a457991fdb16b1a2f35e4bb65"}

[commit-bot: I haz the power, 2017-01-02 14:52:22]

Description was changed from

==========
Improve rtcstats_integrationtest.cc by sanity checking values.

TestMemberIsPositive and TestMemberIsNonNegative added and used in test.

BUG=chromium:627816
==========

to

==========
Improve rtcstats_integrationtest.cc by sanity checking values.

TestMemberIsPositive and TestMemberIsNonNegative added and used in test.

BUG=chromium:627816

Review-Url: https://codereview.webrtc.org/2593623002
Cr-Commit-Position: refs/heads/master@{#15866}
Committed:
https://chromium.googlesource.com/external/webrtc/+/23351197fb981c7a457991fdb...
==========

[commit-bot: I haz the power, 2017-01-02 14:52:22]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/23351197fb981c7a457991fdb...