Emit SignalReadyToSend even for "presumed writable" connections.

webrtc/p2p/base/p2ptransportchannel_unittest.cc

 /*
  *  Copyright 2009 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.
  */
 
@@ skipping 1788 matching lines @@
       CreateUdpCandidate(RELAY_PORT_TYPE, "2.2.2.2", 2, 0));
   EXPECT_EQ(RELAY_PORT_TYPE, LocalCandidate(ep1_ch1())->type());
   EXPECT_EQ(RELAY_PORT_TYPE, RemoteCandidate(ep1_ch1())->type());
   EXPECT_TRUE(ep1_ch1()->writable());
   EXPECT_TRUE(GetEndpoint(0)->ready_to_send_);
   EXPECT_NE(old_selected_connection, ep1_ch1()->selected_connection());
   // Explitly destroy channels, before fake clock is destroyed.
   DestroyChannels();
 }
 
+// Ensure that "SignalReadyToSend" is fired as expected with a "presumed
+// writable" connection. Previously this did not work.
+TEST_F(P2PTransportChannelTest, SignalReadyToSendWithPresumedWritable) {
+  ConfigureEndpoints(OPEN, OPEN, kDefaultPortAllocatorFlags,
+                     kDefaultPortAllocatorFlags);
+  // Only test one endpoint, so we can ensure the connection doesn't receive a

[honghaiz3, 2016/09/29 20:04:51]

If you only test one endpoint, would it be better to create the test based on
P2PTransportChannelPingTest, which has only one endpoint?

[Taylor Brandstetter, 2016/09/29 21:09:09]

I could, but then I'd have to add a TURN server to the test, and I thought that
may be introducing bloat. What do you think?

Also, if I move this test, should I move "TurnToTurnPresumedWritable" above,
which also only uses one endpoint?

[honghaiz3, 2016/09/29 21:47:59]

I will leave it for you to decide. If you wait for my CL being landed, that will
be just one-line to Create the Basic port allocator (and maybe a few more lines
for setting up). 
If you move this, probably it is a good idea to move that too (or put a TODO at
least). 

[Taylor Brandstetter, 2016/09/29 22:24:11]

I looked into this, but the TURN server and network manager are only created in
P2PTransportChannelMostLikelyToWorkFirst test (subclass of
P2PTransportChannelPingTest), which is where it would make the most sense to put
these tests. But then the name doesn't really match what's being tested.

Maybe we could split this file into just two test classes?
"EndToEndP2PTransportChannelTest" (which uses two P2PTranpsortChannels) and
"SingleP2PTransportChannelTest" (which tests an individual channel, simulating
individual pings being received where necessary). Similar to
"peerconnectionendtoend_unittest" vs "peerconnectioninterface_unittest".

Anyway, until we decide where we're going with these tests, I think I'll land
this with a TODO.

+  // binding response and advance beyond being "presumed" writable.

[honghaiz3, 2016/09/29 20:04:51]

nit: s/advance/advances/ ?

[Taylor Brandstetter, 2016/09/29 21:09:09]

I think "advances" is correct here ("doesn't advance" as opposed to "doesn't
advances")

[honghaiz3, 2016/09/29 21:47:59]

I thought you meant (does not receive) and (advances), right?
If you meant (does not receive) and (does not advance), then it should be
"doesn't receive or advance", I think. 
It is just nit anyway. 

+  GetEndpoint(0)->cd1_.ch_.reset(CreateChannel(
+      0, ICE_CANDIDATE_COMPONENT_DEFAULT, kIceParams[0], kIceParams[1]));
+  IceConfig config;
+  config.presume_writable_when_fully_relayed = true;
+  ep1_ch1()->SetIceConfig(config);
+  ep1_ch1()->MaybeStartGathering();
+  EXPECT_EQ_WAIT(IceGatheringState::kIceGatheringComplete,
+                 ep1_ch1()->gathering_state(), kDefaultTimeout);
+  ep1_ch1()->AddRemoteCandidate(
+      CreateUdpCandidate(RELAY_PORT_TYPE, "1.1.1.1", 1, 0));
+  // Sanity checking the type of the connection.
+  EXPECT_TRUE(ep1_ch1()->selected_connection() != nullptr);
+  EXPECT_EQ(RELAY_PORT_TYPE, LocalCandidate(ep1_ch1())->type());
+  EXPECT_EQ(RELAY_PORT_TYPE, RemoteCandidate(ep1_ch1())->type());
+
+  // Tell the socket server to block packets (returning EWOULDBLOCK).
+  virtual_socket_server()->SetSendingBlocked(true);
+  const char* data = "test";
+  int len = static_cast<int>(strlen(data));
+  EXPECT_EQ(-1, SendData(ep1_ch1(), data, len));
+
+  // Reset |ready_to_send_| flag, which is set to true if the event fires as it
+  // should.
+  GetEndpoint(0)->ready_to_send_ = false;
+  virtual_socket_server()->SetSendingBlocked(false);
+  EXPECT_TRUE(GetEndpoint(0)->ready_to_send_);
+  EXPECT_EQ(len, SendData(ep1_ch1(), data, len));
+}
+
 // Test what happens when we have 2 users behind the same NAT. This can lead
 // to interesting behavior because the STUN server will only give out the
 // address of the outermost NAT.
 class P2PTransportChannelSameNatTest : public P2PTransportChannelTestBase {
  protected:
   void ConfigureEndpoints(Config nat_type, Config config1, Config config2) {
     ASSERT(nat_type >= NAT_FULL_CONE && nat_type <= NAT_SYMMETRIC);
     rtc::NATSocketServer::Translator* outer_nat =
         nat()->AddTranslator(kPublicAddrs[0], kNatAddrs[0],
             static_cast<rtc::NATType>(nat_type - NAT_FULL_CONE));
@@ skipping 2194 matching lines @@
 
   // TCP Relay/Relay is the next.
   VerifyNextPingableConnection(RELAY_PORT_TYPE, RELAY_PORT_TYPE,
                                TCP_PROTOCOL_NAME);
 
   // Finally, Local/Relay will be pinged.
   VerifyNextPingableConnection(LOCAL_PORT_TYPE, RELAY_PORT_TYPE);
 }
 
 }  // namespace cricket {