1194019 - ice_unittest IceGatherTest gathers TCP for UDP tests

Status: RESOLVED FIXED

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

Description

[Nils Ohlmeier [:drno]]

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

Comment 1

[Eric Rescorla (:ekr)]

Is this fixed by 

https://hg.mozilla.org/integration/mozilla-inbound/diff/4f0f45233e6b/media/mtransport/test/stunserver.cpp

Comment 2

[Nils Ohlmeier [:drno]]

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

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1194019: new defaults for gather tests. r?bwc

Bug 1194019: new defaults for gather tests. r?bwc

Comment 4

[Byron Campen [:bwc]]

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 5

[Nils Ohlmeier [:drno]]

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

Comment 6

[Nils Ohlmeier [:drno]]

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.

Comment 7

[Byron Campen [:bwc]]

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

Ship It!

Comment 8

[Nils Ohlmeier [:drno]]

Green try run in mozreview.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af522ac9fd4d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/af522ac9fd4d