Increase STUN RTOs

webrtc/p2p/base/stunport_unittest.cc

Index: webrtc/p2p/base/stunport_unittest.cc
diff --git a/webrtc/p2p/base/stunport_unittest.cc b/webrtc/p2p/base/stunport_unittest.cc
index 522369d487c391245d60c1edad292fe29dcdbdf6..48cd7045a688c091628a5b2a13d4a898462cbe65 100644
--- a/webrtc/p2p/base/stunport_unittest.cc
+++ b/webrtc/p2p/base/stunport_unittest.cc
@@ -30,11 +30,6 @@ static const SocketAddress kStunAddr3("127.0.0.1", 3000);
 static const SocketAddress kBadAddr("0.0.0.1", 5000);
 static const SocketAddress kStunHostnameAddr("localhost", 5000);
 static const SocketAddress kBadHostnameAddr("not-a-real-hostname", 5000);
-// STUN timeout (with all retries) is 9500ms.
-// Add some margin of error for slow bots.
-// TODO(deadbeef): Use simulated clock instead of just increasing timeouts to
-// fix flaky tests.
-static const int kTimeoutMs = 15000;
 // stun prio = 100 << 24 | 30 (IPV4) << 8 | 256 - 0
 static const uint32_t kStunCandidatePriority = 1677729535;
 static const int kInfiniteLifetime = -1;
@@ -81,6 +76,7 @@ class StunPortTest : public testing::Test,
         kLocalAddr.ipaddr(), 0, 0, rtc::CreateRandomString(16),
         rtc::CreateRandomString(22), stun_servers, std::string()));
     stun_port_->set_stun_keepalive_delay(stun_keepalive_delay_);
+    stun_port_->set_timeout_delay(cricket::STUN_TOTAL_TIMEOUT);
     // If |stun_keepalive_lifetime_| is negative, let the stun port
     // choose its lifetime from the network type.
     if (stun_keepalive_lifetime_ >= 0) {
@@ -160,7 +156,10 @@ class StunPortTest : public testing::Test,
     return stun_server_2_.get();
   }
 
+  rtc::FakeClock& fake_clock() { return fake_clock_; }
+
  private:
+  rtc::ScopedFakeClock fake_clock_;
   std::unique_ptr<rtc::PhysicalSocketServer> pss_;
   std::unique_ptr<rtc::VirtualSocketServer> ss_;
   rtc::SocketServerScope ss_scope_;
@@ -194,7 +193,7 @@ TEST_F(StunPortTest, TestCreateUdpPort) {
 TEST_F(StunPortTest, TestPrepareAddress) {
   CreateStunPort(kStunAddr1);
   PrepareAddress();
-  EXPECT_TRUE_WAIT(done(), kTimeoutMs);
+  EXPECT_TRUE_SIMULATED_WAIT(done(), cricket::STUN_TOTAL_TIMEOUT, fake_clock());
   ASSERT_EQ(1U, port()->Candidates().size());
   EXPECT_TRUE(kLocalAddr.EqualIPs(port()->Candidates()[0].address()));
 
@@ -206,7 +205,7 @@ TEST_F(StunPortTest, TestPrepareAddress) {
 TEST_F(StunPortTest, TestPrepareAddressFail) {
   CreateStunPort(kBadAddr);
   PrepareAddress();
-  EXPECT_TRUE_WAIT(done(), kTimeoutMs);
+  EXPECT_TRUE_SIMULATED_WAIT(done(), cricket::STUN_TOTAL_TIMEOUT, fake_clock());
   EXPECT_TRUE(error());
   EXPECT_EQ(0U, port()->Candidates().size());
 }
@@ -215,7 +214,7 @@ TEST_F(StunPortTest, TestPrepareAddressFail) {
 TEST_F(StunPortTest, TestPrepareAddressHostname) {
   CreateStunPort(kStunHostnameAddr);
   PrepareAddress();
-  EXPECT_TRUE_WAIT(done(), kTimeoutMs);
+  EXPECT_TRUE_SIMULATED_WAIT(done(), cricket::STUN_TOTAL_TIMEOUT, fake_clock());
   ASSERT_EQ(1U, port()->Candidates().size());
   EXPECT_TRUE(kLocalAddr.EqualIPs(port()->Candidates()[0].address()));
   EXPECT_EQ(kStunCandidatePriority, port()->Candidates()[0].priority());
@@ -225,7 +224,9 @@ TEST_F(StunPortTest, TestPrepareAddressHostname) {
 TEST_F(StunPortTest, TestPrepareAddressHostnameFail) {
   CreateStunPort(kBadHostnameAddr);
   PrepareAddress();
-  EXPECT_TRUE_WAIT(done(), kTimeoutMs);
+  WAIT(false, 1000);  // We have to wait for the lookup to fail, and
+                      // we can't use a simulated clock for the lookup.

[Taylor Brandstetter, 2017/02/06 19:23:43]

This is a sleep(1000) now... Is there a way to avoid this? Why doesn't
EXPECT_TRUE_WAIT still work?

+  EXPECT_TRUE_SIMULATED_WAIT(done(), cricket::STUN_TOTAL_TIMEOUT, fake_clock());
   EXPECT_TRUE(error());
   EXPECT_EQ(0U, port()->Candidates().size());
 }
@@ -236,20 +237,17 @@ TEST_F(StunPortTest, TestKeepAliveResponse) {
   SetKeepaliveDelay(500);  // 500ms of keepalive delay.
   CreateStunPort(kStunHostnameAddr);
   PrepareAddress();
-  EXPECT_TRUE_WAIT(done(), kTimeoutMs);
+  EXPECT_TRUE_SIMULATED_WAIT(done(), cricket::STUN_TOTAL_TIMEOUT, fake_clock());
   ASSERT_EQ(1U, port()->Candidates().size());
   EXPECT_TRUE(kLocalAddr.EqualIPs(port()->Candidates()[0].address()));
-  // Waiting for 1 seond, which will allow us to process
-  // response for keepalive binding request. 500 ms is the keepalive delay.
-  rtc::Thread::Current()->ProcessMessages(1000);
-  ASSERT_EQ(1U, port()->Candidates().size());
+  EXPECT_EQ_SIMULATED_WAIT(1U, port()->Candidates().size(), 500, fake_clock());
 }
 
 // Test that a local candidate can be generated using a shared socket.
 TEST_F(StunPortTest, TestSharedSocketPrepareAddress) {
   CreateSharedUdpPort(kStunAddr1);
   PrepareAddress();
-  EXPECT_TRUE_WAIT(done(), kTimeoutMs);
+  EXPECT_TRUE_SIMULATED_WAIT(done(), cricket::STUN_TOTAL_TIMEOUT, fake_clock());
   ASSERT_EQ(1U, port()->Candidates().size());
   EXPECT_TRUE(kLocalAddr.EqualIPs(port()->Candidates()[0].address()));
 }
@@ -260,7 +258,9 @@ TEST_F(StunPortTest, TestSharedSocketPrepareAddress) {
 TEST_F(StunPortTest, TestSharedSocketPrepareAddressInvalidHostname) {
   CreateSharedUdpPort(kBadHostnameAddr);
   PrepareAddress();
-  EXPECT_TRUE_WAIT(done(), kTimeoutMs);
+  WAIT(false, 1000);  // We have to wait for the lookup to fail, and
+                      // we can't use a simulated clock for the lookup.
+  EXPECT_TRUE_SIMULATED_WAIT(done(), cricket::STUN_TOTAL_TIMEOUT, fake_clock());
   ASSERT_EQ(1U, port()->Candidates().size());
   EXPECT_TRUE(kLocalAddr.EqualIPs(port()->Candidates()[0].address()));
 
@@ -279,7 +279,7 @@ TEST_F(StunPortTest, TestNoDuplicatedAddressWithTwoStunServers) {
   CreateStunPort(stun_servers);
   EXPECT_EQ("stun", port()->Type());
   PrepareAddress();
-  EXPECT_TRUE_WAIT(done(), kTimeoutMs);
+  EXPECT_TRUE_SIMULATED_WAIT(done(), cricket::STUN_TOTAL_TIMEOUT, fake_clock());
   EXPECT_EQ(1U, port()->Candidates().size());
   EXPECT_EQ(port()->Candidates()[0].relay_protocol(), "");
 }
@@ -293,7 +293,7 @@ TEST_F(StunPortTest, TestMultipleStunServersWithBadServer) {
   CreateStunPort(stun_servers);
   EXPECT_EQ("stun", port()->Type());
   PrepareAddress();
-  EXPECT_TRUE_WAIT(done(), kTimeoutMs);
+  EXPECT_TRUE_SIMULATED_WAIT(done(), cricket::STUN_TOTAL_TIMEOUT, fake_clock());
   EXPECT_EQ(1U, port()->Candidates().size());
 }
 
@@ -311,7 +311,7 @@ TEST_F(StunPortTest, TestTwoCandidatesWithTwoStunServersAcrossNat) {
   CreateStunPort(stun_servers);
   EXPECT_EQ("stun", port()->Type());
   PrepareAddress();
-  EXPECT_TRUE_WAIT(done(), kTimeoutMs);
+  EXPECT_TRUE_SIMULATED_WAIT(done(), cricket::STUN_TOTAL_TIMEOUT, fake_clock());
   EXPECT_EQ(2U, port()->Candidates().size());
   EXPECT_EQ(port()->Candidates()[0].relay_protocol(), "");
   EXPECT_EQ(port()->Candidates()[1].relay_protocol(), "");
@@ -360,9 +360,10 @@ TEST_F(StunPortTest, TestStunBindingRequestShortLifetime) {
   SetKeepaliveLifetime(100);
   CreateStunPort(kStunAddr1);
   PrepareAddress();
-  EXPECT_TRUE_WAIT(done(), kTimeoutMs);
-  EXPECT_TRUE_WAIT(!port()->HasPendingRequest(cricket::STUN_BINDING_REQUEST),
-                   2000);
+  EXPECT_TRUE_SIMULATED_WAIT(done(), cricket::STUN_TOTAL_TIMEOUT, fake_clock());
+  EXPECT_TRUE_SIMULATED_WAIT(
+      !port()->HasPendingRequest(cricket::STUN_BINDING_REQUEST), 2000,
+      fake_clock());
 }
 
 // Test that by default, the STUN binding requests will last for a long time.
@@ -370,7 +371,8 @@ TEST_F(StunPortTest, TestStunBindingRequestLongLifetime) {
   SetKeepaliveDelay(101);
   CreateStunPort(kStunAddr1);
   PrepareAddress();
-  EXPECT_TRUE_WAIT(done(), kTimeoutMs);
-  rtc::Thread::Current()->ProcessMessages(1000);
-  EXPECT_TRUE(port()->HasPendingRequest(cricket::STUN_BINDING_REQUEST));
+  EXPECT_TRUE_SIMULATED_WAIT(done(), cricket::STUN_TOTAL_TIMEOUT, fake_clock());
+  EXPECT_TRUE_SIMULATED_WAIT(
+      port()->HasPendingRequest(cricket::STUN_BINDING_REQUEST), 1000,
+      fake_clock());
 }