1355947 - Use TestNrSocket to build a fake ICE implementation for testing

Status: RESOLVED FIXED

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

Description

[Dan Minor [:dminor]]

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

Comment 1

[Dan Minor [:dminor]]

Attachment: Add ICE unit tests based on TestNrSocket

Any feedback you have on what I have so far would be much appreciated.

Comment 2

[Nils Ohlmeier [: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.

Comment 3

[Dan Minor [:dminor]]

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!

Comment 4

[Dan Minor [:dminor]]

Discussed in 1:1, new version of patch coming soon.

Comment 5

[Dan Minor [:dminor]]

Attachment: Bug 1355947 - Add NatDelegate to TestNat;

This allows tests to implement different packet handling schemes without having
to extend or modify TestNat itself.

Review commit: https://reviewboard.mozilla.org/r/148502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148502/

Comment 6

[Dan Minor [:dminor]]

Attachment: Bug 1355947 - Use TestNrSocket to build a fake ICE implementation for testing;

Review commit: https://reviewboard.mozilla.org/r/148504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148504/

Comment 7

[Dan Minor [:dminor]]

Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11847e70c00eee03109fec1a4f22b1319e018b39

At least one orange, I'll investigate and update the patches.

Comment 8

[Dan Minor [:dminor]]

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.

Comment 9

[Jan Keromnes [:janx]]

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

[Byron Campen [:bwc]]

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"

Comment 11

[Dan Minor [:dminor]]

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

[Dan Minor [:dminor]]

Comment on attachment 8877218 [details]
Bug 1355947 - Add NatDelegate to TestNat;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148502/diff/1-2/

Comment 13

[Dan Minor [:dminor]]

Comment on attachment 8877219 [details]
Bug 1355947 - Use TestNrSocket to build a fake ICE implementation for testing;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148504/diff/1-2/

Comment 14

[Dan Minor [:dminor]]

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

[Nils Ohlmeier [:drno]]

Comment on attachment 8877219 [details]
Bug 1355947 - Use TestNrSocket to build a fake ICE implementation for testing;

https://reviewboard.mozilla.org/r/148504/#review157370

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0d40af45f9f
https://hg.mozilla.org/mozilla-central/rev/0e4269675a0e