Create unit tests for RtpDemuxer

webrtc/call/rtp_demuxer_unittest.cc

Index: webrtc/call/rtp_demuxer_unittest.cc
diff --git a/webrtc/call/rtp_demuxer_unittest.cc b/webrtc/call/rtp_demuxer_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d5afc5778be0151ce261342353ae2843a55f2c02
--- /dev/null
+++ b/webrtc/call/rtp_demuxer_unittest.cc
@@ -0,0 +1,159 @@
+/*
+ *  Copyright (c) 2017 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "webrtc/call/rtp_demuxer.h"
+
+#include <memory>
+
+#include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h"

[danilchap, 2017/05/24 17:14:02]

put it inbetween webrtc/base and webrtc/test (alphabetical order)

[eladalon, 2017/05/25 08:00:05]

I will explain my reasoning, and if you still prefer this to be moved, I will
move, and take note of the preference in the future.

My reasoning was that this section had the test-specific headers - what we test
(RtpPacket, etc.) - whereas the next section had the headers for test
infrastructure - implementation details of *how* we test.
Wdyt?

(I realize my approach collides with putting rtp_demuxer.h first. I'd rather
move it back here, then.)

[danilchap, 2017/05/29 08:59:53]

I'm not sure why introduce own convention when there is already one.
1st refactoring will reorder headers alphabetically anyway. It might be even
done without looking, using a script.

[eladalon, 2017/05/30 14:23:37]

Done.

+
+#include "webrtc/base/arraysize.h"
+#include "webrtc/base/checks.h"
+#include "webrtc/test/gmock.h"
+#include "webrtc/test/gtest.h"
+
+namespace webrtc {
+
+namespace {
+
+class MockRtpPacketSink : public RtpPacketSinkInterface {
+ public:
+  MOCK_METHOD1(OnRtpPacket, void(const RtpPacketReceived&));
+};
+
+constexpr uint32_t kSsrcs[] = {101, 202, 303};
+
+MATCHER_P(RtpPacketReceivedSsrcMatcher, other, "") {
+  return arg.Ssrc() == other.Ssrc();
+}
+
+std::unique_ptr<RtpPacketReceived> GetRtpPacketReceived(uint32_t ssrc) {

[danilchap, 2017/05/24 17:14:02]

may be call it Create...

[eladalon, 2017/05/25 08:00:05]

Done.

+  std::unique_ptr<RtpPacketReceived> packet(new RtpPacketReceived());

[danilchap, 2017/05/24 17:14:02]

may be to avoid mentioning RtpPacketReceived twice:
auto packet = rtc::MakeUnique<RtpPacketReceived>();
(using relatively new base/ptr_util.h)

[eladalon, 2017/05/25 08:00:05]

Thank you for bringing this utility function to my attention. I wonder if it
might not be better to use it in cases that require more verbose code (pointer
to an iterator over a map of vectors of iterators...), and in this case keep it
simple? The benefit of this method is that we only construct the unique_ptr
once; with MakeUnique, we construct two, albeit the second one with a simple
move. Wdyt?

[danilchap, 2017/05/29 08:59:53]

The way I see it, MakeUnique tells 'nothing odd going here, just creating a
smart pointer using public and explicit constructor, no need to worry'.
'new' is more flexible than MakeUnique and so require extra attention: need to
check if it is properly deallocated; sometimes it mean it is not public
constructor that is used.
Personally - I always use MakeUnique when I can, try to use WrapUnique when I
can't and use new only when neither of those two work.

(Move of the unique_ptr is so cheap, I always assume it is free)

In this case I'm fine with current code: it is simple enough any way.

[eladalon, 2017/05/30 14:23:37]

Thanks for explaining your rationale for preferring MakeUnique. I've searched
the codebase and only found 103 references to it, so it's probably still being
adopted. For now, I'll only use it for complicated types, where the gain in
readability is enough to offset reader unfamiliarity and the slight associated
cost. If it seems to be gaining popularity as the standard way of doing things,
I'll join the adopters.

[nisse-webrtc, 2017/05/30 14:53:23]

MakeUnique was added recently, and I think the intention is to use it everywhere
possible. 

+  packet->SetSsrc(ssrc);
+  return packet;
+}
+
+class RtpDemuxerTest : public ::testing::Test {
+ protected:
+  RtpDemuxerTest() {
+    for (size_t i = 0; i < kNumOfSinks; i++) {
+      demuxer.AddSink(kSsrcs[i], &sinks[i]);
+    }
+  }
+
+  virtual ~RtpDemuxerTest() {

[danilchap, 2017/05/24 17:14:02]

override instead of virtual

[eladalon, 2017/05/25 08:00:05]

Done.

+    for (size_t i = 0; i < kNumOfSinks; i++) {
+      EXPECT_EQ(demuxer.RemoveSink(&sinks[i]), 1u);
+    }
+  }
+
+  RtpDemuxer demuxer;
+  static constexpr size_t kNumOfSinks = 3;

[danilchap, 2017/05/24 17:14:02]

prefer constants above constructor.
https://google.github.io/styleguide/cppguide.html#Declaration_Order

if you want to keep it here, add a comment why break common order.

[eladalon, 2017/05/25 08:00:05]

I thought you were okay with putting it here?

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
IMHO, the usage makes it clear enough why this is here. I'd prefer avoiding a
comment about order, to avoid distracting from comments about functionality.
Wdyt?

[danilchap, 2017/05/29 08:59:53]

One of the worries: in your tests you rely on fact kNumOfSinks =
arraysize(kSsrc), but those two constants are far away.
if you would use arraysize(kSsrc) instead of 3, the link would be explicit and
wouldn't require any other comment.

Another problem - is mixing together constants and regular members. My only
concern is that it goes against style and thus alarms and ask for extra
attention.

+  MockRtpPacketSink sinks[kNumOfSinks];
+};
+
+TEST_F(RtpDemuxerTest, OnRtpPacketCalledOnCorrectSink) {
+  for (size_t i = 0; i < kNumOfSinks; i++) {
+    auto packet = GetRtpPacketReceived(kSsrcs[i]);
+    EXPECT_CALL(sinks[i], OnRtpPacket(RtpPacketReceivedSsrcMatcher(*packet)));

[danilchap, 2017/05/24 17:14:02]

which line you think would be faster to understand?:
EXPECT_CALL(sinks[i], OnRtpPacket(RtpPacketReceivedSsrcMatcher(*packet)));
EXPECT_CALL(sinks[i], OnRtpPacket(SameSsrc(*packet)));
EXPECT_CALL(sinks[i], OnRtpPacket(SsrcSameAsIn(*packet)));

[eladalon, 2017/05/25 08:00:05]

I see what you mean. Sure, let's go with SsrcSameAsIn.

+    demuxer.OnRtpPacket(*packet);
+    testing::Mock::VerifyAndClearExpectations(&sinks[i]);
+  }
+}
+
+TEST_F(RtpDemuxerTest, MultipleSinksMappedToSameSsrc) {
+  // |sinks| associated with different SSRCs each. Add a few additional sinks
+  // that are all associated with one new, distinct SSRC.
+  constexpr size_t kNumOfSameSsrcSinks = 3;
+  MockRtpPacketSink same_ssrc_sinks[kNumOfSameSsrcSinks];
+  constexpr uint32_t kSharedSsrc = 404;
+  for (size_t i = 0; i < kNumOfSameSsrcSinks; i++) {
+    demuxer.AddSink(kSharedSsrc, &same_ssrc_sinks[i]);
+  }
+
+  // Reception of an RTP packet associated with the shared SSRC triggers the
+  // callback on all of the interfaces associated with it.
+  auto packet = GetRtpPacketReceived(kSharedSsrc);
+  for (size_t i = 0; i < kNumOfSameSsrcSinks; i++) {
+    EXPECT_CALL(same_ssrc_sinks[i],
+                OnRtpPacket(RtpPacketReceivedSsrcMatcher(*packet)));
+  }
+  demuxer.OnRtpPacket(*packet);
+
+  // Test-specific tear-down
+  for (size_t i = 0; i < kNumOfSameSsrcSinks; i++) {
+    EXPECT_EQ(demuxer.RemoveSink(&same_ssrc_sinks[i]), 1u);
+  }
+}
+
+TEST_F(RtpDemuxerTest, SinkMappedToMultipleSsrcs) {
+  // |sinks| associated with different SSRCs each. We set one of them to also
+  // be mapped to additional SSRCs.
+  constexpr uint32_t kSsrcsOfMultiSsrcSink[] = {404, 505, 606};
+  MockRtpPacketSink multi_ssrc_sink;
+  for (uint32_t ssrc : kSsrcsOfMultiSsrcSink) {
+    demuxer.AddSink(ssrc, &multi_ssrc_sink);
+  }
+
+  // The sink which is associated with multiple SSRCs gets the callback
+  // triggered for each of those SSRCs.
+  for (uint32_t ssrc : kSsrcsOfMultiSsrcSink) {
+    auto packet = GetRtpPacketReceived(ssrc);
+    EXPECT_CALL(multi_ssrc_sink,
+                OnRtpPacket(RtpPacketReceivedSsrcMatcher(*packet)));
+    demuxer.OnRtpPacket(*packet);
+    testing::Mock::VerifyAndClearExpectations(&multi_ssrc_sink);
+  }
+
+  // Test-specific tear-down
+  EXPECT_EQ(demuxer.RemoveSink(&multi_ssrc_sink),
+            arraysize(kSsrcsOfMultiSsrcSink));
+}
+
+TEST_F(RtpDemuxerTest, SinkRemovalSanity) {

[danilchap, 2017/05/24 17:14:02]

prefer to name test describing what it is tesing, i.e. both scenario and what
would happen, e.g. AfterSinkRemovedDoesNotForwardPacketsToIt

https://testing.googleblog.com/2014/10/testing-on-toilet-writing-descriptive....

[eladalon, 2017/05/25 08:00:05]

Good point. I've gone with "OnRtpPacketNotCalledOnRemovedSinks".

+  // |sinks| associated with different SSRCs each. We set one of them to also
+  // be mapped to additional SSRCs.
+  constexpr uint32_t kSsrcsOfMultiSsrcSink[] = {404, 505, 606};
+  MockRtpPacketSink multi_ssrc_sink;
+  for (uint32_t ssrc : kSsrcsOfMultiSsrcSink) {
+    demuxer.AddSink(ssrc, &multi_ssrc_sink);
+  }
+
+  // Remove the sink.
+  EXPECT_EQ(demuxer.RemoveSink(&multi_ssrc_sink),
+            arraysize(kSsrcsOfMultiSsrcSink));
+
+  // The removed sink does not get callbacks triggered for any of the SSRCs
+  // with which it was previously associated.

[danilchap, 2017/05/24 17:14:03]

check it with StrictMock<MockRtpPacketSink>
or with
EXPECT_CALL(multi_ssrc_sink, OnRtpPacket(_)).Times(0);

[eladalon, 2017/05/25 08:00:05]

I'm not sure why this would be needed. If you move the removal of the sink to
after this block, you'll see that the test fails already.
As for Times(0), although not strictly necessary, I guess it could be seen as
code self-documentation. (I wonder if it would fail any of the mutations I've
seen on testing-on-the-toilet recently, though, because removing the line would
not make any changes to the test's results.) In the trade-off between code
brevity and code self-documentation, what do you prefer here?

[danilchap, 2017/05/29 08:59:53]

I tried to move RemoveSink after the loop - test still pass.
(Sure, there are few extra message about unexpected calls, but by default they
are just warning, not errors.)
Personally - code self-documentation.
Preliminary brevity imho is as bad as preliminary optimization: it hurts
readability [and thus reliability] and later refactoring more than it helps.
[often you need to refactor in a different direction than initial brevity] 

Also, personally, I prefer comments written in c++ than comments written in
English: those are often checked by compiler and tools and thus more up to date.

[eladalon, 2017/05/30 14:23:37]

Sorry, I mistook a warning for an error. Thank you for your alertness!

+  for (uint32_t ssrc : kSsrcsOfMultiSsrcSink) {
+    auto packet = GetRtpPacketReceived(ssrc);
+    demuxer.OnRtpPacket(*packet);
+  }
+}
+
+#if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
+TEST_F(RtpDemuxerTest, RepeatedAssociationsForbidden) {
+  // Set-up already associated sinks[0] with kSsrcs[0]. Repeating the
+  // association is an error.
+  EXPECT_DEATH(demuxer.AddSink(kSsrcs[0], &sinks[0]), "");
+}
+
+TEST_F(RtpDemuxerTest, SinksMustBeRemovedBeforeDestruction) {
+  std::unique_ptr<RtpDemuxer> bad_demuxer(new RtpDemuxer());
+  MockRtpPacketSink sink;
+  constexpr uint32_t ssrc = 111;
+  bad_demuxer->AddSink(ssrc, &sink);
+  EXPECT_DEATH(bad_demuxer.reset(), "");
+  EXPECT_EQ(bad_demuxer->RemoveSink(&sink), 1u);
+}
+#endif
+
+}  // namespace
+}  // namespace webrtc