Fixing flaky DtmfSenderTest by using fake clock.

webrtc/api/dtmfsender_unittest.cc

 /*
  *  Copyright 2012 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.
  */
 
 #include "webrtc/api/dtmfsender.h"
 
 #include <memory>
 #include <set>
 #include <string>
 #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"
 
 using webrtc::AudioTrackInterface;
 using webrtc::AudioTrack;
 using webrtc::DtmfProviderInterface;
 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 {
  public:
   FakeDtmfObserver() : completed_(false) {}
 
   // Implements DtmfSenderObserverInterface.
   void OnToneChange(const std::string& tone) override {
     LOG(LS_VERBOSE) << "FakeDtmfObserver::OnToneChange '" << tone << "'.";
     tones_.push_back(tone);
@@ skipping 144 matching lines @@
     const std::vector<FakeDtmfProvider::DtmfInfo>& dtmf_queue =
         provider_->dtmf_info_queue();
     ASSERT_EQ(dtmf_queue_ref.size(), dtmf_queue.size());
     std::vector<FakeDtmfProvider::DtmfInfo>::const_iterator it_ref =
         dtmf_queue_ref.begin();
     std::vector<FakeDtmfProvider::DtmfInfo>::const_iterator it =
         dtmf_queue.begin();
     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;
     }
   }
 
   // Verify the observer got all the expected callbacks.
   void VerifyOnObserver(const std::string& tones_ref) {
     const std::vector<std::string>& tones = observer_->tones();
     // The observer will get an empty string at the end.
     EXPECT_EQ(tones_ref.size() + 1, tones.size());
     EXPECT_TRUE(tones.back().empty());
     std::string::const_iterator it_ref = tones_ref.begin();
     std::vector<std::string>::const_iterator it = tones.begin();
     while (it_ref != tones_ref.end() && it != tones.end()) {
       EXPECT_EQ(*it_ref, it->at(0));
       ++it_ref;
       ++it;
     }
   }
 
   rtc::scoped_refptr<AudioTrackInterface> track_;
   std::unique_ptr<FakeDtmfObserver> observer_;
   std::unique_ptr<FakeDtmfProvider> provider_;
   rtc::scoped_refptr<DtmfSender> dtmf_;
+  rtc::ScopedFakeClock fake_clock_;
 };
 
 TEST_F(DtmfSenderTest, CanInsertDtmf) {
   EXPECT_TRUE(dtmf_->CanInsertDtmf());
   provider_->RemoveCanInsertDtmfTrack(kTestAudioLabel);
   EXPECT_FALSE(dtmf_->CanInsertDtmf());
 }
 
 TEST_F(DtmfSenderTest, InsertDtmf) {
   std::string tones = "@1%a&*$";
   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*";
   VerifyOnProvider(known_tones, duration, inter_tone_gap);
   VerifyOnObserver(known_tones);
 }
 
 TEST_F(DtmfSenderTest, InsertDtmfTwice) {
   std::string tones1 = "12";
   std::string tones2 = "ab";
   int duration = 100;
   int inter_tone_gap = 50;
   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);
   GetDtmfInfoFromString("ab", duration, inter_tone_gap, &dtmf_queue_ref);
   VerifyOnProvider(dtmf_queue_ref);
   VerifyOnObserver("1ab");
 }
 
 TEST_F(DtmfSenderTest, InsertDtmfWhileProviderIsDeleted) {
   std::string tones = "@1%a&*$";
   int duration = 100;
   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.
   WAIT(false, 200);
   EXPECT_EQ(1U, observer_->tones().size());
 }
 
 TEST_F(DtmfSenderTest, InsertDtmfWhileSenderIsDeleted) {
   std::string tones = "@1%a&*$";
   int duration = 100;
   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.
   WAIT(false, 200);

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

Change this and another one below to to SIMULATED_WAIT. 

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

Good catch, done.

   EXPECT_EQ(1U, observer_->tones().size());
 }
 
 TEST_F(DtmfSenderTest, InsertEmptyTonesToCancelPreviousTask) {
   std::string tones1 = "12";
   std::string tones2 = "";
   int duration = 100;
   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);
   VerifyOnProvider(dtmf_queue_ref);
   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);
 }
 
 TEST_F(DtmfSenderTest, TryInsertDtmfWhenItDoesNotWork) {
   std::string tones = "3,4";
   int duration = 100;
   int inter_tone_gap = 50;
   provider_->RemoveCanInsertDtmfTrack(kTestAudioLabel);
   EXPECT_FALSE(dtmf_->InsertDtmf(tones, duration, inter_tone_gap));
 }
 
 TEST_F(DtmfSenderTest, InsertDtmfWithInvalidDurationOrGap) {
   std::string tones = "3,4";
   int duration = 100;
   int inter_tone_gap = 50;
 
   EXPECT_FALSE(dtmf_->InsertDtmf(tones, 6001, inter_tone_gap));
   EXPECT_FALSE(dtmf_->InsertDtmf(tones, 69, inter_tone_gap));
   EXPECT_FALSE(dtmf_->InsertDtmf(tones, duration, 49));
 
   EXPECT_TRUE(dtmf_->InsertDtmf(tones, duration, inter_tone_gap));
 }