Closed Bug 1271682 Opened 8 years ago Closed 7 years ago

Move JSEP-related tests from signaling_unittests to jsep_session_unittest

Categories

(Core :: WebRTC: Signaling, defect, P1)

Product:
Core
Component:
WebRTC: Signaling
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed
Blocking Flags:
backlog tech-debt

People

(Reporter: bwc, Assigned: dminor)

References

Details

Attachments

(1 file)

Bug 1271682 - Move JSEP-related tests from signaling_unittests to jsep_session_unittest;
58 bytes, text/x-review-board-request
bwc
: review+
Details 
      No description provided.
backlog: --- → tech-debt
Rank: 25
Priority: -- → P2
Summary: Move JSEP-related tests from signaling_unittests to jsep_unittest → Move JSEP-related tests from signaling_unittests to jsep_session_unittest
See Also: → 1316611
bwc, any chance you could look at this again? Through the dependency chain it's blocking bug 1299187, which is about de-exporting XPCOM symbols so that 3rd party code can't hook into Firefox on Windows. Likewise with bug 1271681.
Flags: needinfo?(docfaraday)
I'm probably not going to get a chance to work on this until January.
Flags: needinfo?(docfaraday)
I'll pick this one up as well.
Assignee: docfaraday → dminor
Blocks: 1316611
Rank: 25 → 17
Priority: P2 → P1
Status: NEW → ASSIGNED
Comment on attachment 8817456 [details]
Bug 1271682 - Move JSEP-related tests from signaling_unittests to jsep_session_unittest;

https://reviewboard.mozilla.org/r/97714/#review98068

Let me read through signaling_unittests and give you a more thorough list, I already see stuff in there that was never commented.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:4426
(Diff revision 1)
> +    "a=candidate:1 1 UDP 2130706431 192.168.2.3 50007 typ host\r\n"
> +    "a=candidate:2 2 UDP 2130706431 192.168.2.4 50008 typ host\r\n";
> +
> +  // The signaling state will remain "stable" because the
> +  // SetRemoteDescription call fails.
> +  nsresult rv = mSessionAns.SetRemoteDescription(kJsepSdpOffer, answer);

What we really want here is kJsepSdpAnswer on the mSessionOff.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:4442
(Diff revision 1)
> +  SetLocalOffer(offer);
> +  ASSERT_EQ(kJsepStateHaveLocalOffer, mSessionOff.GetState());
> +
> +  // The signaling state will remain "have-local-offer" because the
> +  // SetLocalDescription call fails.
> +  nsresult rv = mSessionOff.SetLocalDescription(kJsepSdpOffer, offer);

kJsepSdpAnswer

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:4490
(Diff revision 1)
> +  SetRemoteOffer(offer);
> +  ASSERT_EQ(kJsepStateHaveRemoteOffer, mSessionAns.GetState());
> +
> +  // The signaling state will remain "have-remote-offer" because the
> +  // SetRemoteDescription call fails.
> +  nsresult rv = mSessionAns.SetRemoteDescription(kJsepSdpOffer, offer);

kJsepSdpAnswer
Attachment #8817456 - Flags: review?(docfaraday)
No longer blocks: 1316611
Comment on attachment 8817456 [details]
Bug 1271682 - Move JSEP-related tests from signaling_unittests to jsep_session_unittest;

https://reviewboard.mozilla.org/r/97714/#review101926

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:4389
(Diff revisions 1 - 2)
> +  CreateAnswer();
> +  AddTracks(mSessionAns, "video");

Shouldn't these two be switched? Also, since this is an OfferAnswer test, we also need SetRemoteAnswer/SetLocalAnswer.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:4407
(Diff revisions 1 - 2)
> +  CreateAnswer();
> +  AddTracks(mSessionAns, "audio");

Same here.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:4441
(Diff revisions 1 - 2)
> +  SdpMediaSection& mediaSection = munge->GetMediaSection(0);
> +  mediaSection.AddCodec("8", "PCMA", 8000, 1);
> +  std::string sdpString = munge->ToString();
> +
> +  SetLocalAnswer(sdpString);
> +  SetRemoteAnswer(sdpString);

Set the original answer on this one; I'm not sure JsepSessionImpl will like it if you mess with its codecs like this.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:4453
(Diff revisions 1 - 2)
> +  rv = mSessionOff.AddLocalIceCandidate(strSampleCandidate,
> +                                        nSamplelevel, &mid, &skipped);
> +  ASSERT_EQ(NS_OK, rv);

This test in signaling_unittests was testing setting a remote ICE candidate, and verifying that it didn't work (because it was the wrong state).

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:4473
(Diff revisions 1 - 2)
> +  std::string offer = CreateOffer();
> +  nsresult rv = mSessionAns.SetLocalDescription(kJsepSdpAnswer, offer);
> +  ASSERT_EQ(NS_ERROR_UNEXPECTED, rv);

The original test set the offer on the answerer first.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:4485
(Diff revisions 1 - 2)
> +  std::string ufrag = "a=ice-ufrag:Offerer-ufrag\r\n";
> +  std::size_t pos = offer.find(ufrag);
> +  ASSERT_NE(pos, std::string::npos);
> +  offer.replace(pos, ufrag.length(), "");

Typically I'll just sabotage by rewriting a=ice-ufrag to a=ice-ufrog or something. Less potential for breakage if the ufrag value changes down the line or something.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:4573
(Diff revisions 1 - 2)
> +
> +  std::string answer = CreateAnswer();
> +  SetLocalAnswer(answer);
> +  ASSERT_NE(mSessionAns.GetLocalDescription().find("UDP/TLS/RTP/SAVPF 9\r"),
> +            std::string::npos);
> +  ASSERT_NE(mSessionAns.GetLocalDescription().find("a=rtpmap:9 G722/8000"), std::string::npos);

Fold to 80.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:4608
(Diff revisions 1 - 2)
> +  AddTracks(mSessionOff, "audio");
> +  AddTracks(mSessionAns, "audio");

This test should have a video track too.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:4940
(Diff revisions 1 - 2)
> +// In this test we will change the answer SDP's a=setup value
> +// from active to passive.  This will make both sides do
> +// active and should not connect.
> +// TODO: This was originally a signaling_unittest and checked that no
> +// connection ocurred. Without that check, I'm not sure this test does much.
> +TEST_F(JsepSessionTest, AudioCallMismatchDtlsRoles)
> +{
> +  types.push_back(SdpMediaSection::kAudio);
> +  AddTracks(mSessionOff, "audio");
> +  AddTracks(mSessionAns, "audio");
> +  std::string offer = CreateOffer();
> +  std::string actpass = "\r\na=setup:actpass";
> +  size_t match = offer.find(actpass);
> +  ASSERT_NE(match, std::string::npos);
> +  SetLocalOffer(offer);
> +  SetRemoteOffer(offer);
> +  std::string answer = CreateAnswer();
> +  std::string active = "\r\na=setup:active";
> +  match = answer.find(active);
> +  ASSERT_NE(match, std::string::npos);
> +  SetLocalAnswer(answer);
> +  answer.replace(match, active.length(), "\r\na=setup:passive");
> +  SetRemoteAnswer(answer);
> +}

JsepSessionImpl should barf on the SetRemoteAnswer, and maybe also on the SetLocalAnswer. We can test that I guess.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:5037
(Diff revisions 1 - 2)
> +  std::vector<std::string> to_delete = {
> +    "a=rtcp-fb:120", "a=rtpmap:120",
> +    "a=rtcp-fb:126", "a=rtpmap:126",
> +    "a=fmtp:126"};
> +
> +  for (auto &s: to_delete) {
> +    size_t match = offer.find(s);
> +    fprintf(stdout, "matching >>>> %s\n", s.c_str());
> +    ASSERT_NE(match, std::string::npos);
> +    size_t end = offer.find("\r\n", match);
> +    ASSERT_NE(std::string::npos, end);
> +    offer.replace(match, end - match + 2, "");
> +  }

It would probably be cleaner to use the same sort of approach that ForceH264 does here.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:5087
(Diff revisions 1 - 2)
> +  ASSERT_NE(answer.find("\r\na=rtpmap:120 VP8/90000"), std::string::npos);
> +
> +  size_t match = answer.find("UDP/TLS/RTP/SAVPF 120");
> +  ASSERT_NE(std::string::npos, match);
> +  answer.replace(match,
> +                 strlen("UDP/TLS/RTP/SAVPF 120"),
> +                 "UDP/TLS/RTP/SAVPF 126");
> +
> +  match = answer.find("\r\na=rtpmap:120 VP8/90000");
> +  ASSERT_NE(std::string::npos, match);
> +  answer.replace(match,
> +                 strlen("\r\na=rtpmap:126 H264/90000"),
> +                 "\r\na=rtpmap:126 H264/90000");
> +
> +  match = answer.find("\r\na=rtcp-fb:120 nack");
> +  ASSERT_NE(std::string::npos, match);
> +  answer.replace(match,
> +                 strlen("\r\na=rtcp-fb:126 nack"),
> +                 "\r\na=rtcp-fb:126 nack");
> +
> +  match = answer.find("\r\na=rtcp-fb:120 nack pli");
> +  ASSERT_NE(std::string::npos, match);
> +  answer.replace(match,
> +                 strlen("\r\na=rtcp-fb:126 nack pli"),
> +                 "\r\na=rtcp-fb:126 nack pli");
> +
> +  match = answer.find("\r\na=rtcp-fb:120 ccm fir");
> +  ASSERT_NE(std::string::npos, match);
> +  answer.replace(match,
> +                 strlen("\r\na=rtcp-fb:126 ccm fir"),
> +                 "\r\na=rtcp-fb:126 ccm fir");
> +
> +  std::cout << "Modified SDP " << answer << std::endl;

Same deal here.
Attachment #8817456 - Flags: review?(docfaraday)
Comment on attachment 8817456 [details]
Bug 1271682 - Move JSEP-related tests from signaling_unittests to jsep_session_unittest;

https://reviewboard.mozilla.org/r/97714/#review101926

> JsepSessionImpl should barf on the SetRemoteAnswer, and maybe also on the SetLocalAnswer. We can test that I guess.

As far as I can tell, we don't have anything that will detect this situation right now. Do you want me to file a follow up bug to add a check for this, maybe in JsepSessionImpl::ValidateAnswer?
Comment on attachment 8817456 [details]
Bug 1271682 - Move JSEP-related tests from signaling_unittests to jsep_session_unittest;

https://reviewboard.mozilla.org/r/97714/#review102308

r+ with one small fix.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:4488
(Diff revisions 2 - 3)
> -  nsresult rv = mSessionAns.SetLocalDescription(kJsepSdpAnswer, offer);
> +  nsresult rv = mSessionAns.SetLocalDescription(kJsepSdpOffer, offer);
> +  ASSERT_EQ(NS_ERROR_UNEXPECTED, rv);

Do this as a SetRemoteOffer.
Attachment #8817456 - Flags: review?(docfaraday) → review+
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/54f270d10148
Move JSEP-related tests from signaling_unittests to jsep_session_unittest; r=bwc
https://hg.mozilla.org/mozilla-central/rev/54f270d10148
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: