1136252 - signalling_unittests needs refactoring

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

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.

Comment 1

[Byron Campen [:bwc]]

Attachment: Part 1: Increase the number of instances of signaling_unittests that can be run in parallel

Not useful for CI, but useful for dev testing using gtest-parallel.

Comment 2

[Byron Campen [:bwc]]

Attachment: Part 2: Wait for less RTP in signaling_unittests

Comment 3

[Byron Campen [:bwc]]

Attachment: Part 3: Remove some unnecessary sleeps in signaling_unittests

Comment 4

[Byron Campen [:bwc]]

Attachment: Part 4: Disable the use of an external STUN server when running in CI

Comment 5

[Byron Campen [:bwc]]

Attachment: Part 5: Enable signaling_unittests on CI, except for Android

Comment 6

[Byron Campen [:bwc]]

Attachment: Part 6: Remove/consolidate code in signaling_unittests. Includes removing most SDP checks, since that belongs in jsep_session_unittest

Comment 7

[Byron Campen [:bwc]]

Attachment: Part 6: Remove/consolidate code in signaling_unittests. Includes removing most SDP checks, since that belongs in jsep_session_unittest

Comment 8

[Byron Campen [:bwc]]

Attachment: Part 5: Enable signaling_unittests on CI, except for Android

Comment 9

[Byron Campen [:bwc]]

Attachment: Part 7: Fix bug where candidates could be trickled before setRemote during renegotiation

Comment 10

[Byron Campen [:bwc]]

Moving the CI-related stuff over to bug 1140584, since I want to land this stuff sooner rather than later.

Comment 11

[Byron Campen [:bwc]]

Attachment: Part 4: Remove/consolidate code in signaling_unittests. Includes removing most SDP checks, since that belongs in jsep_session_unittest

Comment 12

[Byron Campen [:bwc]]

Attachment: Part 5: Fix bug where candidates could be trickled before setRemote during renegotiation

Comment 13

[Byron Campen [:bwc]]

Attachment: Part 6: Extend timeouts for RTP/RTCP, until bug 1137948 can be fixed

Comment 14

[Byron Campen [:bwc]]

Attachment: 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/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

Comment 15

[Martin Thomson [:mt:]]

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?

Comment 16

[Martin Thomson [:mt:]]

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 17

[Martin Thomson [:mt:]]

Comment on attachment 8574191 [details]
MozReview Request: bz://1136252/bwc

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

Ship It!

Comment 18

[Byron Campen [:bwc]]

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.

Comment 19

[Martin Thomson [:mt:]]

(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.

Comment 20

[Byron Campen [:bwc]]

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.

Comment 21

[Byron Campen [:bwc]]

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 22

[Byron Campen [:bwc]]

Comment on attachment 8574191 [details]
MozReview Request: bz://1136252/bwc

Carry forward r=mt

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9c890c4ff2a

Comment 23

[Byron Campen [:bwc]]

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

Comment 24

[Byron Campen [:bwc]]

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.

Comment 25

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ec8bb1c0cf8
https://hg.mozilla.org/integration/mozilla-inbound/rev/77605be85deb
https://hg.mozilla.org/integration/mozilla-inbound/rev/00ee5c75c36b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6519a9e7d3d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/856f851dcb5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/8878cee7f4fc

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4ec8bb1c0cf8
https://hg.mozilla.org/mozilla-central/rev/77605be85deb
https://hg.mozilla.org/mozilla-central/rev/00ee5c75c36b
https://hg.mozilla.org/mozilla-central/rev/6519a9e7d3d8
https://hg.mozilla.org/mozilla-central/rev/856f851dcb5d
https://hg.mozilla.org/mozilla-central/rev/8878cee7f4fc

Comment 28

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1136252 - Part 1: Increase the number of instances of signaling_unittests that can be run in parallel. r=mt

Comment 29

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1136252 - Part 2: Wait for less RTP in signaling_unittests. r=mt

Comment 30

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1136252 - Part 3: Remove some unnecessary sleeps in signaling_unittests. r=mt

Comment 31

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1136252 - Part 4: Remove/consolidate code in signaling_unittests. Includes removing most SDP checks, since that belongs in jsep_session_unittest. r=mt

Comment 32

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1136252 - Part 5: Fix bug where candidates could be trickled before setRemote during renegotiation. r=mt

Comment 33

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1136252 - Part 6: Extend timeouts for RTP/RTCP, until bug 1137948 can be fixed. r=mt