RTCTransportStats added.

webrtc/api/rtcstatscollector_unittest.cc

Index: webrtc/api/rtcstatscollector_unittest.cc
diff --git a/webrtc/api/rtcstatscollector_unittest.cc b/webrtc/api/rtcstatscollector_unittest.cc
index c8eed9febbaed7c84e9411dabfb1173488b6d468..6033de4f5bbbfa9befe8727683cd61e9c3a22824 100644
--- a/webrtc/api/rtcstatscollector_unittest.cc
+++ b/webrtc/api/rtcstatscollector_unittest.cc
@@ -438,6 +438,57 @@ class RTCStatsCollectorTest : public testing::Test {
     }
   }
 
+  void ExpectReportContainsTransportStats(
+      const rtc::scoped_refptr<const RTCStatsReport>& report,
+      const cricket::TransportStats& transport,
+      const CertificateInfo* local_certinfo,
+      const CertificateInfo* remote_certinfo) {
+    const RTCStats* stats = report->Get(
+        "RTCTransport_" + transport.transport_name);
+    EXPECT_TRUE(stats);

[Taylor Brandstetter, 2016/10/17 21:19:03]

Won't the test crash if |stats| isn't found? Seems appropriate to use
ASSERT_TRUE.

[hbos, 2016/10/19 16:30:00]

Done.

+    const RTCTransportStats& transport_stats =
+        stats->cast_to<const RTCTransportStats>();
+    uint64_t bytes_sent = 0;
+    uint64_t bytes_received = 0;
+    const cricket::ConnectionInfo* best_connection_info = nullptr;
+    for (const auto& channel_stats : transport.channel_stats) {
+      for (const cricket::ConnectionInfo& info :
+           channel_stats.connection_infos) {
+        bytes_sent += info.sent_total_bytes;
+        bytes_received += info.recv_total_bytes;
+        if (info.best_connection)
+          best_connection_info = &info;
+      }
+    }

[Taylor Brandstetter, 2016/10/17 21:19:03]

I won't ask you to change everything, since it's so far along, but a general
comment about the test methodology: I think it's better if tests minimize the
amount of code used to determine the expected output.

Here we have "Expect" functions here that essentially convert "cricket::" stats
to stats reports, which is the same thing that the code under test is doing. So
the unit tests verify that the two conversions produce the same result, but both
conversions could be wrong without it being obvious (especially if test code was
produced by copy/pasting implementation code). So an ideal unit test, to me,
would have hard-coded RTCStatsReports that it expects to see, and each of these
"ExpectContains" methods would simply look up the report by ID and do an
EXPECT_EQ.

This also has the advantage that one can look at a unit test and easily see the
expected output for a given input.

[hbos, 2016/10/19 16:30:00]

I think that is a good point.

There's no operator== for RTCStats objects, how about I add it in a follow-up
and do EXPECT_EQ with reports hard-coded in the unittests, removing the
ExpectReportContainsBlahBlah functions? But doing so separately.

[Taylor Brandstetter, 2016/10/19 19:03:06]

Sounds good. Though if that seems like more effort than it's worth, I'd
understand, I leave it to your judgement.

+    EXPECT_EQ(*transport_stats.bytes_sent, bytes_sent);
+    EXPECT_EQ(*transport_stats.bytes_received, bytes_received);
+    if (best_connection_info) {
+      EXPECT_EQ(*transport_stats.active_connection, true);
+      EXPECT_EQ(*transport_stats.selected_candidate_pair_id,
+                "RTCIceCandidatePair_" +
+                best_connection_info->local_candidate.id() + "_" +
+                best_connection_info->remote_candidate.id());
+      EXPECT_TRUE(report->Get(*transport_stats.selected_candidate_pair_id));
+    } else {
+      EXPECT_EQ(*transport_stats.active_connection, false);
+      EXPECT_FALSE(transport_stats.selected_candidate_pair_id.is_defined());
+    }
+    // TODO(hbos): Define transport_stats.rtcp_transport_stats_id.
+    // crbug.com/653873
+    EXPECT_FALSE(transport_stats.rtcp_transport_stats_id.is_defined());
+    if (local_certinfo && remote_certinfo) {
+      EXPECT_EQ(*transport_stats.local_certificate_id,
+                "RTCCertificate_" + local_certinfo->fingerprints[0]);
+      EXPECT_EQ(*transport_stats.remote_certificate_id,
+                "RTCCertificate_" + remote_certinfo->fingerprints[0]);
+      EXPECT_TRUE(report->Get(*transport_stats.local_certificate_id));
+      EXPECT_TRUE(report->Get(*transport_stats.remote_certificate_id));
+    } else {
+      EXPECT_FALSE(transport_stats.local_certificate_id.is_defined());
+      EXPECT_FALSE(transport_stats.remote_certificate_id.is_defined());
+    }
+  }
+
  protected:
   rtc::scoped_refptr<RTCStatsCollectorTestHelper> test_;
   rtc::scoped_refptr<RTCStatsCollector> collector_;
@@ -804,6 +855,93 @@ TEST_F(RTCStatsCollectorTest, CollectRTCPeerConnectionStats) {
   }
 }
 
+TEST_F(RTCStatsCollectorTest, CollectRTCTransportStats) {
+  std::unique_ptr<cricket::Candidate> a_local_candidate = CreateFakeCandidate(
+      "42.42.42.42", 42, "protocol", cricket::LOCAL_PORT_TYPE, 42);
+  std::unique_ptr<cricket::Candidate> a_remote_candidate = CreateFakeCandidate(
+      "42.42.42.42", 42, "protocol", cricket::LOCAL_PORT_TYPE, 42);
+  std::unique_ptr<cricket::Candidate> b_local_candidate = CreateFakeCandidate(
+      "42.42.42.42", 42, "protocol", cricket::LOCAL_PORT_TYPE, 42);
+  std::unique_ptr<cricket::Candidate> b_remote_candidate = CreateFakeCandidate(
+      "42.42.42.42", 42, "protocol", cricket::LOCAL_PORT_TYPE, 42);
+
+  SessionStats session_stats;
+
+  cricket::ConnectionInfo a_connection_info;
+  a_connection_info.best_connection = false;
+  a_connection_info.local_candidate = *a_local_candidate.get();
+  a_connection_info.remote_candidate = *a_remote_candidate.get();
+  a_connection_info.sent_total_bytes = 42;
+  a_connection_info.recv_total_bytes = 1337;
+  cricket::TransportChannelStats a_transport_channel_stats;
+  a_transport_channel_stats.connection_infos.push_back(a_connection_info);
+
+  cricket::ConnectionInfo b_connection_info;
+  b_connection_info.best_connection = false;
+  b_connection_info.local_candidate = *b_local_candidate.get();
+  b_connection_info.remote_candidate = *b_remote_candidate.get();
+  b_connection_info.sent_total_bytes = 1337;
+  b_connection_info.recv_total_bytes = 42;
+  cricket::TransportChannelStats b_transport_channel_stats;
+  b_transport_channel_stats.connection_infos.push_back(b_connection_info);

[Taylor Brandstetter, 2016/10/17 21:19:03]

One of these channel stats should be component RTCP. There will never be two
channels under a transport with the same component (which is a good argument for
making "channel_stats" a map, but it's probably a little late for that).

[hbos, 2016/10/19 16:30:00]

Acknowledged.

+
+  session_stats.transport_stats["transport"].transport_name = "transport";
+  session_stats.transport_stats["transport"].channel_stats.push_back(
+      a_transport_channel_stats);
+  session_stats.transport_stats["transport"].channel_stats.push_back(
+      b_transport_channel_stats);
+
+  // Mock the session to return the desired candidates.
+  EXPECT_CALL(test_->session(), GetTransportStats(_)).WillRepeatedly(Invoke(
+      [this, &session_stats](SessionStats* stats) {
+        *stats = session_stats;
+        return true;
+      }));
+
+  // Get stats without an active connection or certificates.
+  rtc::scoped_refptr<const RTCStatsReport> report = GetStatsReport();
+  ExpectReportContainsTransportStats(
+      report, session_stats.transport_stats["transport"], nullptr, nullptr);
+
+  // Get stats with an active connection.
+  b_connection_info.best_connection = true;
+
+  collector_->ClearCachedStatsReport();
+  report = GetStatsReport();
+  ExpectReportContainsTransportStats(
+      report, session_stats.transport_stats["transport"], nullptr, nullptr);
+
+  // Get stats with certificates.
+  std::unique_ptr<CertificateInfo> local_certinfo =
+      CreateFakeCertificateAndInfoFromDers(
+          std::vector<std::string>({ "(local) local", "(local) chain" }));
+  std::unique_ptr<CertificateInfo> remote_certinfo =
+      CreateFakeCertificateAndInfoFromDers(
+          std::vector<std::string>({ "(remote) local", "(remote) chain" }));
+  EXPECT_CALL(test_->session(), GetLocalCertificate(_, _)).WillRepeatedly(
+    Invoke([this, &local_certinfo](const std::string& transport_name,
+           rtc::scoped_refptr<rtc::RTCCertificate>* certificate) {
+      if (transport_name == "transport") {
+        *certificate = local_certinfo->certificate;
+        return true;
+      }
+      return false;
+    }));
+  EXPECT_CALL(test_->session(),
+      GetRemoteSSLCertificate_ReturnsRawPointer(_)).WillRepeatedly(Invoke(
+      [this, &remote_certinfo](const std::string& transport_name) {
+        if (transport_name == "transport")
+          return remote_certinfo->certificate->ssl_certificate().GetReference();
+        return static_cast<rtc::SSLCertificate*>(nullptr);
+      }));
+
+  collector_->ClearCachedStatsReport();
+  report = GetStatsReport();
+  ExpectReportContainsTransportStats(
+      report, session_stats.transport_stats["transport"],
+      local_certinfo.get(), remote_certinfo.get());
+}
+
 class RTCStatsCollectorTestWithFakeCollector : public testing::Test {
  public:
   RTCStatsCollectorTestWithFakeCollector()