RTCTransportStats added.

webrtc/api/rtcstatscollector_unittest.cc

 /*
  *  Copyright 2016 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 420 matching lines @@
       EXPECT_EQ(*cert_stats.base64_certificate, cert_info.pems[i]);
       if (i + 1 < cert_info.fingerprints.size()) {
         EXPECT_EQ(*cert_stats.issuer_certificate_id,
                   "RTCCertificate_" + cert_info.fingerprints[i + 1]);
       } else {
         EXPECT_FALSE(cert_stats.issuer_certificate_id.is_defined());
       }
     }
   }
 
+  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_;
 };
 
 TEST_F(RTCStatsCollectorTest, SingleCallback) {
   rtc::scoped_refptr<const RTCStatsReport> result;
   collector_->GetStatsReport(StatsCallback::Create(&result));
   EXPECT_TRUE_WAIT(result, kGetStatsReportTimeoutMs);
 }
@@ skipping 346 matching lines @@
     // channels that have been opened and closed, not the numbers currently
     // open/closed, we would expect opened >= closed and (opened - closed) to be
     // the number currently open. crbug.com/636818.
     const RTCPeerConnectionStats& pcstats =
         stats->cast_to<RTCPeerConnectionStats>();
     EXPECT_EQ(*pcstats.data_channels_opened, static_cast<uint32_t>(1));
     EXPECT_EQ(*pcstats.data_channels_closed, static_cast<uint32_t>(3));
   }
 }
 
+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()
     : test_(new rtc::RefCountedObject<RTCStatsCollectorTestHelper>()),
       collector_(FakeRTCStatsCollector::Create(
           &test_->pc(), 50 * rtc::kNumMicrosecsPerMillisec)) {
   }
 
  protected:
   rtc::scoped_refptr<RTCStatsCollectorTestHelper> test_;
   rtc::scoped_refptr<FakeRTCStatsCollector> collector_;
 };
 
 TEST_F(RTCStatsCollectorTestWithFakeCollector, ThreadUsageAndResultsMerging) {
   collector_->VerifyThreadUsageAndResultsMerging();
 }
 
 }  // namespace
 
 }  // namespace webrtc