Closed Bug 1355947 Opened 4 years ago Closed 4 years ago

Use TestNrSocket to build a fake ICE implementation for testing

Categories

(Core :: WebRTC: Signaling, enhancement, P1)

enhancement

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 :)
Blocks: 1355950
Rank: 19
Any feedback you have on what I have so far would be much appreciated.
Attachment #8871913 - Flags: feedback?(drno)
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+
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)
Discussed in 1:1, new version of patch coming soon.
Flags: needinfo?(drno)
Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11847e70c00eee03109fec1a4f22b1319e018b39

At least one orange, I'll investigate and update the patches.
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.
Attachment #8877219 - Flags: review?(drno)
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 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+
(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!
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 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+
Status: NEW → ASSIGNED
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
https://hg.mozilla.org/mozilla-central/rev/d0d40af45f9f
https://hg.mozilla.org/mozilla-central/rev/0e4269675a0e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.