Closed Bug 1194019 Opened 6 years ago Closed 6 years ago

ice_unittest IceGatherTest gathers TCP for UDP tests

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed
Blocking Flags:

People

(Reporter: drno, Assigned: drno)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

It appears that the default on having ICE TCP on results in the very first test TestGatherFakeStunServerHostnameNoResolver to do TCP.
But flipping the default to disabled only makes that first test skip TCP. Once test TestGatherFakeStunServerTcpHostnameNoResolver ran all subsequent tests seem to have TCP enabled (enough not requested).
(In reply to Eric Rescorla (:ekr) from comment #1)
> Is this fixed by 
> 
> https://hg.mozilla.org/integration/mozilla-inbound/diff/4f0f45233e6b/media/
> mtransport/test/stunserver.cpp

Not for me :-(
Bug 1194019: new defaults for gather tests. r?bwc
Attachment #8648307 - Flags: review?(docfaraday)
Summary: ice_unittest gathers TCP for UDP tests → ice_unittest IceGatherTest gathers TCP for UDP tests
Attachment #8648307 - Flags: review?(docfaraday) → review+
Comment on attachment 8648307 [details]
MozReview Request: Bug 1194019: new defaults for gather tests. r?bwc

https://reviewboard.mozilla.org/r/16185/#review14529

::: media/mtransport/test/ice_unittest.cpp:805
(Diff revision 1)
> -    std::cerr << "Candidate initialized: " << candidate << std::endl;
> +    std::cerr << "Candidate for stream " << stream->name() << " initialized: " << candidate << std::endl;

Line wrap.

::: media/mtransport/test/ice_unittest.cpp:1151
(Diff revision 1)
> -  void EnsurePeer() {
> +  void EnsurePeer(bool tcp_enabled = false) {

I would recommend either passing an enum/flag here (eg; kEnableIceTcp) for readability. We're already moving in that direction for this stuff in bug 1193437.

::: media/mtransport/test/ice_unittest.cpp:1153
(Diff revision 1)
> -      peer_ = new IceTestPeer("P1", true, false);
> +      peer_ = new IceTestPeer("P1", true, false, false, tcp_enabled, false, false);

Line wrap (I think, reviewboard wraps before 80 but not by much).
Comment on attachment 8648307 [details]
MozReview Request: Bug 1194019: new defaults for gather tests. r?bwc

Bug 1194019: new defaults for gather tests. r?bwc
https://reviewboard.mozilla.org/r/16185/#review14529

> I would recommend either passing an enum/flag here (eg; kEnableIceTcp) for readability. We're already moving in that direction for this stuff in bug 1193437.

I think an enum would not work here as we sometimes enable multiple features. That's why I went with a bitmask.
Assignee: nobody → drno
Green try run in mozreview.
Keywords: checkin-needed
backlog: --- → webrtc/webaudio+
Rank: 25
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/af522ac9fd4d
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.