Revert "Reland "Parameterize PeerConnection signaling tests for Unified Plan""

This reverts commit 7b581eb1cab0b2ccd0a2d60163bb2b73c244346a.

Reason for revert: Breaks downstream projects

Original change's description:
> 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 <deadbeef@webrtc.org>
> > Commit-Queue: Steve Anton <steveanton@webrtc.org>
> > 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 <steveanton@webrtc.org>
> Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
> Reviewed-by: Steve Anton <steveanton@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#21860}

TBR=steveanton@webrtc.org,deadbeef@webrtc.org,pthatcher@webrtc.org

Change-Id: I15490e4db3290a8ab6056cf82959be7a97e6b1c3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:8813
Reviewed-on: https://webrtc-review.googlesource.com/47340
Reviewed-by: Oleh Prypin <oprypin@webrtc.org>
Commit-Queue: Oleh Prypin <oprypin@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21865}

pc/peerconnection.cc

@@ -1674,17 +1674,20 @@ void PeerConnection::CreateAnswer(CreateSessionDescriptionObserver* observer,
     return;
   }
 
-  if (!(signaling_state_ == kHaveRemoteOffer ||
+  if (IsClosed()) {
-        signaling_state_ == kHaveLocalPrAnswer)) {
+    std::string error = "CreateAnswer called when PeerConnection is closed.";
-    std::string error =
-        "CreateAnswer called when PeerConnection is in a wrong state.";
     RTC_LOG(LS_ERROR) << error;
     PostCreateSessionDescriptionFailure(observer, error);
     return;
   }
 
-  // The remote description should be set if we're in the right state.
+  if (remote_description() &&
-  RTC_DCHECK(remote_description());
+      remote_description()->GetType() != SdpType::kOffer) {
+    std::string error = "CreateAnswer called without remote offer.";
+    RTC_LOG(LS_ERROR) << error;
+    PostCreateSessionDescriptionFailure(observer, error);
+    return;
+  }
 
   if (IsUnifiedPlan()) {
     if (options.offer_to_receive_audio != RTCOfferAnswerOptions::kUndefined) {
@@ -3483,15 +3486,18 @@ void PeerConnection::GetOptionsForPlanBAnswer(
   rtc::Optional<size_t> audio_index;
   rtc::Optional<size_t> video_index;
   rtc::Optional<size_t> 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,
+        RtpTransceiverDirectionFromSendRecv(send_video, recv_video),
-      &video_index, &data_index, session_options);
+        &audio_index, &video_index, &data_index, session_options);
+  }
 
   cricket::MediaDescriptionOptions* audio_media_description_options =
       !audio_index ? nullptr

pc/peerconnection_ice_unittest.cc

@@ -303,7 +303,7 @@ TEST_P(PeerConnectionIceTest, AnswerContainsGatheredCandidates) {
 
   EXPECT_TRUE_WAIT(callee->IsIceGatheringDone(), kIceCandidatesTimeout);
 
-  auto* answer = callee->pc()->local_description();
+  auto answer = callee->CreateAnswer();
   EXPECT_LT(0u, caller->observer()->GetCandidatesByMline(0).size());
   EXPECT_EQ(callee->observer()->GetCandidatesByMline(0).size(),
             answer->candidates(0)->count());

pc/peerconnection_signaling_unittest.cc

@@ -55,14 +55,12 @@ class PeerConnectionWrapperForSignalingTest : public PeerConnectionWrapper {
   }
 };
 
-class PeerConnectionSignalingBaseTest : public ::testing::Test {
+class PeerConnectionSignalingTest : public ::testing::Test {
  protected:
   typedef std::unique_ptr<PeerConnectionWrapperForSignalingTest> WrapperPtr;
 
-  explicit PeerConnectionSignalingBaseTest(SdpSemantics sdp_semantics)
+  PeerConnectionSignalingTest()
-      : vss_(new rtc::VirtualSocketServer()),
+      : vss_(new rtc::VirtualSocketServer()), main_(vss_.get()) {
-        main_(vss_.get()),
-        sdp_semantics_(sdp_semantics) {
 #ifdef WEBRTC_ANDROID
     InitializeAndroidObjects();
 #endif
@@ -78,10 +76,8 @@ class PeerConnectionSignalingBaseTest : public ::testing::Test {
 
   WrapperPtr CreatePeerConnection(const RTCConfiguration& config) {
     auto observer = rtc::MakeUnique<MockPeerConnectionObserver>();
-    RTCConfiguration modified_config = config;
+    auto pc = pc_factory_->CreatePeerConnection(config, nullptr, nullptr,
-    modified_config.sdp_semantics = sdp_semantics_;
+                                                observer.get());
-    auto pc = pc_factory_->CreatePeerConnection(modified_config, nullptr,
-                                                nullptr, observer.get());
     if (!pc) {
       return nullptr;
     }
@@ -106,24 +102,16 @@ class PeerConnectionSignalingBaseTest : public ::testing::Test {
   std::unique_ptr<rtc::VirtualSocketServer> vss_;
   rtc::AutoSocketServerThread main_;
   rtc::scoped_refptr<PeerConnectionFactoryInterface> pc_factory_;
-  const SdpSemantics sdp_semantics_;
 };
 
-class PeerConnectionSignalingTest
+TEST_F(PeerConnectionSignalingTest, SetLocalOfferTwiceWorks) {
-    : public PeerConnectionSignalingBaseTest,
-      public ::testing::WithParamInterface<SdpSemantics> {
- protected:
-  PeerConnectionSignalingTest() : PeerConnectionSignalingBaseTest(GetParam()) {}
-};
-
-TEST_P(PeerConnectionSignalingTest, SetLocalOfferTwiceWorks) {
   auto caller = CreatePeerConnection();
 
   EXPECT_TRUE(caller->SetLocalDescription(caller->CreateOffer()));
   EXPECT_TRUE(caller->SetLocalDescription(caller->CreateOffer()));
 }
 
-TEST_P(PeerConnectionSignalingTest, SetRemoteOfferTwiceWorks) {
+TEST_F(PeerConnectionSignalingTest, SetRemoteOfferTwiceWorks) {
   auto caller = CreatePeerConnection();
   auto callee = CreatePeerConnection();
 
@@ -131,14 +119,14 @@ TEST_P(PeerConnectionSignalingTest, SetRemoteOfferTwiceWorks) {
   EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
 }
 
-TEST_P(PeerConnectionSignalingTest, FailToSetNullLocalDescription) {
+TEST_F(PeerConnectionSignalingTest, FailToSetNullLocalDescription) {
   auto caller = CreatePeerConnection();
   std::string error;
   ASSERT_FALSE(caller->SetLocalDescription(nullptr, &error));
   EXPECT_EQ("SessionDescription is NULL.", error);
 }
 
-TEST_P(PeerConnectionSignalingTest, FailToSetNullRemoteDescription) {
+TEST_F(PeerConnectionSignalingTest, FailToSetNullRemoteDescription) {
   auto caller = CreatePeerConnection();
   std::string error;
   ASSERT_FALSE(caller->SetRemoteDescription(nullptr, &error));
@@ -154,15 +142,9 @@ TEST_P(PeerConnectionSignalingTest, FailToSetNullRemoteDescription) {
 // state the PeerConnection was created in before it was closed.
 
 class PeerConnectionSignalingStateTest
-    : public PeerConnectionSignalingBaseTest,
+    : public PeerConnectionSignalingTest,
-      public ::testing::WithParamInterface<
+      public ::testing::WithParamInterface<std::tuple<SignalingState, bool>> {
-          std::tuple<SdpSemantics, SignalingState, bool>> {
  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(
@@ -170,10 +152,6 @@ class PeerConnectionSignalingStateTest
     return config;
   }
 
-  WrapperPtr CreatePeerConnectionUnderTest() {
-    return CreatePeerConnectionInState(state_under_test_);
-  }
-
   WrapperPtr CreatePeerConnectionInState(SignalingState state) {
     return CreatePeerConnectionInState(std::make_tuple(state, false));
   }
@@ -229,12 +207,10 @@ class PeerConnectionSignalingStateTest
 
     return wrapper;
   }
-
-  std::tuple<SignalingState, bool> state_under_test_;
 };
 
 TEST_P(PeerConnectionSignalingStateTest, CreateOffer) {
-  auto wrapper = CreatePeerConnectionUnderTest();
+  auto wrapper = CreatePeerConnectionInState(GetParam());
   if (wrapper->signaling_state() != SignalingState::kClosed) {
     EXPECT_TRUE(wrapper->CreateOffer());
   } else {
@@ -246,21 +222,30 @@ TEST_P(PeerConnectionSignalingStateTest, CreateOffer) {
 }
 
 TEST_P(PeerConnectionSignalingStateTest, CreateAnswer) {
-  auto wrapper = CreatePeerConnectionUnderTest();
+  auto wrapper = CreatePeerConnectionInState(GetParam());
   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 called when PeerConnection is in a wrong state.");
+          "CreateAnswer can't be called before SetRemoteDescription.");
+    }
   }
 }
 
 TEST_P(PeerConnectionSignalingStateTest, SetLocalOffer) {
-  auto wrapper = CreatePeerConnectionUnderTest();
+  auto wrapper = CreatePeerConnectionInState(GetParam());
   if (wrapper->signaling_state() == SignalingState::kStable ||
       wrapper->signaling_state() == SignalingState::kHaveLocalOffer) {
     // Need to call CreateOffer on the PeerConnection under test, otherwise when
@@ -288,7 +273,7 @@ TEST_P(PeerConnectionSignalingStateTest, SetLocalPrAnswer) {
   auto pranswer =
       CloneSessionDescription(wrapper_for_pranswer->pc()->local_description());
 
-  auto wrapper = CreatePeerConnectionUnderTest();
+  auto wrapper = CreatePeerConnectionInState(GetParam());
   if (wrapper->signaling_state() == SignalingState::kHaveLocalPrAnswer ||
       wrapper->signaling_state() == SignalingState::kHaveRemoteOffer) {
     EXPECT_TRUE(wrapper->SetLocalDescription(std::move(pranswer)));
@@ -306,7 +291,7 @@ TEST_P(PeerConnectionSignalingStateTest, SetLocalAnswer) {
       CreatePeerConnectionInState(SignalingState::kHaveRemoteOffer);
   auto answer = wrapper_for_answer->CreateAnswer();
 
-  auto wrapper = CreatePeerConnectionUnderTest();
+  auto wrapper = CreatePeerConnectionInState(GetParam());
   if (wrapper->signaling_state() == SignalingState::kHaveLocalPrAnswer ||
       wrapper->signaling_state() == SignalingState::kHaveRemoteOffer) {
     EXPECT_TRUE(wrapper->SetLocalDescription(std::move(answer)));
@@ -325,7 +310,7 @@ TEST_P(PeerConnectionSignalingStateTest, SetRemoteOffer) {
   auto offer =
       CloneSessionDescription(wrapper_for_offer->pc()->remote_description());
 
-  auto wrapper = CreatePeerConnectionUnderTest();
+  auto wrapper = CreatePeerConnectionInState(GetParam());
   if (wrapper->signaling_state() == SignalingState::kStable ||
       wrapper->signaling_state() == SignalingState::kHaveRemoteOffer) {
     EXPECT_TRUE(wrapper->SetRemoteDescription(std::move(offer)));
@@ -344,7 +329,7 @@ TEST_P(PeerConnectionSignalingStateTest, SetRemotePrAnswer) {
   auto pranswer =
       CloneSessionDescription(wrapper_for_pranswer->pc()->remote_description());
 
-  auto wrapper = CreatePeerConnectionUnderTest();
+  auto wrapper = CreatePeerConnectionInState(GetParam());
   if (wrapper->signaling_state() == SignalingState::kHaveLocalOffer ||
       wrapper->signaling_state() == SignalingState::kHaveRemotePrAnswer) {
     EXPECT_TRUE(wrapper->SetRemoteDescription(std::move(pranswer)));
@@ -362,7 +347,7 @@ TEST_P(PeerConnectionSignalingStateTest, SetRemoteAnswer) {
       CreatePeerConnectionInState(SignalingState::kHaveRemoteOffer);
   auto answer = wrapper_for_answer->CreateAnswer();
 
-  auto wrapper = CreatePeerConnectionUnderTest();
+  auto wrapper = CreatePeerConnectionInState(GetParam());
   if (wrapper->signaling_state() == SignalingState::kHaveLocalOffer ||
       wrapper->signaling_state() == SignalingState::kHaveRemotePrAnswer) {
     EXPECT_TRUE(wrapper->SetRemoteDescription(std::move(answer)));
@@ -377,35 +362,46 @@ TEST_P(PeerConnectionSignalingStateTest, SetRemoteAnswer) {
 
 INSTANTIATE_TEST_CASE_P(PeerConnectionSignalingTest,
                         PeerConnectionSignalingStateTest,
-                        Combine(Values(SdpSemantics::kPlanB,
+                        Combine(Values(SignalingState::kStable,
-                                       SdpSemantics::kUnifiedPlan),
-                                Values(SignalingState::kStable,
                                        SignalingState::kHaveLocalOffer,
                                        SignalingState::kHaveLocalPrAnswer,
                                        SignalingState::kHaveRemoteOffer,
                                        SignalingState::kHaveRemotePrAnswer),
                                 Bool()));
 
-// Test that CreateAnswer fails if a round of offer/answer has been done and
+TEST_F(PeerConnectionSignalingTest,
-// the PeerConnection is in the stable state.
+       CreateAnswerSucceedsIfStableAndRemoteDescriptionIsOffer) {
-TEST_P(PeerConnectionSignalingTest, CreateAnswerFailsIfStable) {
   auto caller = CreatePeerConnection();
   auto callee = CreatePeerConnection();
 
-  ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
-
+  ASSERT_TRUE(
-  ASSERT_EQ(SignalingState::kStable, caller->signaling_state());
+      caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
-  EXPECT_FALSE(caller->CreateAnswer());
 
   ASSERT_EQ(SignalingState::kStable, callee->signaling_state());
-  EXPECT_FALSE(callee->CreateAnswer());
+  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_EQ(SignalingState::kStable, caller->signaling_state());
+  std::string error;
+  ASSERT_FALSE(caller->CreateAnswer(RTCOfferAnswerOptions(), &error));
+  EXPECT_EQ("CreateAnswer called without remote offer.", error);
 }
 
 // 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_P(PeerConnectionSignalingTest,
+TEST_F(PeerConnectionSignalingTest,
        SessionVersionIncrementedInSubsequentDifferentOffer) {
   auto caller = CreatePeerConnection();
   auto callee = CreatePeerConnection();
@@ -426,14 +422,14 @@ TEST_P(PeerConnectionSignalingTest,
   EXPECT_LT(rtc::FromString<uint64_t>(original_version),
             rtc::FromString<uint64_t>(later_offer->session_version()));
 }
-TEST_P(PeerConnectionSignalingTest,
+TEST_F(PeerConnectionSignalingTest,
        SessionVersionIncrementedInSubsequentDifferentAnswer) {
   auto caller = CreatePeerConnection();
   auto callee = CreatePeerConnection();
 
   ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
 
-  auto original_answer = callee->CreateAnswer();
+  auto original_answer = callee->CreateAnswerAndSetAsLocal();
   const std::string original_id = original_answer->session_id();
   const std::string original_version = original_answer->session_version();
 
@@ -447,7 +443,7 @@ TEST_P(PeerConnectionSignalingTest,
             rtc::FromString<uint64_t>(later_answer->session_version()));
 }
 
-TEST_P(PeerConnectionSignalingTest, InitiatorFlagSetOnCallerAndNotOnCallee) {
+TEST_F(PeerConnectionSignalingTest, InitiatorFlagSetOnCallerAndNotOnCallee) {
   auto caller = CreatePeerConnectionWithAudioVideo();
   auto callee = CreatePeerConnectionWithAudioVideo();
 
@@ -470,7 +466,7 @@ TEST_P(PeerConnectionSignalingTest, InitiatorFlagSetOnCallerAndNotOnCallee) {
 // PeerConnection and make sure we get success/failure callbacks for all of the
 // requests.
 // Background: crbug.com/507307
-TEST_P(PeerConnectionSignalingTest, CreateOffersAndShutdown) {
+TEST_F(PeerConnectionSignalingTest, CreateOffersAndShutdown) {
   auto caller = CreatePeerConnection();
 
   RTCOfferAnswerOptions options;
@@ -495,9 +491,4 @@ TEST_P(PeerConnectionSignalingTest, CreateOffersAndShutdown) {
   }
 }
 
-INSTANTIATE_TEST_CASE_P(PeerConnectionSignalingTest,
-                        PeerConnectionSignalingTest,
-                        Values(SdpSemantics::kPlanB,
-                               SdpSemantics::kUnifiedPlan));
-
 }  // namespace webrtc