A few updates on connection states

webrtc/p2p/base/p2ptransportchannel_unittest.cc

Index: webrtc/p2p/base/p2ptransportchannel_unittest.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
index d0277f481461015f67320fc6f1eadc4166a59813..80cc016b8f4933f3d1c3035dd9f6ba72037e35ca 100644
--- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
+++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -1493,7 +1493,8 @@ TEST_F(P2PTransportChannelMultihomedTest, DISABLED_TestBasic) {
 }
 
 // Test that we can quickly switch links if an interface goes down.
-TEST_F(P2PTransportChannelMultihomedTest, TestFailover) {
+// The controlled side has two interfaces and one will die.
+TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControlledSide) {
   AddAddress(0, kPublicAddrs[0]);
   // Adding alternate address will make sure |kPublicAddrs| has the higher
   // priority than others. This is due to FakeNetwork::AddInterface method.
@@ -1516,19 +1517,80 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailover) {
 
   // Blackhole any traffic to or from the public addrs.
   LOG(LS_INFO) << "Failing over...";
-  fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY,
-                kPublicAddrs[1]);
-
-  // We should detect loss of connectivity within 5 seconds or so.
-  EXPECT_TRUE_WAIT(!ep1_ch1()->writable(), 7000);
+  fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, kPublicAddrs[1]);
+  // The best connections will switch, so keep references to them.
+  const cricket::Connection* best_connection1 = ep1_ch1()->best_connection();
+  const cricket::Connection* best_connection2 = ep2_ch1()->best_connection();
+  // We should detect loss of receiving within 2.5 seconds or so.
+  EXPECT_TRUE_WAIT(
+      !best_connection1->receiving() && !best_connection2->receiving(), 3000);

[pthatcher1, 2015/09/22 23:41:46]

See my comments below and please change this place as well.

[honghaiz3, 2015/09/23 03:16:10]

Done.

 
-  // We should switch over to use the alternate addr immediately
-  // when we lose writability.
+  // We should switch over to use the alternate addr immediately on both sides
+  // when we are not receiving.
   EXPECT_TRUE_WAIT(
+      ep1_ch1()->best_connection()->receiving() &&
+      ep2_ch1()->best_connection()->receiving(), 1000);
+  best_connection1 = ep1_ch1()->best_connection();
+  best_connection2 = ep2_ch1()->best_connection();
+  EXPECT_TRUE(best_connection1->writable());
+  EXPECT_TRUE(best_connection2->writable());
+  EXPECT_TRUE(LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]));
+  EXPECT_TRUE(
+      RemoteCandidate(ep1_ch1())->address().EqualIPs(kAlternateAddrs[1]));
+  EXPECT_TRUE(
+      LocalCandidate(ep2_ch1())->address().EqualIPs(kAlternateAddrs[1]));
+
+  DestroyChannels();
+}
+
+// Test that we can quickly switch links if an interface goes down.
+// The controlling side has two interfaces and one will die.
+TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControllingSide) {
+  // Adding alternate address will make sure |kPublicAddrs| has the higher
+  // priority than others. This is due to FakeNetwork::AddInterface method.
+  AddAddress(0, kAlternateAddrs[0]);
+  AddAddress(0, kPublicAddrs[0]);
+
+  AddAddress(1, kPublicAddrs[1]);
+
+  // Use only local ports for simplicity.
+  SetAllocatorFlags(0, kOnlyLocalPorts);
+  SetAllocatorFlags(1, kOnlyLocalPorts);
+
+  // Create channels and let them go writable, as usual.
+  CreateChannels(1);
+  EXPECT_TRUE_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
+                   ep2_ch1()->receiving() && ep2_ch1()->writable(),
+                   1000);
+  EXPECT_TRUE(
       ep1_ch1()->best_connection() && ep2_ch1()->best_connection() &&
       LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]) &&
-      RemoteCandidate(ep1_ch1())->address().EqualIPs(kAlternateAddrs[1]),
-      3000);
+      RemoteCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[1]));
+
+  // Blackhole any traffic to or from the public addrs.
+  LOG(LS_INFO) << "Failing over...";
+  fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, kPublicAddrs[0]);
+  // The best connections will switch, so keep references to them.
+  const cricket::Connection* best_connection1 = ep1_ch1()->best_connection();
+  const cricket::Connection* best_connection2 = ep2_ch1()->best_connection();
+  // We should detect loss of receiving within 2.5 seconds or so.
+  EXPECT_TRUE_WAIT(
+      !best_connection1->receiving() && !best_connection2->receiving(), 3000);

[pthatcher1, 2015/09/22 23:41:46]

Can you lower the receive timeout for the tests to make them run faster?    And
can you bump the expect timeout to make sure this test doesn't go flaky?

[honghaiz3, 2015/09/23 03:16:10]

I set the receive timeout to 1 second. Because of that, I think we do not need
to increase the expect timeout any more. 

+
+  // We should switch over to use the alternate addr immediately on both sides
+  // when we are not receiving.
+  EXPECT_TRUE_WAIT(
+      ep1_ch1()->best_connection()->receiving() &&
+      ep2_ch1()->best_connection()->receiving(), 1000);
+  best_connection1 = ep1_ch1()->best_connection();
+  best_connection2 = ep2_ch1()->best_connection();
+  EXPECT_TRUE(best_connection1->writable());
+  EXPECT_TRUE(best_connection2->writable());

[pthatcher1, 2015/09/22 23:41:46]

Do you really need to check this?  It doesn't matter right?   And if a test bot
gets really slow, this might be false by the time you get here.

[honghaiz3, 2015/09/23 03:16:10]

Right. removed these two lines here and above.

+  EXPECT_TRUE(
+    LocalCandidate(ep1_ch1())->address().EqualIPs(kAlternateAddrs[0]));
+  EXPECT_TRUE(RemoteCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[1]));
+  EXPECT_TRUE(
+      RemoteCandidate(ep2_ch1())->address().EqualIPs(kAlternateAddrs[0]));
 
   DestroyChannels();
 }