Closed Bug 1136252 Opened 5 years ago Closed 5 years ago

signalling_unittests needs refactoring

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bwc, Assigned: bwc)

References

Details

Attachments

(6 files, 13 obsolete files)

39 bytes, text/x-review-board-request
bwc
: review+
Details
39 bytes, text/x-review-board-request
bwc
: review+
Details
39 bytes, text/x-review-board-request
bwc
: review+
Details
39 bytes, text/x-review-board-request
bwc
: review+
Details
39 bytes, text/x-review-board-request
bwc
: review+
Details
39 bytes, text/x-review-board-request
bwc
: review+
Details
Currently, signaling_unittests is the only test case we have for integration testing without needing a display (as opposed to the mochitests). Unfortunately, it takes a very long time to run (longer than the mochitest suite), is full of crufty copy/paste code, and has many test-cases that belong in jsep_session_unittest. Additionally, the long run time means that this test is not run in CI, which leads to frequent bustage. Some time needs to be spent fixing these problems.
Depends on: 1137932
Depends on: 1137948
Not useful for CI, but useful for dev testing using gtest-parallel.
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Attachment #8572199 - Attachment is obsolete: true
Attachment #8570784 - Attachment is obsolete: true
Blocks: 1140584
No longer depends on: 1137948, 1137932
Attachment #8573553 - Attachment is obsolete: true
Attachment #8570783 - Attachment is obsolete: true
Moving the CI-related stuff over to bug 1140584, since I want to land this stuff sooner rather than later.
Attachment #8573552 - Attachment is obsolete: true
Attachment #8573554 - Attachment is obsolete: true
Attached file MozReview Request: bz://1136252/bwc (obsolete) —
/r/4999 - Bug 1136252 - Part 1: Increase the number of instances of signaling_unittests that can be run in parallel.
/r/5001 - Bug 1136252 - Part 2: Wait for less RTP in signaling_unittests.
/r/5003 - Bug 1136252 - Part 3: Remove some unnecessary sleeps in signaling_unittests.
/r/5005 - Bug 1136252 - Part 4: Remove/consolidate code in signaling_unittests. Includes removing most SDP checks, since that belongs in jsep_session_unittest.
/r/5007 - Bug 1136252 - Part 5: Fix bug where candidates could be trickled before setRemote during renegotiation.
/r/5009 - Bug 1136252 - Part 6: Extend timeouts for RTP/RTCP, until bug 1137948 can be fixed.

Pull down these commits:

hg pull review -r 1d97b8d410b2aeed92db1fc9223db57f3a09683a
Attachment #8574191 - Flags: review?(martin.thomson)
Comment on attachment 8574191 [details]
MozReview Request: bz://1136252/bwc

https://reviewboard.mozilla.org/r/4997/#review3999

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 1)
>    // TODO(bug 1056650): once we have multistream support, all we need to do
>    // here is run a test with two audio streams, since that will prevent the
>    // PTs from being unique

Want to check this comment?
Attachment #8574191 - Flags: review?(martin.thomson)
https://reviewboard.mozilla.org/r/4997/#review4161

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 1)
> +      ASSERT_LE(4, pipeline->rtp_packets_sent())
> +        << "Local track " << trackId << " isn't sending RTP";
> +      ASSERT_LE(1, pipeline->rtcp_packets_received())
> +        << "Local track " << trackId << " isn't receiving RTCP";

You aren't checking packet send counts on the video side here.

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 1)
> +      ASSERT_LE(4, pipeline->rtp_packets_received())
> +        << "Remote track " << trackId << " isn't receiving RTP";
> +      ASSERT_LE(1, pipeline->rtcp_packets_sent())
> +        << "Remote track " << trackId << " isn't sending RTCP";

As above.

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 1)
> -  ASSERT_GE(a1_->GetPacketsSent(0), 40);
> +  ASSERT_GE(a1_->GetPacketsSent(0), 4);
>    // In this case we should receive nothing.
>    ASSERT_EQ(a2_->GetPacketsReceived(0), 0);

Can these be switched to something identifying the stream ID rather than index?

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 1)
> +  int GetPacketsReceived(size_t stream) const {

I saw just one instance where this was called.  Inlining it in the std::string version might be nice.

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 1)
> +  int GetPacketsSent(size_t stream) const {

Ditto.

::: media/webrtc/signaling/test/signaling_unittests.cpp
(Diff revision 1)
> +  void CheckStreams()

Is the fact that this is only checking audio a problem?

Also, the two blocks in this function are wholly duplicated.
Comment on attachment 8574191 [details]
MozReview Request: bz://1136252/bwc

https://reviewboard.mozilla.org/r/4997/#review4195

Ship It!
Attachment #8574191 - Flags: review+
https://reviewboard.mozilla.org/r/4997/#review4197

> You aren't checking packet send counts on the video side here.

If I recall correctly, no video ends up flowing because we cannot do fake video in this test yet.

> Can these be switched to something identifying the stream ID rather than index?

I suppose this could be re-written to find the identifiers and use those.

> Is the fact that this is only checking audio a problem?
> 
> Also, the two blocks in this function are wholly duplicated.

Yeah, I can clean that up.
(In reply to Byron Campen [:bwc] from comment #18)
> https://reviewboard.mozilla.org/r/4997/#review4197
> 
> > You aren't checking packet send counts on the video side here.
> 
> If I recall correctly, no video ends up flowing because we cannot do fake
> video in this test yet.

OK, a comment to that effect might be wise.
https://reviewboard.mozilla.org/r/4997/#review4231

> I suppose this could be re-written to find the identifiers and use those.

Looked into this, and modifying would just end up being the same amount of code, moved into a less-common place.

> If I recall correctly, no video ends up flowing because we cannot do fake video in this test yet.

Bug filed (1142320) and comment added.
Attachment #8574191 - Flags: review+
Comment on attachment 8574191 [details]
MozReview Request: bz://1136252/bwc

/r/4999 - Bug 1136252 - Part 1: Increase the number of instances of signaling_unittests that can be run in parallel. r=mt
/r/5001 - Bug 1136252 - Part 2: Wait for less RTP in signaling_unittests. r=mt
/r/5003 - Bug 1136252 - Part 3: Remove some unnecessary sleeps in signaling_unittests. r=mt
/r/5005 - Bug 1136252 - Part 4: Remove/consolidate code in signaling_unittests. Includes removing most SDP checks, since that belongs in jsep_session_unittest. r=mt
/r/5007 - Bug 1136252 - Part 5: Fix bug where candidates could be trickled before setRemote during renegotiation. r=mt
/r/5009 - Bug 1136252 - Part 6: Extend timeouts for RTP/RTCP, until bug 1137948 can be fixed. r=mt

Pull down these commits:

hg pull review -r 0f53df6af5e22ca2023eb4e5a020f62f359d1ea6
Comment on attachment 8574191 [details]
MozReview Request: bz://1136252/bwc

Carry forward r=mt

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9c890c4ff2a
Attachment #8574191 - Flags: review+
Attachment #8570779 - Attachment is obsolete: true
Attachment #8570780 - Attachment is obsolete: true
Attachment #8570781 - Attachment is obsolete: true
Attachment #8574185 - Attachment is obsolete: true
Attachment #8574186 - Attachment is obsolete: true
Attachment #8574187 - Attachment is obsolete: true
Flags: needinfo?(docfaraday)
Seeing something that looks like infra bustage on that last try that retriggers aren't helping with. Let's see if a rebase clears things up.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=572cccf7f73c
Rank: 25
Flags: firefox-backlog+
Priority: -- → P2
Looks like try is just hosed on OS 10.6 right now. Doing some retriggers on other infra errors, and will land without the 10.6 builds and hope for the best.
Attachment #8574191 - Attachment is obsolete: true
Attachment #8619577 - Flags: review+
Attachment #8619578 - Flags: review+
Attachment #8619579 - Flags: review+
Attachment #8619580 - Flags: review+
Attachment #8619581 - Flags: review+
Attachment #8619582 - Flags: review+
You need to log in before you can comment on or make changes to this bug.