Closed Bug 1220441 Opened 6 years ago Closed 6 years ago

ice unit tests TestStun[Tcp]ServerTrickle are at least unreliable

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
Blocking Flags:

People

(Reporter: drno, Assigned: drno)

References

Details

Attachments

(1 file)

I'm not quite sure what the TestStun[Tcp]ServerTrickle tests are suppose to prove.

We configure the fake STUN server to throw away incoming packets...
  TestStunServer::GetInstance(AF_INET)->SetActive(false);
We start gathering.
  Gather(0);
Now we assert that the round trip time between our stun client and the fake stun server is bigger then the execution time between the return of the previous statement and the next assert?!?!
  ASSERT_FALSE(StreamHasMatchingCandidate(0, "192.0.2.1"));
Now we let the fake stun server answer requests...
  TestStunServer::GetInstance(AF_INET)->SetActive(true);
  WaitForGather();
  ASSERT_TRUE(StreamHasMatchingCandidate(0, "192.0.2.1"));

I guess the point is suppose to be that the stun client keeps retransmitting its requests until it gets a response (and we hopefully don't time out).

Problem is: for TCP there are no retransmits. So if the fake STUN server was set to inactive at the time of receiving the first request it drops the request and then will never send a response!

For UDP one possible improvement could be to tell the STUN server how many requests to throw away before answering, if that is what we want to test here. For TCP I could only see us delaying the answer here so that the gathering will take time until it finishes.
Byron: any thoughts/ideas?
backlog: --- → webrtc/webaudio+
Rank: 22
Flags: needinfo?(docfaraday)
Priority: -- → P2
I think your analysis is correct, and this this test makes no sense for TCP.
Flags: needinfo?(docfaraday)
Comment on attachment 8682205 [details]
MozReview Request: Bug 1220441: Improve gather trickle ice unit tests. r?bwc,mjf

Bug 1220441: Improve gather trickle ice unit tests. r?bwc,mjf
Attachment #8682205 - Attachment description: MozReview Request: Bug 1220441: Improve gather trickle ice unit tests → MozReview Request: Bug 1220441: Improve gather trickle ice unit tests. r?bwc,mjf
Attachment #8682205 - Flags: review?(mfroman)
Attachment #8682205 - Flags: review?(docfaraday)
Just looking for one review. So whoever can get this done quicker... :-)
Attachment #8682205 - Flags: review?(mfroman)
Comment on attachment 8682205 [details]
MozReview Request: Bug 1220441: Improve gather trickle ice unit tests. r?bwc,mjf

https://reviewboard.mozilla.org/r/23989/#review21583

::: media/mtransport/test/stunserver.cpp:479
(Diff revision 2)
>      server->Process(reinterpret_cast<const uint8_t *>(message), message_len,

Looks like this can now be called even if !server->active.  Is this intended?
Comment on attachment 8682205 [details]
MozReview Request: Bug 1220441: Improve gather trickle ice unit tests. r?bwc,mjf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23989/diff/2-3/
Attachment #8682205 - Flags: review?(mfroman)
Assignee: nobody → drno
Comment on attachment 8682205 [details]
MozReview Request: Bug 1220441: Improve gather trickle ice unit tests. r?bwc,mjf

https://reviewboard.mozilla.org/r/23989/#review21893
Attachment #8682205 - Flags: review?(docfaraday) → review+
Try run is all green.
Keywords: checkin-needed
Attachment #8682205 - Flags: review?(mfroman)
https://hg.mozilla.org/mozilla-central/rev/27066ece22a9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8682205 [details]
MozReview Request: Bug 1220441: Improve gather trickle ice unit tests. r?bwc,mjf

https://reviewboard.mozilla.org/r/23989/#review30909
Attachment #8682205 - Flags: review+
You need to log in before you can comment on or make changes to this bug.