RTCStatsCollector collecting stats on multiple threads.

webrtc/stats/rtcstatscollector_unittest.cc

Index: webrtc/stats/rtcstatscollector_unittest.cc
diff --git a/webrtc/stats/rtcstatscollector_unittest.cc b/webrtc/stats/rtcstatscollector_unittest.cc
index a3b0572c0756f0ee2630ffaaf68b9026b8aa3210..1ee1e7c46fcea8111cc9ecd40f947ada1b9e0948 100644
--- a/webrtc/stats/rtcstatscollector_unittest.cc
+++ b/webrtc/stats/rtcstatscollector_unittest.cc
@@ -24,6 +24,7 @@
 #include "webrtc/base/gunit.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/test/faketiming.h"
+#include "webrtc/base/thread_checker.h"
 #include "webrtc/media/base/fakemediaengine.h"
 
 using testing::Return;
@@ -31,6 +32,10 @@ using testing::ReturnRef;
 
 namespace webrtc {
 
+namespace {
+
+const int64_t kGetStatsTimeoutMs = 1000;
+
 class RTCStatsCollectorTester : public SetSessionDescriptionObserver {
  public:
   RTCStatsCollectorTester()
@@ -74,40 +79,240 @@ class RTCStatsCollectorTester : public SetSessionDescriptionObserver {
   std::vector<rtc::scoped_refptr<DataChannel>> data_channels_;
 };
 
+class RTCTestStats : public RTCStats {
+ public:
+  RTCTestStats(const std::string& id, double timestamp)
+      : RTCStats(id, timestamp),
+        dummy_stat("dummyStat") {}
+
+  WEBRTC_RTCSTATS_IMPL(RTCStats, RTCTestStats,
+      &dummy_stat);
+
+  RTCStatsMember<int32_t> dummy_stat;
+};
+
+const char RTCTestStats::kType[] = "test-stats";
+
+// Overrides the stats collection to verify thread usage and that the resulting
+// partial reports are merged.
+class FakeRTCStatsCollector : public RTCStatsCollector,
+                              public RTCStatsCollectorCallback {

[hta-webrtc, 2016/08/31 09:57:36]

If you're faking the implementation under test, that's not a good sign. Is there
a way to test the implementation you're testing (RTCStatsCollector) without
subclassing it?

If not, your class may fail the "testability" sniff test.

[hbos, 2016/09/01 14:16:16]

I wanted to test the threaded aspects of the class as a separate test
("distribute work on different threads, merge the results") without relying on
real stats collection to do so. Currently there are only real stats gathered on
the signaling thread, nothing on the worker or network threads. If we had stats
gathered on all threads, then the tests for those stats would implicitly test
this functionality.

I could put the "distribute work on different threads, merge the results"
functionality into a separate helper class and test that separately, but that
just seems like work-around to avoid the "Fake" keyword.

+ public:
+  static rtc::scoped_refptr<FakeRTCStatsCollector> Create(
+      PeerConnection* pc,
+      double cache_lifetime,
+      std::unique_ptr<rtc::Timing> timing) {
+    return rtc::scoped_refptr<FakeRTCStatsCollector>(
+        new rtc::RefCountedObject<FakeRTCStatsCollector>(
+            pc, cache_lifetime, timing.release()));
+  }
+
+  void OnStatsDelivered(
+      const rtc::scoped_refptr<const RTCStatsReport>& report) override {
+    EXPECT_TRUE(signaling_thread_->IsCurrent());
+    rtc::CritScope cs(&lock_);
+    delivered_report_ = report;
+  }
+
+  bool IsStatsDelivered() {
+    EXPECT_TRUE(signaling_thread_->IsCurrent());
+    rtc::CritScope cs(&lock_);
+    if (!delivered_report_)
+      return false;
+    EXPECT_EQ(produced_on_signaling_thread_, 1);
+    EXPECT_EQ(produced_on_worker_thread_, 1);
+    EXPECT_EQ(produced_on_network_thread_, 1);
+
+    EXPECT_TRUE(delivered_report_->Get("SignalingThreadStats"));
+    EXPECT_TRUE(delivered_report_->Get("WorkerThreadStats"));
+    EXPECT_TRUE(delivered_report_->Get("NetworkThreadStats"));
+
+    produced_on_signaling_thread_ = 0;
+    produced_on_worker_thread_ = 0;
+    produced_on_network_thread_ = 0;
+    delivered_report_ = nullptr;
+    return true;
+  }
+
+ protected:
+  FakeRTCStatsCollector(
+      PeerConnection* pc,
+      double cache_lifetime,
+      rtc::Timing* timing)
+      : RTCStatsCollector(pc, cache_lifetime, timing),
+        signaling_thread_(pc->session()->signaling_thread()),
+        worker_thread_(pc->session()->worker_thread()),
+        network_thread_(pc->session()->network_thread()) {
+  }
+
+  void ProducePartialResultsOnSignalingThread_s(double timestamp) override {
+    EXPECT_TRUE(signaling_thread_->IsCurrent());
+    {
+      rtc::CritScope cs(&lock_);
+      EXPECT_FALSE(delivered_report_);
+      ++produced_on_signaling_thread_;
+    }
+
+    rtc::scoped_refptr<RTCStatsReport> signaling_report =
+        RTCStatsReport::Create();
+    signaling_report->AddStats(std::unique_ptr<const RTCStats>(
+        new RTCTestStats("SignalingThreadStats", timestamp)));
+    AddPartialResults(signaling_report, timestamp);
+  }
+  void ProducePartialResultsOnWorkerThread_w(double timestamp) override {
+    EXPECT_TRUE(worker_thread_->IsCurrent());
+    {
+      rtc::CritScope cs(&lock_);
+      EXPECT_FALSE(delivered_report_);
+      ++produced_on_worker_thread_;
+    }
+
+    rtc::scoped_refptr<RTCStatsReport> worker_report = RTCStatsReport::Create();
+    worker_report->AddStats(std::unique_ptr<const RTCStats>(
+        new RTCTestStats("WorkerThreadStats", timestamp)));
+    AddPartialResults(worker_report, timestamp);
+  }
+  void ProducePartialResultsOnNetworkThread_n(double timestamp) override {
+    EXPECT_TRUE(network_thread_->IsCurrent());
+    {
+      rtc::CritScope cs(&lock_);
+      EXPECT_FALSE(delivered_report_);
+      ++produced_on_network_thread_;
+    }
+
+    rtc::scoped_refptr<RTCStatsReport> network_report =
+        RTCStatsReport::Create();
+    network_report->AddStats(std::unique_ptr<const RTCStats>(
+        new RTCTestStats("NetworkThreadStats", timestamp)));
+    AddPartialResults(network_report, timestamp);
+  }
+
+ private:
+  rtc::Thread* const signaling_thread_;
+  rtc::Thread* const worker_thread_;
+  rtc::Thread* const network_thread_;
+
+  rtc::CriticalSection lock_;
+  rtc::scoped_refptr<const RTCStatsReport> delivered_report_;
+  int produced_on_signaling_thread_ = 0;
+  int produced_on_worker_thread_ = 0;
+  int produced_on_network_thread_ = 0;
+};
+
+class StatsCallback : public RTCStatsCollectorCallback {
+ public:
+  static rtc::scoped_refptr<StatsCallback> Create(
+      rtc::scoped_refptr<const RTCStatsReport>* report_ptr = nullptr) {
+    return rtc::scoped_refptr<StatsCallback>(
+        new rtc::RefCountedObject<StatsCallback>(report_ptr));
+  }
+
+  void OnStatsDelivered(
+      const rtc::scoped_refptr<const RTCStatsReport>& report) override {
+    EXPECT_TRUE(thread_checker_.CalledOnValidThread());
+    report_ = report;
+    if (report_ptr_)
+      *report_ptr_ = report_;
+  }
+
+  rtc::scoped_refptr<const RTCStatsReport> report() const {
+    EXPECT_TRUE(thread_checker_.CalledOnValidThread());
+    return report_;
+  }
+
+ protected:
+  explicit StatsCallback(rtc::scoped_refptr<const RTCStatsReport>* report_ptr)
+      : report_ptr_(report_ptr) {}
+
+ private:
+  rtc::ThreadChecker thread_checker_;
+  rtc::scoped_refptr<const RTCStatsReport> report_;
+  rtc::scoped_refptr<const RTCStatsReport>* report_ptr_;
+};
+
+}  // namespace
+
 class RTCStatsCollectorTest : public testing::Test {
  public:
+  static const bool kUseRealCollector = false;
+  static const bool kUseFakeCollector = true;
+
   RTCStatsCollectorTest()
     : test_(new rtc::RefCountedObject<RTCStatsCollectorTester>()),
-      timing_(new rtc::FakeTiming()),
-      collector_(&test_->pc(), 0.05, std::unique_ptr<rtc::Timing>(timing_)) {
+      timing_(new rtc::FakeTiming()) {
+  }
+
+  void InitCollector(bool fake_collector) {

[hta-webrtc, 2016/08/31 09:57:36]

Not a good code smell. If you really need to have fake_collector, make two
functions: InitCollector and InitFakeCollector.

or even better: Write two test fixture classes - RTCStatsCollectorTest and
RTCStatsCollectorTestWithFakeCollector. The second one can subclass the first
one to reduce code duplication. New test cases can be added to the appropriate
test set, and all those that don't care about the threading model can just go
into the RTCStatsCollectorTest class.

Simple things should be simple.

[hbos, 2016/09/01 14:16:16]

Done.

+    EXPECT_FALSE(collector_);
+    fake_collector_ = fake_collector;
+    if (!fake_collector_) {
+      collector_ = RTCStatsCollector::Create(
+          &test_->pc(), 0.05, std::unique_ptr<rtc::Timing>(timing_));
+    } else {
+      rtc::scoped_refptr<FakeRTCStatsCollector> fake_collector =
+          FakeRTCStatsCollector::Create(
+              &test_->pc(), 0.05, std::unique_ptr<rtc::Timing>(timing_));
+      collector_ = fake_collector;
+    }
+  }
+
+  FakeRTCStatsCollector* fake_collector() const {
+    EXPECT_TRUE(fake_collector_);
+    return static_cast<FakeRTCStatsCollector*>(collector_.get());
+  }
+
+  rtc::scoped_refptr<const RTCStatsReport> GetStatsReport() {
+    EXPECT_TRUE(collector_);
+    rtc::scoped_refptr<StatsCallback> callback = StatsCallback::Create();
+    collector_->GetStatsReport(callback);
+    EXPECT_TRUE_WAIT(callback->report(), kGetStatsTimeoutMs);
+    return callback->report();
   }
 
  protected:
   rtc::scoped_refptr<RTCStatsCollectorTester> test_;
   rtc::FakeTiming* timing_;  // Owned by |collector_|.
-  RTCStatsCollector collector_;
+  rtc::scoped_refptr<RTCStatsCollector> collector_;
+  bool fake_collector_ = false;
 };
 
+// With a fake collector verify that stats are collected on all threads and that
+// the partial results are assembled into a final report with all results.
+TEST_F(RTCStatsCollectorTest, VerifyThreadUsageAndMergedResults) {
+  InitCollector(kUseFakeCollector);
+  collector_->GetStatsReport(fake_collector());
+  // |IsStatsDelivered| also verifies the results.
+  EXPECT_TRUE_WAIT(fake_collector()->IsStatsDelivered(), kGetStatsTimeoutMs);
+}
+
+// Verify that the callback is kept alive and still invoked even if the only
+// reference to it is by |GetStatsReport|.
+TEST_F(RTCStatsCollectorTest, CallbackNoExternalReferences) {
+  InitCollector(kUseFakeCollector);
+  rtc::scoped_refptr<const RTCStatsReport> result;
+  collector_->GetStatsReport(StatsCallback::Create(&result));
+  EXPECT_TRUE_WAIT(result, kGetStatsTimeoutMs);
+}

[hta-webrtc, 2016/08/31 09:57:36]

Please add a test that calls GetStatsReport() twice in succession (with no
waits), and verify that both callbacks get called.

For extra coverage, call GetStatsReport(), increment the (fake) time by 100 msec
(so that the collector will assume that the first result is stale), call
GetStatsReport again, and see that both callbacks get called. Whether the two
results should have the same timestamp or not is a good question; whether the
result will be the same across 1000 test runs is an even better one.

[hbos, 2016/09/01 14:16:16]

Done. The FakeClock unfortunately processes messages when you advance time. This
means both that stats gathering is completed and that the cache is invalidated
as a result of advancing "cache lifetime" time. (This caused me a great deal of
confusion.)

+
 TEST_F(RTCStatsCollectorTest, CachedStatsReport) {
+  InitCollector(kUseRealCollector);
   // Caching should ensure |a| and |b| are the same report.
-  rtc::scoped_refptr<const RTCStatsReport> a = collector_.GetStatsReport();
-  rtc::scoped_refptr<const RTCStatsReport> b = collector_.GetStatsReport();
-  EXPECT_TRUE(a);
+  rtc::scoped_refptr<const RTCStatsReport> a = GetStatsReport();
+  rtc::scoped_refptr<const RTCStatsReport> b = GetStatsReport();
   EXPECT_EQ(a.get(), b.get());
   // Invalidate cache by clearing it.
-  collector_.ClearCachedStatsReport();
-  rtc::scoped_refptr<const RTCStatsReport> c = collector_.GetStatsReport();
-  EXPECT_TRUE(c);
+  collector_->ClearCachedStatsReport();
+  rtc::scoped_refptr<const RTCStatsReport> c = GetStatsReport();
   EXPECT_NE(b.get(), c.get());
   // Invalidate cache by advancing time.
   timing_->AdvanceTimeMillisecs(51.0);
-  rtc::scoped_refptr<const RTCStatsReport> d = collector_.GetStatsReport();
-  EXPECT_TRUE(d);
+  rtc::scoped_refptr<const RTCStatsReport> d = GetStatsReport();
   EXPECT_NE(c.get(), d.get());
 }
 
 TEST_F(RTCStatsCollectorTest, CollectRTCPeerConnectionStats) {
-  rtc::scoped_refptr<const RTCStatsReport> report = collector_.GetStatsReport();
+  InitCollector(kUseRealCollector);
+  rtc::scoped_refptr<const RTCStatsReport> report = GetStatsReport();
   EXPECT_EQ(report->GetStatsOfType<RTCPeerConnectionStats>().size(),
             static_cast<size_t>(1)) << "Expecting 1 RTCPeerConnectionStats.";
   const RTCStats* stats = report->Get("RTCPeerConnection");
@@ -130,8 +335,8 @@ TEST_F(RTCStatsCollectorTest, CollectRTCPeerConnectionStats) {
   test_->data_channels().push_back(
       new MockDataChannel(DataChannelInterface::kClosed));
 
-  collector_.ClearCachedStatsReport();
-  report = collector_.GetStatsReport();
+  collector_->ClearCachedStatsReport();
+  report = GetStatsReport();
   EXPECT_EQ(report->GetStatsOfType<RTCPeerConnectionStats>().size(),
             static_cast<size_t>(1)) << "Expecting 1 RTCPeerConnectionStats.";
   stats = report->Get("RTCPeerConnection");