From 7b581eb1cab0b2ccd0a2d60163bb2b73c244346a Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Thu, 1 Feb 2018 11:32:31 -0800 Subject: [PATCH] Reland "Parameterize PeerConnection signaling tests for Unified Plan" Original change's description: > Parameterize PeerConnection signaling tests for Unified Plan > > This also changes the behavior of CreateAnswer to fail unless > the signaling state is kHaveRemoteOffer or kHaveLocalPranswer, > as per the WebRTC specification. > > Bug: webrtc:8765 > Change-Id: I60ac67cd92b17fcbff964afc14d049481e816a28 > Reviewed-on: https://webrtc-review.googlesource.com/41042 > Reviewed-by: Taylor Brandstetter > Commit-Queue: Steve Anton > Cr-Commit-Position: refs/heads/master@{#21779} Bug: webrtc:8813 Change-Id: I9f608fcd0b7aca00b4c1092e271dbd9cd710c38a Reviewed-on: https://webrtc-review.googlesource.com/46861 Commit-Queue: Steve Anton Reviewed-by: Taylor Brandstetter Reviewed-by: Steve Anton Cr-Commit-Position: refs/heads/master@{#21860} --- pc/peerconnection.cc | 36 +++---- pc/peerconnection_ice_unittest.cc | 2 +- pc/peerconnection_signaling_unittest.cc | 125 +++++++++++++----------- 3 files changed, 83 insertions(+), 80 deletions(-) diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index ca97e811ad..b4908f5359 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1674,20 +1674,17 @@ void PeerConnection::CreateAnswer(CreateSessionDescriptionObserver* observer, return; } - if (IsClosed()) { - std::string error = "CreateAnswer called when PeerConnection is closed."; + if (!(signaling_state_ == kHaveRemoteOffer || + signaling_state_ == kHaveLocalPrAnswer)) { + std::string error = + "CreateAnswer called when PeerConnection is in a wrong state."; RTC_LOG(LS_ERROR) << error; PostCreateSessionDescriptionFailure(observer, error); return; } - if (remote_description() && - remote_description()->GetType() != SdpType::kOffer) { - std::string error = "CreateAnswer called without remote offer."; - RTC_LOG(LS_ERROR) << error; - PostCreateSessionDescriptionFailure(observer, error); - return; - } + // The remote description should be set if we're in the right state. + RTC_DCHECK(remote_description()); if (IsUnifiedPlan()) { if (options.offer_to_receive_audio != RTCOfferAnswerOptions::kUndefined) { @@ -3486,18 +3483,15 @@ void PeerConnection::GetOptionsForPlanBAnswer( rtc::Optional audio_index; rtc::Optional video_index; rtc::Optional data_index; - if (remote_description()) { - // The pending remote description should be an offer. - RTC_DCHECK(remote_description()->GetType() == SdpType::kOffer); - // Generate m= sections that match those in the offer. - // Note that mediasession.cc will handle intersection our preferred - // direction with the offered direction. - GenerateMediaDescriptionOptions( - remote_description(), - RtpTransceiverDirectionFromSendRecv(send_audio, recv_audio), - RtpTransceiverDirectionFromSendRecv(send_video, recv_video), - &audio_index, &video_index, &data_index, session_options); - } + + // Generate m= sections that match those in the offer. + // Note that mediasession.cc will handle intersection our preferred + // direction with the offered direction. + GenerateMediaDescriptionOptions( + remote_description(), + RtpTransceiverDirectionFromSendRecv(send_audio, recv_audio), + RtpTransceiverDirectionFromSendRecv(send_video, recv_video), &audio_index, + &video_index, &data_index, session_options); cricket::MediaDescriptionOptions* audio_media_description_options = !audio_index ? nullptr diff --git a/pc/peerconnection_ice_unittest.cc b/pc/peerconnection_ice_unittest.cc index b6a8124b2c..6f6bd73871 100644 --- a/pc/peerconnection_ice_unittest.cc +++ b/pc/peerconnection_ice_unittest.cc @@ -303,7 +303,7 @@ TEST_P(PeerConnectionIceTest, AnswerContainsGatheredCandidates) { EXPECT_TRUE_WAIT(callee->IsIceGatheringDone(), kIceCandidatesTimeout); - auto answer = callee->CreateAnswer(); + auto* answer = callee->pc()->local_description(); EXPECT_LT(0u, caller->observer()->GetCandidatesByMline(0).size()); EXPECT_EQ(callee->observer()->GetCandidatesByMline(0).size(), answer->candidates(0)->count()); diff --git a/pc/peerconnection_signaling_unittest.cc b/pc/peerconnection_signaling_unittest.cc index cd26f8ba51..2cb2fc7c4b 100644 --- a/pc/peerconnection_signaling_unittest.cc +++ b/pc/peerconnection_signaling_unittest.cc @@ -55,12 +55,14 @@ class PeerConnectionWrapperForSignalingTest : public PeerConnectionWrapper { } }; -class PeerConnectionSignalingTest : public ::testing::Test { +class PeerConnectionSignalingBaseTest : public ::testing::Test { protected: typedef std::unique_ptr WrapperPtr; - PeerConnectionSignalingTest() - : vss_(new rtc::VirtualSocketServer()), main_(vss_.get()) { + explicit PeerConnectionSignalingBaseTest(SdpSemantics sdp_semantics) + : vss_(new rtc::VirtualSocketServer()), + main_(vss_.get()), + sdp_semantics_(sdp_semantics) { #ifdef WEBRTC_ANDROID InitializeAndroidObjects(); #endif @@ -76,8 +78,10 @@ class PeerConnectionSignalingTest : public ::testing::Test { WrapperPtr CreatePeerConnection(const RTCConfiguration& config) { auto observer = rtc::MakeUnique(); - auto pc = pc_factory_->CreatePeerConnection(config, nullptr, nullptr, - observer.get()); + RTCConfiguration modified_config = config; + modified_config.sdp_semantics = sdp_semantics_; + auto pc = pc_factory_->CreatePeerConnection(modified_config, nullptr, + nullptr, observer.get()); if (!pc) { return nullptr; } @@ -102,16 +106,24 @@ class PeerConnectionSignalingTest : public ::testing::Test { std::unique_ptr vss_; rtc::AutoSocketServerThread main_; rtc::scoped_refptr pc_factory_; + const SdpSemantics sdp_semantics_; }; -TEST_F(PeerConnectionSignalingTest, SetLocalOfferTwiceWorks) { +class PeerConnectionSignalingTest + : public PeerConnectionSignalingBaseTest, + public ::testing::WithParamInterface { + protected: + PeerConnectionSignalingTest() : PeerConnectionSignalingBaseTest(GetParam()) {} +}; + +TEST_P(PeerConnectionSignalingTest, SetLocalOfferTwiceWorks) { auto caller = CreatePeerConnection(); EXPECT_TRUE(caller->SetLocalDescription(caller->CreateOffer())); EXPECT_TRUE(caller->SetLocalDescription(caller->CreateOffer())); } -TEST_F(PeerConnectionSignalingTest, SetRemoteOfferTwiceWorks) { +TEST_P(PeerConnectionSignalingTest, SetRemoteOfferTwiceWorks) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); @@ -119,14 +131,14 @@ TEST_F(PeerConnectionSignalingTest, SetRemoteOfferTwiceWorks) { EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); } -TEST_F(PeerConnectionSignalingTest, FailToSetNullLocalDescription) { +TEST_P(PeerConnectionSignalingTest, FailToSetNullLocalDescription) { auto caller = CreatePeerConnection(); std::string error; ASSERT_FALSE(caller->SetLocalDescription(nullptr, &error)); EXPECT_EQ("SessionDescription is NULL.", error); } -TEST_F(PeerConnectionSignalingTest, FailToSetNullRemoteDescription) { +TEST_P(PeerConnectionSignalingTest, FailToSetNullRemoteDescription) { auto caller = CreatePeerConnection(); std::string error; ASSERT_FALSE(caller->SetRemoteDescription(nullptr, &error)); @@ -142,9 +154,15 @@ TEST_F(PeerConnectionSignalingTest, FailToSetNullRemoteDescription) { // state the PeerConnection was created in before it was closed. class PeerConnectionSignalingStateTest - : public PeerConnectionSignalingTest, - public ::testing::WithParamInterface> { + : public PeerConnectionSignalingBaseTest, + public ::testing::WithParamInterface< + std::tuple> { protected: + PeerConnectionSignalingStateTest() + : PeerConnectionSignalingBaseTest(std::get<0>(GetParam())), + state_under_test_(std::make_tuple(std::get<1>(GetParam()), + std::get<2>(GetParam()))) {} + RTCConfiguration GetConfig() { RTCConfiguration config; config.certificates.push_back( @@ -152,6 +170,10 @@ class PeerConnectionSignalingStateTest return config; } + WrapperPtr CreatePeerConnectionUnderTest() { + return CreatePeerConnectionInState(state_under_test_); + } + WrapperPtr CreatePeerConnectionInState(SignalingState state) { return CreatePeerConnectionInState(std::make_tuple(state, false)); } @@ -207,10 +229,12 @@ class PeerConnectionSignalingStateTest return wrapper; } + + std::tuple state_under_test_; }; TEST_P(PeerConnectionSignalingStateTest, CreateOffer) { - auto wrapper = CreatePeerConnectionInState(GetParam()); + auto wrapper = CreatePeerConnectionUnderTest(); if (wrapper->signaling_state() != SignalingState::kClosed) { EXPECT_TRUE(wrapper->CreateOffer()); } else { @@ -222,30 +246,21 @@ TEST_P(PeerConnectionSignalingStateTest, CreateOffer) { } TEST_P(PeerConnectionSignalingStateTest, CreateAnswer) { - auto wrapper = CreatePeerConnectionInState(GetParam()); + auto wrapper = CreatePeerConnectionUnderTest(); if (wrapper->signaling_state() == SignalingState::kHaveLocalPrAnswer || wrapper->signaling_state() == SignalingState::kHaveRemoteOffer) { EXPECT_TRUE(wrapper->CreateAnswer()); } else { std::string error; ASSERT_FALSE(wrapper->CreateAnswer(RTCOfferAnswerOptions(), &error)); - if (wrapper->signaling_state() == SignalingState::kClosed) { - EXPECT_PRED_FORMAT2(AssertStartsWith, error, - "CreateAnswer called when PeerConnection is closed."); - } else if (wrapper->signaling_state() == - SignalingState::kHaveRemotePrAnswer) { - EXPECT_PRED_FORMAT2(AssertStartsWith, error, - "CreateAnswer called without remote offer."); - } else { - EXPECT_PRED_FORMAT2( - AssertStartsWith, error, - "CreateAnswer can't be called before SetRemoteDescription."); - } + EXPECT_PRED_FORMAT2( + AssertStartsWith, error, + "CreateAnswer called when PeerConnection is in a wrong state."); } } TEST_P(PeerConnectionSignalingStateTest, SetLocalOffer) { - auto wrapper = CreatePeerConnectionInState(GetParam()); + auto wrapper = CreatePeerConnectionUnderTest(); if (wrapper->signaling_state() == SignalingState::kStable || wrapper->signaling_state() == SignalingState::kHaveLocalOffer) { // Need to call CreateOffer on the PeerConnection under test, otherwise when @@ -273,7 +288,7 @@ TEST_P(PeerConnectionSignalingStateTest, SetLocalPrAnswer) { auto pranswer = CloneSessionDescription(wrapper_for_pranswer->pc()->local_description()); - auto wrapper = CreatePeerConnectionInState(GetParam()); + auto wrapper = CreatePeerConnectionUnderTest(); if (wrapper->signaling_state() == SignalingState::kHaveLocalPrAnswer || wrapper->signaling_state() == SignalingState::kHaveRemoteOffer) { EXPECT_TRUE(wrapper->SetLocalDescription(std::move(pranswer))); @@ -291,7 +306,7 @@ TEST_P(PeerConnectionSignalingStateTest, SetLocalAnswer) { CreatePeerConnectionInState(SignalingState::kHaveRemoteOffer); auto answer = wrapper_for_answer->CreateAnswer(); - auto wrapper = CreatePeerConnectionInState(GetParam()); + auto wrapper = CreatePeerConnectionUnderTest(); if (wrapper->signaling_state() == SignalingState::kHaveLocalPrAnswer || wrapper->signaling_state() == SignalingState::kHaveRemoteOffer) { EXPECT_TRUE(wrapper->SetLocalDescription(std::move(answer))); @@ -310,7 +325,7 @@ TEST_P(PeerConnectionSignalingStateTest, SetRemoteOffer) { auto offer = CloneSessionDescription(wrapper_for_offer->pc()->remote_description()); - auto wrapper = CreatePeerConnectionInState(GetParam()); + auto wrapper = CreatePeerConnectionUnderTest(); if (wrapper->signaling_state() == SignalingState::kStable || wrapper->signaling_state() == SignalingState::kHaveRemoteOffer) { EXPECT_TRUE(wrapper->SetRemoteDescription(std::move(offer))); @@ -329,7 +344,7 @@ TEST_P(PeerConnectionSignalingStateTest, SetRemotePrAnswer) { auto pranswer = CloneSessionDescription(wrapper_for_pranswer->pc()->remote_description()); - auto wrapper = CreatePeerConnectionInState(GetParam()); + auto wrapper = CreatePeerConnectionUnderTest(); if (wrapper->signaling_state() == SignalingState::kHaveLocalOffer || wrapper->signaling_state() == SignalingState::kHaveRemotePrAnswer) { EXPECT_TRUE(wrapper->SetRemoteDescription(std::move(pranswer))); @@ -347,7 +362,7 @@ TEST_P(PeerConnectionSignalingStateTest, SetRemoteAnswer) { CreatePeerConnectionInState(SignalingState::kHaveRemoteOffer); auto answer = wrapper_for_answer->CreateAnswer(); - auto wrapper = CreatePeerConnectionInState(GetParam()); + auto wrapper = CreatePeerConnectionUnderTest(); if (wrapper->signaling_state() == SignalingState::kHaveLocalOffer || wrapper->signaling_state() == SignalingState::kHaveRemotePrAnswer) { EXPECT_TRUE(wrapper->SetRemoteDescription(std::move(answer))); @@ -362,46 +377,35 @@ TEST_P(PeerConnectionSignalingStateTest, SetRemoteAnswer) { INSTANTIATE_TEST_CASE_P(PeerConnectionSignalingTest, PeerConnectionSignalingStateTest, - Combine(Values(SignalingState::kStable, + Combine(Values(SdpSemantics::kPlanB, + SdpSemantics::kUnifiedPlan), + Values(SignalingState::kStable, SignalingState::kHaveLocalOffer, SignalingState::kHaveLocalPrAnswer, SignalingState::kHaveRemoteOffer, SignalingState::kHaveRemotePrAnswer), Bool())); -TEST_F(PeerConnectionSignalingTest, - CreateAnswerSucceedsIfStableAndRemoteDescriptionIsOffer) { +// Test that CreateAnswer fails if a round of offer/answer has been done and +// the PeerConnection is in the stable state. +TEST_P(PeerConnectionSignalingTest, CreateAnswerFailsIfStable) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - ASSERT_TRUE( - caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); - - ASSERT_EQ(SignalingState::kStable, callee->signaling_state()); - EXPECT_TRUE(callee->CreateAnswer()); -} - -TEST_F(PeerConnectionSignalingTest, - CreateAnswerFailsIfStableButRemoteDescriptionIsAnswer) { - auto caller = CreatePeerConnection(); - auto callee = CreatePeerConnection(); - - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - ASSERT_TRUE( - caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); ASSERT_EQ(SignalingState::kStable, caller->signaling_state()); - std::string error; - ASSERT_FALSE(caller->CreateAnswer(RTCOfferAnswerOptions(), &error)); - EXPECT_EQ("CreateAnswer called without remote offer.", error); + EXPECT_FALSE(caller->CreateAnswer()); + + ASSERT_EQ(SignalingState::kStable, callee->signaling_state()); + EXPECT_FALSE(callee->CreateAnswer()); } // According to https://tools.ietf.org/html/rfc3264#section-8, the session id // stays the same but the version must be incremented if a later, different // session description is generated. These two tests verify that is the case for // both offers and answers. -TEST_F(PeerConnectionSignalingTest, +TEST_P(PeerConnectionSignalingTest, SessionVersionIncrementedInSubsequentDifferentOffer) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); @@ -422,14 +426,14 @@ TEST_F(PeerConnectionSignalingTest, EXPECT_LT(rtc::FromString(original_version), rtc::FromString(later_offer->session_version())); } -TEST_F(PeerConnectionSignalingTest, +TEST_P(PeerConnectionSignalingTest, SessionVersionIncrementedInSubsequentDifferentAnswer) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - auto original_answer = callee->CreateAnswerAndSetAsLocal(); + auto original_answer = callee->CreateAnswer(); const std::string original_id = original_answer->session_id(); const std::string original_version = original_answer->session_version(); @@ -443,7 +447,7 @@ TEST_F(PeerConnectionSignalingTest, rtc::FromString(later_answer->session_version())); } -TEST_F(PeerConnectionSignalingTest, InitiatorFlagSetOnCallerAndNotOnCallee) { +TEST_P(PeerConnectionSignalingTest, InitiatorFlagSetOnCallerAndNotOnCallee) { auto caller = CreatePeerConnectionWithAudioVideo(); auto callee = CreatePeerConnectionWithAudioVideo(); @@ -466,7 +470,7 @@ TEST_F(PeerConnectionSignalingTest, InitiatorFlagSetOnCallerAndNotOnCallee) { // PeerConnection and make sure we get success/failure callbacks for all of the // requests. // Background: crbug.com/507307 -TEST_F(PeerConnectionSignalingTest, CreateOffersAndShutdown) { +TEST_P(PeerConnectionSignalingTest, CreateOffersAndShutdown) { auto caller = CreatePeerConnection(); RTCOfferAnswerOptions options; @@ -491,4 +495,9 @@ TEST_F(PeerConnectionSignalingTest, CreateOffersAndShutdown) { } } +INSTANTIATE_TEST_CASE_P(PeerConnectionSignalingTest, + PeerConnectionSignalingTest, + Values(SdpSemantics::kPlanB, + SdpSemantics::kUnifiedPlan)); + } // namespace webrtc