TransportController refactoring

webrtc/p2p/base/transportcontroller_unittest.cc

Index: webrtc/p2p/base/transportcontroller_unittest.cc
diff --git a/webrtc/p2p/base/transportcontroller_unittest.cc b/webrtc/p2p/base/transportcontroller_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f13695e45b9454ab3a770fb1b56fc3c1dc0ed596
--- /dev/null
+++ b/webrtc/p2p/base/transportcontroller_unittest.cc
@@ -0,0 +1,617 @@
+/*
+ *  Copyright 2015 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 <map>
+
+#include "webrtc/base/fakesslidentity.h"
+#include "webrtc/base/gunit.h"
+#include "webrtc/base/helpers.h"
+#include "webrtc/base/scoped_ptr.h"
+#include "webrtc/base/sslidentity.h"
+#include "webrtc/base/thread.h"
+#include "webrtc/p2p/base/dtlstransportchannel.h"
+#include "webrtc/p2p/base/faketransportcontroller.h"
+#include "webrtc/p2p/base/p2ptransportchannel.h"
+#include "webrtc/p2p/base/portallocator.h"
+#include "webrtc/p2p/base/transportcontroller.h"
+#include "webrtc/p2p/base/transportchannelproxy.h"
+#include "webrtc/p2p/client/fakeportallocator.h"
+
+static const int kTimeout = 100;
+static const char kIceUfrag1[] = "TESTICEUFRAG0001";
+static const char kIcePwd1[] = "TESTICEPWD00000000000001";
+static const char kIceUfrag2[] = "TESTICEUFRAG0002";
+static const char kIcePwd2[] = "TESTICEPWD00000000000002";
+
+using cricket::Candidate;
+using cricket::Candidates;
+using cricket::FakeTransportChannel;
+using cricket::FakeTransportController;
+using cricket::IceConnectionState;
+using cricket::IceGatheringState;
+using cricket::TransportChannel;
+using cricket::TransportController;
+using cricket::TransportDescription;
+using cricket::TransportStats;
+
+class TransportControllerTest : public testing::Test,
+                                public sigslot::has_slots<> {
+ public:
+  TransportControllerTest() : signaling_thread_(rtc::Thread::Current()) {
+    CreateTransportController(false);
+  }
+
+  void CreateTransportController(bool using_worker_thread) {

[pthatcher1, 2015/09/01 17:05:22]

Instead of passing a bool, can you have something like
CreateTransportController() and CreateTransportControllerWithWorkerThread()?  I
think that would be more readable.

[Taylor Brandstetter, 2015/09/01 23:53:31]

Done.

+    if (using_worker_thread) {
+      transport_controller_.reset(
+          new FakeTransportController(&worker_thread_, true));
+    } else {
+      transport_controller_.reset(new FakeTransportController());
+    }
+    transport_controller_->SignalConnectionState.connect(
+        this, &TransportControllerTest::OnConnectionState);
+    transport_controller_->SignalReceiving.connect(
+        this, &TransportControllerTest::OnReceiving);
+    transport_controller_->SignalGatheringState.connect(
+        this, &TransportControllerTest::OnGatheringState);
+    transport_controller_->SignalCandidatesGathered.connect(
+        this, &TransportControllerTest::OnCandidatesGathered);
+  }
+
+  FakeTransportChannel* CreateChannel(const std::string& content,
+                                      int component) {
+    TransportChannel* channel =
+        transport_controller_->CreateTransportChannel_w(content, component);
+    return static_cast<FakeTransportChannel*>(channel);
+  }
+
+  void DestroyChannel(const std::string& content, int component) {
+    transport_controller_->DestroyTransportChannel_w(content, component);
+  }
+
+  Candidate CreateCandidate(int component) {
+    Candidate c;
+    c.set_address(rtc::SocketAddress("192.168.1.1", 8000));
+    c.set_component(1);
+    c.set_protocol(cricket::UDP_PROTOCOL_NAME);
+    c.set_priority(1);
+    return c;
+  }
+
+  // Used for thread hopping test
+  void CompleteConnection() {
+    worker_thread_.Invoke<void>(
+        rtc::Bind(&TransportControllerTest::CompleteConnection_w, this));
+  }
+
+  void CompleteConnection_w() {
+    transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLING);
+    FakeTransportChannel* channel1 = CreateChannel("audio", 1);
+    ASSERT_NE(nullptr, channel1);
+    FakeTransportChannel* channel2 = CreateChannel("video", 1);
+    ASSERT_NE(nullptr, channel2);
+
+    TransportDescription local_desc(
+        std::vector<std::string>(), kIceUfrag1, kIcePwd1, cricket::ICEMODE_FULL,
+        cricket::CONNECTIONROLE_ACTPASS, nullptr, Candidates());
+    std::string err;
+    transport_controller_->SetLocalTransportDescription(
+        "audio", local_desc, cricket::CA_OFFER, &err);
+    transport_controller_->SetLocalTransportDescription(
+        "video", local_desc, cricket::CA_OFFER, &err);
+    channel1->SignalCandidateGathered(channel1, CreateCandidate(1));
+    channel2->SignalCandidateGathered(channel2, CreateCandidate(1));
+    channel1->SetCandidatesGatheringComplete();
+    channel2->SetCandidatesGatheringComplete();
+    channel1->SetConnectionCount(2);
+    channel2->SetConnectionCount(2);
+    channel1->SetReceiving(true);
+    channel2->SetReceiving(true);
+    channel1->SetWritable(true);
+    channel2->SetWritable(true);
+    channel1->SetConnectionCount(1);
+    channel2->SetConnectionCount(1);
+  }
+
+ protected:
+  void OnConnectionState(IceConnectionState state) {
+    if (!signaling_thread_->IsCurrent()) {
+      signaled_on_non_signaling_thread_ = true;
+    }
+    connection_state_ = state;
+    ++connection_state_signal_count_;
+  }
+
+  void OnReceiving(bool receiving) {
+    if (!signaling_thread_->IsCurrent()) {
+      signaled_on_non_signaling_thread_ = true;
+    }
+    receiving_ = receiving;
+    ++receiving_signal_count_;
+  }
+
+  void OnGatheringState(IceGatheringState state) {
+    if (!signaling_thread_->IsCurrent()) {
+      signaled_on_non_signaling_thread_ = true;
+    }
+    gathering_state_ = state;
+    ++gathering_state_signal_count_;
+  }
+
+  void OnCandidatesGathered(const std::string& transport_name,
+                            const Candidates& candidates) {
+    if (!signaling_thread_->IsCurrent()) {
+      signaled_on_non_signaling_thread_ = true;
+    }
+    candidates_[transport_name].insert(candidates_[transport_name].end(),
+                                       candidates.begin(), candidates.end());
+    ++candidates_signal_count_;
+  }
+
+  rtc::Thread worker_thread_;  // not used for most tests

[pthatcher1, 2015/09/01 17:05:21]

Please capitalize and add a ".", like a full sentence.

[Taylor Brandstetter, 2015/09/01 23:53:31]

Done.

+  rtc::scoped_ptr<FakeTransportController> transport_controller_;

[pthatcher1, 2015/09/01 17:05:22]

I'm confused about this.  This is supposed to be a set of tests for a
TransportController, but you create a FakeTransportController.  Typically, a
"fake" doesn't share code with the "real" object, and is only used by code that
depends on the real thing but needs a fake for tests.

So how are we testing the real thing by creating a fake thing?

[Taylor Brandstetter, 2015/09/01 23:53:31]

Addressed by your other comment

+
+  // Information received from signals from transport controller
+  IceConnectionState connection_state_ = cricket::kIceConnectionConnecting;
+  bool receiving_ = false;
+  IceGatheringState gathering_state_ = cricket::kIceGatheringNew;
+  std::map<std::string, Candidates> candidates_;

[pthatcher1, 2015/09/01 17:05:22]

Could you comment on the key.  Something like "// transport_name => candidates"
would be sufficient.

[Taylor Brandstetter, 2015/09/01 23:53:32]

Done.

+  // Counts of each signal emitted
+  int connection_state_signal_count_ = 0;
+  int receiving_signal_count_ = 0;
+  int gathering_state_signal_count_ = 0;
+  int candidates_signal_count_ = 0;
+
+  // Used to make sure signals only come on signaling thread
+  rtc::Thread* const signaling_thread_ = nullptr;
+  bool signaled_on_non_signaling_thread_ = false;
+};
+
+TEST_F(TransportControllerTest, TestSetIceReceivingTimeout) {
+  FakeTransportChannel* channel1 = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel1);
+
+  transport_controller_->SetIceConnectionReceivingTimeout(1000);
+  EXPECT_EQ(1000, channel1->receiving_timeout());
+
+  // Test that value stored in controller is applied to new channels
+  FakeTransportChannel* channel2 = CreateChannel("video", 1);
+  ASSERT_NE(nullptr, channel2);
+  EXPECT_EQ(1000, channel2->receiving_timeout());
+}
+
+TEST_F(TransportControllerTest, TestSetSslMaxProtocolVersion) {
+  EXPECT_TRUE(transport_controller_->SetSslMaxProtocolVersion(
+      rtc::SSL_PROTOCOL_DTLS_12));
+  FakeTransportChannel* channel = CreateChannel("audio", 1);
+
+  ASSERT_NE(nullptr, channel);
+  EXPECT_EQ(rtc::SSL_PROTOCOL_DTLS_12, channel->ssl_max_protocol_version());
+
+  // Setting max version after transport is created should fail
+  EXPECT_FALSE(transport_controller_->SetSslMaxProtocolVersion(
+      rtc::SSL_PROTOCOL_DTLS_10));
+}
+
+TEST_F(TransportControllerTest, TestSetIceRole) {
+  FakeTransportChannel* channel1 = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel1);
+
+  transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLING);
+  EXPECT_EQ(cricket::ICEROLE_CONTROLLING, channel1->GetIceRole());
+  transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLED);
+  EXPECT_EQ(cricket::ICEROLE_CONTROLLED, channel1->GetIceRole());
+
+  // Test that value stored in controller is applied to new channels
+  FakeTransportChannel* channel2 = CreateChannel("video", 1);
+  ASSERT_NE(nullptr, channel2);
+  EXPECT_EQ(cricket::ICEROLE_CONTROLLED, channel2->GetIceRole());
+}
+
+// Test that when one channel encounters a role conflict, the ICE role is
+// swapped on every channel.
+TEST_F(TransportControllerTest, TestIceRoleConflict) {
+  FakeTransportChannel* channel1 = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel1);
+  FakeTransportChannel* channel2 = CreateChannel("video", 1);
+  ASSERT_NE(nullptr, channel2);
+
+  transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLING);
+  EXPECT_EQ(cricket::ICEROLE_CONTROLLING, channel1->GetIceRole());
+  EXPECT_EQ(cricket::ICEROLE_CONTROLLING, channel2->GetIceRole());
+
+  channel1->SignalRoleConflict(channel1);
+  EXPECT_EQ(cricket::ICEROLE_CONTROLLED, channel1->GetIceRole());
+  EXPECT_EQ(cricket::ICEROLE_CONTROLLED, channel2->GetIceRole());
+}
+
+TEST_F(TransportControllerTest, TestGetSslRole) {
+  FakeTransportChannel* channel = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel);
+  ASSERT_TRUE(channel->SetSslRole(rtc::SSL_CLIENT));
+  rtc::SSLRole role;
+  EXPECT_TRUE(transport_controller_->GetSslRole(&role));
+  EXPECT_EQ(rtc::SSL_CLIENT, role);
+}
+
+TEST_F(TransportControllerTest, TestSetAndGetCertificate) {
+  rtc::scoped_refptr<rtc::RTCCertificate> certificate1 =
+      rtc::RTCCertificate::Create(
+          rtc::scoped_ptr<rtc::SSLIdentity>(
+              rtc::SSLIdentity::Generate("session1", rtc::KT_DEFAULT)).Pass());
+  rtc::scoped_refptr<rtc::RTCCertificate> certificate2 =
+      rtc::RTCCertificate::Create(
+          rtc::scoped_ptr<rtc::SSLIdentity>(
+              rtc::SSLIdentity::Generate("session2", rtc::KT_DEFAULT)).Pass());
+  rtc::scoped_refptr<rtc::RTCCertificate> returned_certificate;
+
+  FakeTransportChannel* channel1 = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel1);
+
+  EXPECT_TRUE(transport_controller_->SetCertificate(certificate1));
+  EXPECT_TRUE(
+      transport_controller_->GetCertificate("audio", &returned_certificate));
+  EXPECT_EQ(certificate1->identity()->certificate().ToPEMString(),
+            returned_certificate->identity()->certificate().ToPEMString());
+
+  // Should fail if called for a nonexistant transport
+  EXPECT_FALSE(
+      transport_controller_->GetCertificate("video", &returned_certificate));
+
+  // Test that identity stored in controller is applied to new channels
+  FakeTransportChannel* channel2 = CreateChannel("video", 1);
+  ASSERT_NE(nullptr, channel2);
+  EXPECT_TRUE(
+      transport_controller_->GetCertificate("video", &returned_certificate));
+  EXPECT_EQ(certificate1->identity()->certificate().ToPEMString(),
+            returned_certificate->identity()->certificate().ToPEMString());
+
+  // Shouldn't be able to change the identity once set
+  EXPECT_FALSE(transport_controller_->SetCertificate(certificate2));
+}
+
+TEST_F(TransportControllerTest, TestGetRemoteCertificate) {
+  rtc::FakeSSLCertificate fake_certificate("fake_data");
+  rtc::scoped_ptr<rtc::SSLCertificate> returned_certificate;
+
+  FakeTransportChannel* channel = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel);
+
+  channel->SetRemoteCertificate(&fake_certificate);
+  EXPECT_TRUE(transport_controller_->GetRemoteCertificate(
+      "audio", returned_certificate.accept()));
+  EXPECT_EQ(fake_certificate.ToPEMString(),
+            returned_certificate->ToPEMString());
+
+  // Should fail if called for a nonexistant transport
+  EXPECT_FALSE(transport_controller_->GetRemoteCertificate(
+      "video", returned_certificate.accept()));
+}
+
+TEST_F(TransportControllerTest, TestSetLocalTransportDescription) {
+  FakeTransportChannel* channel = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel);
+  TransportDescription local_desc(
+      std::vector<std::string>(), kIceUfrag1, kIcePwd1, cricket::ICEMODE_FULL,
+      cricket::CONNECTIONROLE_ACTPASS, nullptr, Candidates());
+  std::string err;
+  EXPECT_TRUE(transport_controller_->SetLocalTransportDescription(
+      "audio", local_desc, cricket::CA_OFFER, &err));
+  // Check that ICE ufrag and pwd were propagated to channel
+  EXPECT_EQ(kIceUfrag1, channel->ice_ufrag());
+  EXPECT_EQ(kIcePwd1, channel->ice_pwd());
+  // We also expect that the channel started gathering as a result of the
+  // description being set
+  EXPECT_EQ_WAIT(cricket::kIceGatheringGathering, gathering_state_, kTimeout);
+  EXPECT_EQ(1, gathering_state_signal_count_);
+}
+
+TEST_F(TransportControllerTest, TestSetRemoteTransportDescription) {
+  FakeTransportChannel* channel = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel);
+  TransportDescription remote_desc(
+      std::vector<std::string>(), kIceUfrag1, kIcePwd1, cricket::ICEMODE_FULL,
+      cricket::CONNECTIONROLE_ACTPASS, nullptr, Candidates());
+  std::string err;
+  EXPECT_TRUE(transport_controller_->SetRemoteTransportDescription(
+      "audio", remote_desc, cricket::CA_OFFER, &err));
+  // Check that ICE ufrag and pwd were propagated to channel
+  EXPECT_EQ(kIceUfrag1, channel->remote_ice_ufrag());
+  EXPECT_EQ(kIcePwd1, channel->remote_ice_pwd());
+}
+
+TEST_F(TransportControllerTest, TestAddRemoteCandidates) {
+  FakeTransportChannel* channel = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel);
+  Candidates candidates;
+  candidates.push_back(CreateCandidate(1));
+  std::string err;
+  EXPECT_TRUE(
+      transport_controller_->AddRemoteCandidates("audio", candidates, &err));
+  EXPECT_EQ(1U, channel->remote_candidates().size());
+}
+
+TEST_F(TransportControllerTest, TestReadyForRemoteCandidates) {
+  FakeTransportChannel* channel = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel);
+  // We expect to be ready for remote candidates only after local and remote
+  // descriptions are set.
+  EXPECT_FALSE(transport_controller_->ReadyForRemoteCandidates("audio"));
+
+  std::string err;
+  TransportDescription remote_desc(
+      std::vector<std::string>(), kIceUfrag1, kIcePwd1, cricket::ICEMODE_FULL,
+      cricket::CONNECTIONROLE_ACTPASS, nullptr, Candidates());
+  EXPECT_TRUE(transport_controller_->SetRemoteTransportDescription(
+      "audio", remote_desc, cricket::CA_OFFER, &err));
+  EXPECT_FALSE(transport_controller_->ReadyForRemoteCandidates("audio"));
+
+  TransportDescription local_desc(
+      std::vector<std::string>(), kIceUfrag2, kIcePwd2, cricket::ICEMODE_FULL,
+      cricket::CONNECTIONROLE_ACTPASS, nullptr, Candidates());
+  EXPECT_TRUE(transport_controller_->SetLocalTransportDescription(
+      "audio", local_desc, cricket::CA_ANSWER, &err));
+  EXPECT_TRUE(transport_controller_->ReadyForRemoteCandidates("audio"));
+}
+
+TEST_F(TransportControllerTest, TestGetStats) {
+  FakeTransportChannel* channel1 = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel1);
+  FakeTransportChannel* channel2 = CreateChannel("audio", 2);
+  ASSERT_NE(nullptr, channel2);
+  FakeTransportChannel* channel3 = CreateChannel("video", 1);
+  ASSERT_NE(nullptr, channel3);
+
+  TransportStats stats;
+  EXPECT_TRUE(transport_controller_->GetStats("audio", &stats));
+  EXPECT_EQ("audio", stats.content_name);
+  EXPECT_EQ(2U, stats.channel_stats.size());
+}
+
+// Test that transport gets destroyed when it has no more channels

[pthatcher1, 2015/09/01 17:05:21]

In general can you put "."s on your sentences?

[Taylor Brandstetter, 2015/09/01 23:53:31]

I'm understanding that I should always capitalize comments, but how do I decide
when one gets a period? Usually my rule of thumb has been "sentences get
periods, sentence fragments don't", but there are some cases (like "not used for
most tests" above) that defy that rule.

+TEST_F(TransportControllerTest, TestCreateAndDestroyChannel) {
+  FakeTransportChannel* channel1 = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel1);
+  FakeTransportChannel* channel2 = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel2);
+  ASSERT_EQ(channel1, channel2);
+  FakeTransportChannel* channel3 = CreateChannel("audio", 2);
+  ASSERT_NE(nullptr, channel3);
+
+  // Using GetStats to check if transport is destroyed from an outside class's
+  // perspective.
+  TransportStats stats;
+  EXPECT_TRUE(transport_controller_->GetStats("audio", &stats));
+  DestroyChannel("audio", 2);
+  DestroyChannel("audio", 1);
+  EXPECT_TRUE(transport_controller_->GetStats("audio", &stats));
+  DestroyChannel("audio", 1);
+  EXPECT_FALSE(transport_controller_->GetStats("audio", &stats));
+}
+
+TEST_F(TransportControllerTest, TestSignalConnectionStateFailed) {
+  // Need controlling ICE role to get in failed state
+  transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLING);
+  FakeTransportChannel* channel1 = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel1);
+  FakeTransportChannel* channel2 = CreateChannel("video", 1);
+  ASSERT_NE(nullptr, channel2);
+
+  // Should signal "failed" if any channel failed; channel is considered failed
+  // if it previously had a connection but now has none, and gathering is
+  // complete.
+  channel1->SetCandidatesGatheringComplete();
+  channel1->SetConnectionCount(1);
+  channel1->SetConnectionCount(0);
+  EXPECT_EQ_WAIT(cricket::kIceConnectionFailed, connection_state_, kTimeout);
+  EXPECT_EQ(1, connection_state_signal_count_);
+}
+
+TEST_F(TransportControllerTest, TestSignalConnectionStateConnected) {
+  transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLING);
+  FakeTransportChannel* channel1 = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel1);
+  FakeTransportChannel* channel2 = CreateChannel("video", 1);
+  ASSERT_NE(nullptr, channel2);
+  FakeTransportChannel* channel3 = CreateChannel("video", 2);
+  ASSERT_NE(nullptr, channel3);
+
+  // First, have one channel connect, and another fail, to ensure that
+  // the first channel connecting didn't trigger a "connected" state signal.
+  // We should only get a signal when all are connected.
+  channel1->SetConnectionCount(2);
+  channel1->SetWritable(true);
+  channel3->SetCandidatesGatheringComplete();
+  channel3->SetConnectionCount(1);
+  channel3->SetConnectionCount(0);
+  EXPECT_EQ_WAIT(cricket::kIceConnectionFailed, connection_state_, kTimeout);
+  // Signal count of 1 means that the only signal emitted was "failed".
+  EXPECT_EQ(1, connection_state_signal_count_);
+
+  // Destroy the failed channel to return to "connecting" state
+  DestroyChannel("video", 2);
+  EXPECT_EQ_WAIT(cricket::kIceConnectionConnecting, connection_state_,
+                 kTimeout);
+  EXPECT_EQ(2, connection_state_signal_count_);
+
+  // Make the remaining channel reach a connected state
+  channel2->SetConnectionCount(2);
+  channel2->SetWritable(true);
+  EXPECT_EQ_WAIT(cricket::kIceConnectionConnected, connection_state_, kTimeout);
+  EXPECT_EQ(3, connection_state_signal_count_);
+}
+
+TEST_F(TransportControllerTest, TestSignalConnectionStateComplete) {
+  transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLING);
+  FakeTransportChannel* channel1 = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel1);
+  FakeTransportChannel* channel2 = CreateChannel("video", 1);
+  ASSERT_NE(nullptr, channel2);
+  FakeTransportChannel* channel3 = CreateChannel("video", 2);
+  ASSERT_NE(nullptr, channel3);
+
+  // Similar to above test, but we're now reaching the completed state, which
+  // means only one connection per FakeTransportChannel.
+  channel1->SetCandidatesGatheringComplete();
+  channel1->SetConnectionCount(1);
+  channel1->SetWritable(true);
+  channel3->SetCandidatesGatheringComplete();
+  channel3->SetConnectionCount(1);
+  channel3->SetConnectionCount(0);
+  EXPECT_EQ_WAIT(cricket::kIceConnectionFailed, connection_state_, kTimeout);
+  // Signal count of 1 means that the only signal emitted was "failed".
+  EXPECT_EQ(1, connection_state_signal_count_);
+
+  // Destroy the failed channel to return to "connecting" state
+  DestroyChannel("video", 2);
+  EXPECT_EQ_WAIT(cricket::kIceConnectionConnecting, connection_state_,
+                 kTimeout);
+  EXPECT_EQ(2, connection_state_signal_count_);
+
+  // Make the remaining channel reach a connected state
+  channel2->SetCandidatesGatheringComplete();
+  channel2->SetConnectionCount(2);
+  channel2->SetWritable(true);
+  EXPECT_EQ_WAIT(cricket::kIceConnectionConnected, connection_state_, kTimeout);
+  EXPECT_EQ(3, connection_state_signal_count_);
+
+  // Finally, transition to completed state
+  channel2->SetConnectionCount(1);
+  EXPECT_EQ_WAIT(cricket::kIceConnectionCompleted, connection_state_, kTimeout);
+  EXPECT_EQ(4, connection_state_signal_count_);
+}
+
+TEST_F(TransportControllerTest, TestSignalReceiving) {
+  FakeTransportChannel* channel1 = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel1);
+  FakeTransportChannel* channel2 = CreateChannel("video", 1);
+  ASSERT_NE(nullptr, channel2);
+
+  // Should signal receiving as soon as any channel is receiving
+  channel1->SetReceiving(true);
+  EXPECT_EQ_WAIT(true, receiving_, kTimeout);
+  EXPECT_EQ(1, receiving_signal_count_);
+
+  channel2->SetReceiving(true);
+  channel1->SetReceiving(false);
+  channel2->SetReceiving(false);
+  EXPECT_EQ_WAIT(false, receiving_, kTimeout);
+  EXPECT_EQ(2, receiving_signal_count_);
+}
+
+TEST_F(TransportControllerTest, TestSignalGatheringStateGathering) {
+  FakeTransportChannel* channel1 = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel1);
+  FakeTransportChannel* channel2 = CreateChannel("video", 1);
+  ASSERT_NE(nullptr, channel2);
+  // Connect normally starts candidate gathering

[pthatcher1, 2015/09/01 17:05:22]

"normally"?

[Taylor Brandstetter, 2015/09/01 23:53:31]

Removed. I added a comment to transportchannelimpl.h confirming that this is
what Connect is EXPECTED to do, which makes the word "normally" here redundant.

+  channel1->Connect();
+  // Should be in the gathering state as soon as any transport starts gathering
+  EXPECT_EQ_WAIT(cricket::kIceGatheringGathering, gathering_state_, kTimeout);
+  EXPECT_EQ(1, gathering_state_signal_count_);
+}
+
+TEST_F(TransportControllerTest, TestSignalGatheringStateComplete) {
+  FakeTransportChannel* channel1 = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel1);
+  FakeTransportChannel* channel2 = CreateChannel("video", 1);
+  ASSERT_NE(nullptr, channel2);
+  FakeTransportChannel* channel3 = CreateChannel("data", 1);

[pthatcher1, 2015/09/01 17:05:22]

Instead of channel1, channel2, channel3, can you give them more meaningful
names, like "complete_than_destroy" or "complete_later", etc, so you can tell
what it's supposed to do.  You could apply the same idea to lots of unit tests.

[Taylor Brandstetter, 2015/09/01 23:53:32]

I tried that, but it made things less readable to me. Given that each of these
tests is so short, I feel it's easier to just read the code to see how channel
is used. Do you want to chat about this some time?

[pthatcher1, 2015/09/02 04:22:47]

If it's less readable, then don't do it.

+  ASSERT_NE(nullptr, channel3);
+
+  channel3->Connect();
+  EXPECT_EQ_WAIT(cricket::kIceGatheringGathering, gathering_state_, kTimeout);
+  EXPECT_EQ(1, gathering_state_signal_count_);
+
+  // Have one channel finish gathering, then destroy it, to make sure gathering
+  // completion wasn't signalled if only one transport finished gathering.
+  channel3->SetCandidatesGatheringComplete();
+  DestroyChannel("data", 1);
+  EXPECT_EQ_WAIT(cricket::kIceGatheringNew, gathering_state_, kTimeout);
+  EXPECT_EQ(2, gathering_state_signal_count_);
+
+  // Make remaining channels start and then finish gathering
+  channel1->Connect();
+  channel2->Connect();
+  EXPECT_EQ_WAIT(cricket::kIceGatheringGathering, gathering_state_, kTimeout);
+  EXPECT_EQ(3, gathering_state_signal_count_);
+
+  channel1->SetCandidatesGatheringComplete();
+  channel2->SetCandidatesGatheringComplete();
+  EXPECT_EQ_WAIT(cricket::kIceGatheringComplete, gathering_state_, kTimeout);
+  EXPECT_EQ(4, gathering_state_signal_count_);
+}
+
+// Test that when the last transport that hasn't finished connecting and/or
+// gathering is destroyed, the aggregate state jumps to "completed". This can
+// happen if, for example, we have an audio and video transport, the audio
+// transport completes, then we start bundling video on the audio transport.
+TEST_F(TransportControllerTest,
+       TestSignalingWhenLastIncompleteTransportDestroyed) {
+  transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLING);
+  FakeTransportChannel* channel1 = CreateChannel("audio", 1);

[pthatcher1, 2015/09/01 17:05:22]

channel1/channel2 could be audio/video here, since this is kind of a bundle
test, that make sense.

+  ASSERT_NE(nullptr, channel1);
+  FakeTransportChannel* channel2 = CreateChannel("video", 1);
+  ASSERT_NE(nullptr, channel2);
+
+  channel1->SetCandidatesGatheringComplete();
+  EXPECT_EQ_WAIT(cricket::kIceGatheringGathering, gathering_state_, kTimeout);
+  EXPECT_EQ(1, gathering_state_signal_count_);
+
+  channel1->SetConnectionCount(1);
+  channel1->SetWritable(true);
+  DestroyChannel("video", 1);
+  EXPECT_EQ_WAIT(cricket::kIceConnectionCompleted, connection_state_, kTimeout);
+  EXPECT_EQ(1, connection_state_signal_count_);
+  EXPECT_EQ_WAIT(cricket::kIceGatheringComplete, gathering_state_, kTimeout);
+  EXPECT_EQ(2, gathering_state_signal_count_);
+}
+
+TEST_F(TransportControllerTest, TestSignalCandidatesGathered) {
+  FakeTransportChannel* channel = CreateChannel("audio", 1);
+  ASSERT_NE(nullptr, channel);
+
+  // Transport won't signal candidates until it has a local description
+  TransportDescription local_desc(
+      std::vector<std::string>(), kIceUfrag1, kIcePwd1, cricket::ICEMODE_FULL,
+      cricket::CONNECTIONROLE_ACTPASS, nullptr, Candidates());
+  std::string err;
+  EXPECT_TRUE(transport_controller_->SetLocalTransportDescription(
+      "audio", local_desc, cricket::CA_OFFER, &err));
+
+  channel->SignalCandidateGathered(channel, CreateCandidate(1));
+  EXPECT_EQ_WAIT(1, candidates_signal_count_, kTimeout);
+  EXPECT_EQ(1U, candidates_["audio"].size());
+}
+
+TEST_F(TransportControllerTest, TestSignalingOccursOnSignalingThread) {
+  worker_thread_.Start();

[pthatcher1, 2015/09/01 17:05:22]

I think it would make sense to make worker_thread_ a smart pointer that is
.reset() every time you call CreateTransportController with a worker thread. 
That way, you don't have to remember to call Start() with every test that uses
it.

[Taylor Brandstetter, 2015/09/01 23:53:31]

Done.

+  CreateTransportController(true);
+  CompleteConnection();

[pthatcher1, 2015/09/01 17:05:22]

It seems like this could use a more clear name, explaining what it does. 

[Taylor Brandstetter, 2015/09/01 23:53:31]

Changed to "CreateChannelsAndCompleteConnectionOnWorkerThread"; somewhat verbose
but makes it pretty clear I think

+
+  // connecting --> connected --> completed
+  EXPECT_EQ_WAIT(cricket::kIceConnectionCompleted, connection_state_, kTimeout);
+  EXPECT_EQ(2, connection_state_signal_count_);
+
+  EXPECT_EQ_WAIT(true, receiving_, kTimeout);
+  EXPECT_EQ(1, receiving_signal_count_);
+
+  // new --> gathering --> complete
+  EXPECT_EQ_WAIT(cricket::kIceGatheringComplete, gathering_state_, kTimeout);
+  EXPECT_EQ(2, gathering_state_signal_count_);
+
+  EXPECT_EQ_WAIT(1U, candidates_["audio"].size(), kTimeout);
+  EXPECT_EQ_WAIT(1U, candidates_["video"].size(), kTimeout);
+  EXPECT_EQ(2, candidates_signal_count_);
+
+  EXPECT_EQ(false, signaled_on_non_signaling_thread_);
+
+  transport_controller_.reset(nullptr);

[pthatcher1, 2015/09/01 17:05:22]

Why do you need to do this?

[Taylor Brandstetter, 2015/09/01 23:53:31]

It's obsolete now; I'm getting rid of it.

+}