Fixing flaky DtmfSenderTest by using fake clock.

webrtc/api/dtmfsender_unittest.cc

Index: webrtc/api/dtmfsender_unittest.cc
diff --git a/webrtc/api/dtmfsender_unittest.cc b/webrtc/api/dtmfsender_unittest.cc
index e5fe26171b504701692fe6296930caa5a77acead..69bee90c180cec78a8ca8e0c5164c2aec0d2a30c 100644
--- a/webrtc/api/dtmfsender_unittest.cc
+++ b/webrtc/api/dtmfsender_unittest.cc
@@ -16,6 +16,7 @@
 #include <vector>
 
 #include "webrtc/api/audiotrack.h"
+#include "webrtc/base/fakeclock.h"
 #include "webrtc/base/gunit.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/timeutils.h"
@@ -27,6 +28,9 @@ using webrtc::DtmfSender;
 using webrtc::DtmfSenderObserverInterface;
 
 static const char kTestAudioLabel[] = "test_audio_track";
+// TODO(deadbeef): Even though this test now uses a fake clock, it has a
+// generous 3-second timeout for every test case. The timeout could be tuned
+// to each test based on the tones sent, instead.

[honghaiz3, 2016/10/28 17:23:26]

Is there a problem if we change it in this CL?

[Taylor Brandstetter, 2016/10/28 20:31:23]

It just seemed like more work than necessary at the moment. The goal here is
just to fix the flakiness of the test, not make the test do more than it did
originally.

 static const int kMaxWaitMs = 3000;
 
 class FakeDtmfObserver : public DtmfSenderObserverInterface {
@@ -191,9 +195,9 @@ class DtmfSenderTest : public testing::Test {
     while (it_ref != dtmf_queue_ref.end() && it != dtmf_queue.end()) {
       EXPECT_EQ(it_ref->code, it->code);
       EXPECT_EQ(it_ref->duration, it->duration);
-      // Allow ~100ms error.
-      EXPECT_GE(it_ref->gap, it->gap - 100);
-      EXPECT_LE(it_ref->gap, it->gap + 100);
+      // Allow ~10ms error (can be small since we're using a fake clock).
+      EXPECT_GE(it_ref->gap, it->gap - 10);
+      EXPECT_LE(it_ref->gap, it->gap + 10);
       ++it_ref;
       ++it;
     }
@@ -218,6 +222,7 @@ class DtmfSenderTest : public testing::Test {
   std::unique_ptr<FakeDtmfObserver> observer_;
   std::unique_ptr<FakeDtmfProvider> provider_;
   rtc::scoped_refptr<DtmfSender> dtmf_;
+  rtc::ScopedFakeClock fake_clock_;
 };
 
 TEST_F(DtmfSenderTest, CanInsertDtmf) {
@@ -231,7 +236,7 @@ TEST_F(DtmfSenderTest, InsertDtmf) {
   int duration = 100;
   int inter_tone_gap = 50;
   EXPECT_TRUE(dtmf_->InsertDtmf(tones, duration, inter_tone_gap));
-  EXPECT_TRUE_WAIT(observer_->completed(), kMaxWaitMs);
+  EXPECT_TRUE_SIMULATED_WAIT(observer_->completed(), kMaxWaitMs, fake_clock_);
 
   // The unrecognized characters should be ignored.
   std::string known_tones = "1a*";
@@ -247,13 +252,14 @@ TEST_F(DtmfSenderTest, InsertDtmfTwice) {
   EXPECT_TRUE(dtmf_->InsertDtmf(tones1, duration, inter_tone_gap));
   VerifyExpectedState(track_, tones1, duration, inter_tone_gap);
   // Wait until the first tone got sent.
-  EXPECT_TRUE_WAIT(observer_->tones().size() == 1, kMaxWaitMs);
+  EXPECT_TRUE_SIMULATED_WAIT(observer_->tones().size() == 1, kMaxWaitMs,
+                             fake_clock_);
   VerifyExpectedState(track_, "2", duration, inter_tone_gap);
   // Insert with another tone buffer.
   EXPECT_TRUE(dtmf_->InsertDtmf(tones2, duration, inter_tone_gap));
   VerifyExpectedState(track_, tones2, duration, inter_tone_gap);
   // Wait until it's completed.
-  EXPECT_TRUE_WAIT(observer_->completed(), kMaxWaitMs);
+  EXPECT_TRUE_SIMULATED_WAIT(observer_->completed(), kMaxWaitMs, fake_clock_);
 
   std::vector<FakeDtmfProvider::DtmfInfo> dtmf_queue_ref;
   GetDtmfInfoFromString("1", duration, inter_tone_gap, &dtmf_queue_ref);
@@ -268,7 +274,8 @@ TEST_F(DtmfSenderTest, InsertDtmfWhileProviderIsDeleted) {
   int inter_tone_gap = 50;
   EXPECT_TRUE(dtmf_->InsertDtmf(tones, duration, inter_tone_gap));
   // Wait until the first tone got sent.
-  EXPECT_TRUE_WAIT(observer_->tones().size() == 1, kMaxWaitMs);
+  EXPECT_TRUE_SIMULATED_WAIT(observer_->tones().size() == 1, kMaxWaitMs,
+                             fake_clock_);
   // Delete provider.
   provider_.reset();
   // The queue should be discontinued so no more tone callbacks.
@@ -282,7 +289,8 @@ TEST_F(DtmfSenderTest, InsertDtmfWhileSenderIsDeleted) {
   int inter_tone_gap = 50;
   EXPECT_TRUE(dtmf_->InsertDtmf(tones, duration, inter_tone_gap));
   // Wait until the first tone got sent.
-  EXPECT_TRUE_WAIT(observer_->tones().size() == 1, kMaxWaitMs);
+  EXPECT_TRUE_SIMULATED_WAIT(observer_->tones().size() == 1, kMaxWaitMs,
+                             fake_clock_);
   // Delete the sender.
   dtmf_ = NULL;
   // The queue should be discontinued so no more tone callbacks.
@@ -297,11 +305,12 @@ TEST_F(DtmfSenderTest, InsertEmptyTonesToCancelPreviousTask) {
   int inter_tone_gap = 50;
   EXPECT_TRUE(dtmf_->InsertDtmf(tones1, duration, inter_tone_gap));
   // Wait until the first tone got sent.
-  EXPECT_TRUE_WAIT(observer_->tones().size() == 1, kMaxWaitMs);
+  EXPECT_TRUE_SIMULATED_WAIT(observer_->tones().size() == 1, kMaxWaitMs,
+                             fake_clock_);
   // Insert with another tone buffer.
   EXPECT_TRUE(dtmf_->InsertDtmf(tones2, duration, inter_tone_gap));
   // Wait until it's completed.
-  EXPECT_TRUE_WAIT(observer_->completed(), kMaxWaitMs);
+  EXPECT_TRUE_SIMULATED_WAIT(observer_->completed(), kMaxWaitMs, fake_clock_);
 
   std::vector<FakeDtmfProvider::DtmfInfo> dtmf_queue_ref;
   GetDtmfInfoFromString("1", duration, inter_tone_gap, &dtmf_queue_ref);
@@ -309,14 +318,12 @@ TEST_F(DtmfSenderTest, InsertEmptyTonesToCancelPreviousTask) {
   VerifyOnObserver("1");
 }
 
-// Flaky when run in parallel.
-// See https://code.google.com/p/webrtc/issues/detail?id=4219.
-TEST_F(DtmfSenderTest, DISABLED_InsertDtmfWithCommaAsDelay) {
+TEST_F(DtmfSenderTest, InsertDtmfWithCommaAsDelay) {
   std::string tones = "3,4";
   int duration = 100;
   int inter_tone_gap = 50;
   EXPECT_TRUE(dtmf_->InsertDtmf(tones, duration, inter_tone_gap));
-  EXPECT_TRUE_WAIT(observer_->completed(), kMaxWaitMs);
+  EXPECT_TRUE_SIMULATED_WAIT(observer_->completed(), kMaxWaitMs, fake_clock_);
 
   VerifyOnProvider(tones, duration, inter_tone_gap);
   VerifyOnObserver(tones);