Remove ForTesting methods from BaseChannel
The testing code prevents the production code from protecting the
member variables properly. The convenience methods for testing
purposes, can be located with the testing code.
Bug: none
Change-Id: Ieda248a199db84336dfafbd66c93c35508ab2582
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/213661
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33635}
diff --git a/pc/channel.h b/pc/channel.h
index dbcdf9d..fe3778b 100644
--- a/pc/channel.h
+++ b/pc/channel.h
@@ -133,16 +133,6 @@
return rtp_transport_ && rtp_transport_->IsSrtpActive();
}
- // Version of the above that can be called from any thread.
- bool SrtpActiveForTesting() const {
- if (!network_thread_->IsCurrent()) {
- return network_thread_->Invoke<bool>(RTC_FROM_HERE,
- [this] { return srtp_active(); });
- }
- RTC_DCHECK_RUN_ON(network_thread());
- return srtp_active();
- }
-
// Set an RTP level transport which could be an RtpTransport without
// encryption, an SrtpTransport for SDES or a DtlsSrtpTransport for DTLS-SRTP.
// This can be called from any thread and it hops to the network thread
@@ -154,16 +144,6 @@
return rtp_transport_;
}
- // Version of the above that can be called from any thread.
- webrtc::RtpTransportInternal* RtpTransportForTesting() const {
- if (!network_thread_->IsCurrent()) {
- return network_thread_->Invoke<webrtc::RtpTransportInternal*>(
- RTC_FROM_HERE, [this] { return rtp_transport(); });
- }
- RTC_DCHECK_RUN_ON(network_thread());
- return rtp_transport();
- }
-
// Channel control
bool SetLocalContent(const MediaContentDescription* content,
webrtc::SdpType type,
@@ -207,12 +187,6 @@
// RtpPacketSinkInterface overrides.
void OnRtpPacket(const webrtc::RtpPacketReceived& packet) override;
- // Used by the RTCStatsCollector tests to set the transport name without
- // creating RtpTransports.
- void set_transport_name_for_testing(const std::string& transport_name) {
- transport_name_ = transport_name;
- }
-
MediaChannel* media_channel() const override {
return media_channel_.get();
}
diff --git a/pc/channel_unittest.cc b/pc/channel_unittest.cc
index 7ff25a9..e85500a 100644
--- a/pc/channel_unittest.cc
+++ b/pc/channel_unittest.cc
@@ -510,12 +510,31 @@
// Base implementation.
}
+ // Utility method that calls BaseChannel::srtp_active() on the network thread
+ // and returns the result. The |srtp_active()| state is maintained on the
+ // network thread, which callers need to factor in.
+ bool IsSrtpActive(std::unique_ptr<typename T::Channel>& channel) {
+ RTC_DCHECK(channel.get());
+ return network_thread_->Invoke<bool>(
+ RTC_FROM_HERE, [&] { return channel->srtp_active(); });
+ }
+
+ // Returns true iff the transport is set for a channel and rtcp_mux_enabled()
+ // returns true.
+ bool IsRtcpMuxEnabled(std::unique_ptr<typename T::Channel>& channel) {
+ RTC_DCHECK(channel.get());
+ return network_thread_->Invoke<bool>(RTC_FROM_HERE, [&] {
+ return channel->rtp_transport() &&
+ channel->rtp_transport()->rtcp_mux_enabled();
+ });
+ }
+
// Tests that can be used by derived classes.
// Basic sanity check.
void TestInit() {
CreateChannels(0, 0);
- EXPECT_FALSE(channel1_->SrtpActiveForTesting());
+ EXPECT_FALSE(IsSrtpActive(channel1_));
EXPECT_FALSE(media_channel1_->sending());
if (verify_playout_) {
EXPECT_FALSE(media_channel1_->playout());
@@ -857,14 +876,14 @@
// Test setting up a call.
void TestCallSetup() {
CreateChannels(0, 0);
- EXPECT_FALSE(channel1_->SrtpActiveForTesting());
+ EXPECT_FALSE(IsSrtpActive(channel1_));
EXPECT_TRUE(SendInitiate());
if (verify_playout_) {
EXPECT_TRUE(media_channel1_->playout());
}
EXPECT_FALSE(media_channel1_->sending());
EXPECT_TRUE(SendAccept());
- EXPECT_FALSE(channel1_->SrtpActiveForTesting());
+ EXPECT_FALSE(IsSrtpActive(channel1_));
EXPECT_TRUE(media_channel1_->sending());
EXPECT_EQ(1U, media_channel1_->codecs().size());
if (verify_playout_) {
@@ -901,8 +920,8 @@
CreateChannels(RTCP_MUX, RTCP_MUX);
EXPECT_TRUE(SendInitiate());
EXPECT_TRUE(SendAccept());
- EXPECT_TRUE(channel1_->RtpTransportForTesting()->rtcp_mux_enabled());
- EXPECT_TRUE(channel2_->RtpTransportForTesting()->rtcp_mux_enabled());
+ EXPECT_TRUE(IsRtcpMuxEnabled(channel1_));
+ EXPECT_TRUE(IsRtcpMuxEnabled(channel2_));
SendRtp1();
SendRtp2();
WaitForThreads();
@@ -925,13 +944,13 @@
void SendDtlsSrtpToDtlsSrtp(int flags1, int flags2) {
CreateChannels(flags1 | DTLS, flags2 | DTLS);
- EXPECT_FALSE(channel1_->SrtpActiveForTesting());
- EXPECT_FALSE(channel2_->SrtpActiveForTesting());
+ EXPECT_FALSE(IsSrtpActive(channel1_));
+ EXPECT_FALSE(IsSrtpActive(channel2_));
EXPECT_TRUE(SendInitiate());
WaitForThreads();
EXPECT_TRUE(SendAccept());
- EXPECT_TRUE(channel1_->SrtpActiveForTesting());
- EXPECT_TRUE(channel2_->SrtpActiveForTesting());
+ EXPECT_TRUE(IsSrtpActive(channel1_));
+ EXPECT_TRUE(IsSrtpActive(channel2_));
SendRtp1();
SendRtp2();
WaitForThreads();
@@ -949,10 +968,10 @@
CreateChannels(SSRC_MUX | RTCP_MUX | DTLS, SSRC_MUX | RTCP_MUX | DTLS);
EXPECT_TRUE(SendOffer());
EXPECT_TRUE(SendProvisionalAnswer());
- EXPECT_TRUE(channel1_->SrtpActiveForTesting());
- EXPECT_TRUE(channel2_->SrtpActiveForTesting());
- EXPECT_TRUE(channel1_->RtpTransportForTesting()->rtcp_mux_enabled());
- EXPECT_TRUE(channel2_->RtpTransportForTesting()->rtcp_mux_enabled());
+ EXPECT_TRUE(IsSrtpActive(channel1_));
+ EXPECT_TRUE(IsSrtpActive(channel2_));
+ EXPECT_TRUE(IsRtcpMuxEnabled(channel1_));
+ EXPECT_TRUE(IsRtcpMuxEnabled(channel2_));
WaitForThreads(); // Wait for 'sending' flag go through network thread.
SendCustomRtp1(kSsrc1, ++sequence_number1_1);
WaitForThreads();
@@ -965,8 +984,8 @@
// Complete call setup and ensure everything is still OK.
EXPECT_TRUE(SendFinalAnswer());
- EXPECT_TRUE(channel1_->SrtpActiveForTesting());
- EXPECT_TRUE(channel2_->SrtpActiveForTesting());
+ EXPECT_TRUE(IsSrtpActive(channel1_));
+ EXPECT_TRUE(IsSrtpActive(channel2_));
SendCustomRtp1(kSsrc1, ++sequence_number1_1);
SendCustomRtp2(kSsrc2, ++sequence_number2_2);
WaitForThreads();
@@ -995,8 +1014,8 @@
CreateChannels(RTCP_MUX, RTCP_MUX);
EXPECT_TRUE(SendInitiate());
EXPECT_TRUE(SendAccept());
- EXPECT_TRUE(channel1_->RtpTransportForTesting()->rtcp_mux_enabled());
- EXPECT_TRUE(channel2_->RtpTransportForTesting()->rtcp_mux_enabled());
+ EXPECT_TRUE(IsRtcpMuxEnabled(channel1_));
+ EXPECT_TRUE(IsRtcpMuxEnabled(channel2_));
SendRtp1();
SendRtp2();
WaitForThreads();
diff --git a/pc/test/fake_peer_connection_for_stats.h b/pc/test/fake_peer_connection_for_stats.h
index 70f8dd5..f51a69a 100644
--- a/pc/test/fake_peer_connection_for_stats.h
+++ b/pc/test/fake_peer_connection_for_stats.h
@@ -75,6 +75,64 @@
constexpr bool kDefaultRtcpMuxRequired = true;
constexpr bool kDefaultSrtpRequired = true;
+class VoiceChannelForTesting : public cricket::VoiceChannel {
+ public:
+ VoiceChannelForTesting(rtc::Thread* worker_thread,
+ rtc::Thread* network_thread,
+ rtc::Thread* signaling_thread,
+ std::unique_ptr<cricket::VoiceMediaChannel> channel,
+ const std::string& content_name,
+ bool srtp_required,
+ webrtc::CryptoOptions crypto_options,
+ rtc::UniqueRandomIdGenerator* ssrc_generator,
+ std::string transport_name)
+ : VoiceChannel(worker_thread,
+ network_thread,
+ signaling_thread,
+ std::move(channel),
+ content_name,
+ srtp_required,
+ std::move(crypto_options),
+ ssrc_generator),
+ test_transport_name_(std::move(transport_name)) {}
+
+ private:
+ const std::string& transport_name() const override {
+ return test_transport_name_;
+ }
+
+ const std::string test_transport_name_;
+};
+
+class VideoChannelForTesting : public cricket::VideoChannel {
+ public:
+ VideoChannelForTesting(rtc::Thread* worker_thread,
+ rtc::Thread* network_thread,
+ rtc::Thread* signaling_thread,
+ std::unique_ptr<cricket::VideoMediaChannel> channel,
+ const std::string& content_name,
+ bool srtp_required,
+ webrtc::CryptoOptions crypto_options,
+ rtc::UniqueRandomIdGenerator* ssrc_generator,
+ std::string transport_name)
+ : VideoChannel(worker_thread,
+ network_thread,
+ signaling_thread,
+ std::move(channel),
+ content_name,
+ srtp_required,
+ std::move(crypto_options),
+ ssrc_generator),
+ test_transport_name_(std::move(transport_name)) {}
+
+ private:
+ const std::string& transport_name() const override {
+ return test_transport_name_;
+ }
+
+ const std::string test_transport_name_;
+};
+
// This class is intended to be fed into the StatsCollector and
// RTCStatsCollector so that the stats functionality can be unit tested.
// Individual tests can configure this fake as needed to simulate scenarios
@@ -140,11 +198,10 @@
auto voice_media_channel =
std::make_unique<FakeVoiceMediaChannelForStats>();
auto* voice_media_channel_ptr = voice_media_channel.get();
- voice_channel_ = std::make_unique<cricket::VoiceChannel>(
+ voice_channel_ = std::make_unique<VoiceChannelForTesting>(
worker_thread_, network_thread_, signaling_thread_,
std::move(voice_media_channel), mid, kDefaultSrtpRequired,
- webrtc::CryptoOptions(), &ssrc_generator_);
- voice_channel_->set_transport_name_for_testing(transport_name);
+ webrtc::CryptoOptions(), &ssrc_generator_, transport_name);
GetOrCreateFirstTransceiverOfType(cricket::MEDIA_TYPE_AUDIO)
->internal()
->SetChannel(voice_channel_.get());
@@ -158,11 +215,10 @@
auto video_media_channel =
std::make_unique<FakeVideoMediaChannelForStats>();
auto video_media_channel_ptr = video_media_channel.get();
- video_channel_ = std::make_unique<cricket::VideoChannel>(
+ video_channel_ = std::make_unique<VideoChannelForTesting>(
worker_thread_, network_thread_, signaling_thread_,
std::move(video_media_channel), mid, kDefaultSrtpRequired,
- webrtc::CryptoOptions(), &ssrc_generator_);
- video_channel_->set_transport_name_for_testing(transport_name);
+ webrtc::CryptoOptions(), &ssrc_generator_, transport_name);
GetOrCreateFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO)
->internal()
->SetChannel(video_channel_.get());
