Closed
Bug 1355947
Opened 8 years ago
Closed 8 years ago
Use TestNrSocket to build a fake ICE implementation for testing
Categories
(Core :: WebRTC: Signaling, enhancement, P1)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: dminor, Assigned: dminor)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
It sounds like the basic idea is to set up a new gtest which uses TestNrSocket to create a fake ICE implementation to be able to better test nICEr.
I discussed this with Nils, and it seems like the best place to start would be to write some simple tests for things like timeouts. I'm not very familiar with the signaling code, but I'll take on the initial work and hopefully will be more familiar with it by the time I'm done :)
Updated•8 years ago
|
Rank: 19
Assignee | ||
Comment 1•8 years ago
|
||
Any feedback you have on what I have so far would be much appreciated.
Attachment #8871913 -
Flags: feedback?(drno)
Comment 2•8 years ago
|
||
Comment on attachment 8871913 [details] [diff] [review]
Add ICE unit tests based on TestNrSocket
Review of attachment 8871913 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good so far.
I did not realize how much boiler plate code this would need.
I assume we will get to the real meat once we start adding test code to msg_recvd() or by providing our version of TestNat.
::: media/mtransport/test/test_nr_socket_ice_unittest.cpp
@@ +370,5 @@
> +TEST_F(FakeIceUnitTest, TestIcePeersBlockStun) {
> + IcePeer peer("IcePeer", nullptr, NR_ICE_CTX_FLAGS_AGGRESSIVE_NOMINATION,
> + test_utils_);
> + RefPtr<TestNat> nat(new TestNat);
> + nat->block_stun_ = true;
I think it would be good if this test could ensure/verify how many requests it received from the other peer.
I guess my initial thinking here was that we would provide our own implementation of TestNat for this test which exposes more fine grained control then the current TestNat. But another option might be to extend TestNat. Although with what we probably want to do here in the future TestNat is not the appropriate name any more any how.
Attachment #8871913 -
Flags: feedback?(drno) → feedback+
Assignee | ||
Comment 3•8 years ago
|
||
Provided our own implementation of TestNat isn't as straightforward as it could be because it uses NS_INLINE_DECL_THREADSAFE_REFCOUNTING which requires a private destructor which prevents instantiating subclasses (unless I'm missing something!)
It seems like the only place we share TestNats between TestNrSockets is in the test code, so this might be a matter of refactoring the test code so that the test functions take care of TestNat destruction and stop using RefPtr<>. It might also be possible to make the TestNat type a template argument to TestNrSocket.
Before I spend too much time on this, what kind of enhancements for fine grained control do you have in mind for TestNat? Maybe we could use callbacks / delegates to move the decision making into another class without having to subclass TestNat? Thanks!
Flags: needinfo?(drno)
Assignee | ||
Comment 4•8 years ago
|
||
Discussed in 1:1, new version of patch coming soon.
Flags: needinfo?(drno)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•8 years ago
|
||
Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11847e70c00eee03109fec1a4f22b1319e018b39
At least one orange, I'll investigate and update the patches.
Assignee | ||
Comment 8•8 years ago
|
||
It appears the culprit is the timeout waiting for the connection to fail when stun is blocked. I thought it worth while testing that things work ok in the event of a timeout, but I guess to do that I'll have to either increase the period of time the test waits or set a shorter timeout.
Assignee | ||
Updated•8 years ago
|
Attachment #8877219 -
Flags: review?(drno)
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8877219 [details]
Bug 1355947 - Use TestNrSocket to build a fake ICE implementation for testing;
https://reviewboard.mozilla.org/r/148504/#review153390
Added a few drive-by nits generated by the clang-tidy static analyzer.
::: media/mtransport/test/test_nr_socket_ice_unittest.cpp:175
(Diff revision 1)
> + return candidates;
> + }
> +
> + std::vector<std::string> GetGlobalAttributes() {
> +
> + char **attrs = 0;
Nit: You could be using `nullptr` [clang-tidy: modernize-use-nullptr]
::: media/mtransport/test/test_nr_socket_ice_unittest.cpp:180
(Diff revision 1)
> + char **attrs = 0;
> + int attrct;
> + int r;
> + std::vector<std::string> ret;
> +
> + r = nr_ice_get_global_attributes(ice_ctx_, &attrs, &attrct);
Nit: Value stored to 'r' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
::: media/mtransport/test/test_nr_socket_ice_unittest.cpp:378
(Diff revision 1)
> + class NatDelegate : public TestNat::NatDelegate {
> + public:
> + NatDelegate()
> + : messages_blocked(0) {}
> +
> + virtual int on_read(TestNat *nat, void *buf, size_t maxlen, size_t *len)
Nit: You might prefer using 'override' or (rarely) 'final' instead of 'virtual' [clang-tidy: modernize-use-override]
::: media/mtransport/test/test_nr_socket_ice_unittest.cpp:383
(Diff revision 1)
> + virtual int on_read(TestNat *nat, void *buf, size_t maxlen, size_t *len)
> + {
> + return 0;
> + }
> +
> + virtual int on_sendto(TestNat *nat, const void *msg, size_t len,
Nit: Same as above [clang-tidy: modernize-use-override]
::: media/mtransport/test/test_nr_socket_ice_unittest.cpp:393
(Diff revision 1)
> + return 1;
> + }
> + return 0;
> + }
> +
> + virtual int on_write(TestNat *nat, const void *msg, size_t len, size_t *written)
Nit: Same as above [clang-tidy: modernize-use-override]
::: media/mtransport/test/test_nr_socket_ice_unittest.cpp:436
(Diff revision 1)
> + class NatDelegate : public TestNat::NatDelegate {
> + public:
> + NatDelegate()
> + : messages(0) {}
> +
> + virtual int on_read(TestNat *nat, void *buf, size_t maxlen, size_t *len)
Nit: Same as above [clang-tidy: modernize-use-override]
::: media/mtransport/test/test_nr_socket_ice_unittest.cpp:441
(Diff revision 1)
> + virtual int on_read(TestNat *nat, void *buf, size_t maxlen, size_t *len)
> + {
> + return 0;
> + }
> +
> + virtual int on_sendto(TestNat *nat, const void *msg, size_t len,
Nit: Same as above [clang-tidy: modernize-use-override]
::: media/mtransport/test/test_nr_socket_ice_unittest.cpp:452
(Diff revision 1)
> + return 1;
> + }
> + return 0;
> + }
> +
> + virtual int on_write(TestNat *nat, const void *msg, size_t len, size_t *written)
Nit: Same as above [clang-tidy: modernize-use-override]
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8877218 [details]
Bug 1355947 - Add NatDelegate to TestNat;
https://reviewboard.mozilla.org/r/148502/#review153990
::: media/mtransport/test_nr_socket.h:116
(Diff revision 1)
> * @note We deliberately avoid addref/release of TestNrSocket here to avoid
> * masking lifetime errors elsewhere.
> */
> class TestNat {
> public:
> +
Let's put a comment here that makes it clear how this is used (you can passively inspect traffic, and returning an error will cause that traffic to be dropped).
::: media/mtransport/test_nr_socket.h:119
(Diff revision 1)
> + virtual int on_read(TestNat* nat, void *buf, size_t maxlen, size_t *len) = 0;
> + virtual int on_sendto(TestNat* nat, const void *msg, size_t len,
> + int flags, nr_transport_addr *to) = 0;
> + virtual int on_write(TestNat* nat, const void *msg, size_t len, size_t *written) = 0;
"TestNat *nat"
Attachment #8877218 -
Flags: review?(docfaraday) → review+
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Jan Keromnes [:janx] from comment #9)
> Comment on attachment 8877219 [details]
> Bug 1355947 - Use TestNrSocket to build a fake ICE implementation for
> testing;
>
> https://reviewboard.mozilla.org/r/148504/#review153390
>
> Added a few drive-by nits generated by the clang-tidy static analyzer.
>
Fixed, thank you!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•8 years ago
|
||
Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff8b4b1730a55c359141c92e8a0d3e731a4dfd66
The Windows 10 ASAN job seems to be try only and is broken on other people's try pushes.
I've just removed the problematic stun blocking test for now, new and better tests seems like something for follow up bugs.
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8877219 [details]
Bug 1355947 - Use TestNrSocket to build a fake ICE implementation for testing;
https://reviewboard.mozilla.org/r/148504/#review157370
Attachment #8877219 -
Flags: review?(drno) → review+
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 16•8 years ago
|
||
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0d40af45f9f
Add NatDelegate to TestNat; r=bwc
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e4269675a0e
Use TestNrSocket to build a fake ICE implementation for testing; r=drno
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d0d40af45f9f
https://hg.mozilla.org/mozilla-central/rev/0e4269675a0e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•