Allow packets to be reordered in the fake network pipe.

webrtc/test/fake_network_pipe_unittest.cc

 /*
  *  Copyright (c) 2012 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 "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #include "webrtc/base/scoped_ptr.h"
 #include "webrtc/call.h"
 #include "webrtc/system_wrappers/include/clock.h"
 #include "webrtc/test/fake_network_pipe.h"
 
 using ::testing::_;
 using ::testing::AnyNumber;
 using ::testing::Return;
 using ::testing::Invoke;
 
 namespace webrtc {
 
-class MockReceiver : public PacketReceiver {
+class UnitTestReceiver : public PacketReceiver {

[stefan-webrtc, 2016/01/25 13:16:57]

TestReceiver would be enough

[philipel, 2016/01/26 10:50:47]

Done.

  public:
-  MockReceiver() {}
-  virtual ~MockReceiver() {}
+  UnitTestReceiver() {}
+  virtual ~UnitTestReceiver() {}
 
   void IncomingPacket(const uint8_t* data, size_t length) {
     DeliverPacket(MediaType::ANY, data, length, PacketTime());
     delete [] data;
   }
 
-  MOCK_METHOD4(
+  virtual MOCK_METHOD4(
       DeliverPacket,
       DeliveryStatus(MediaType, const uint8_t*, size_t, const PacketTime&));
 };
 
+class UnitTestImplReceiver : public UnitTestReceiver {

[stefan-webrtc, 2016/01/25 13:16:57]

Impl suggests that this is implementing from an interface, which it's not.

I'd suggest naming this ReorderTestReceiver and have it inherit directly from
PacketReceiver. Then you can simply reset receiver_ with a ReorderTestReceiver
in the test cases where you need it.

[philipel, 2016/01/26 10:50:47]

I have tried to do this but then the receiver_ has to be casted to either a
TestReceiver or a ReorderedTestReceiver for every test case to either set up the
mock behavior or to access the delivered_sequence_numbers_.

[stefan-webrtc, 2016/01/26 11:50:46]

OK, I can live with how it is now.

+ public:
+  UnitTestImplReceiver() {}
+  virtual ~UnitTestImplReceiver() {}
+
+  DeliveryStatus DeliverPacket(MediaType media_type,
+                               const uint8_t* packet,
+                               size_t length,
+                               const PacketTime& packet_time) override {
+    int seq_num;
+    memcpy(&seq_num, packet, sizeof(int));
+    delivered_sequence_numbers_.push_back(seq_num);
+    return PacketReceiver::DELIVERY_OK;
+  }
+  std::vector<int> delivered_sequence_numbers_;
+};
+
 class FakeNetworkPipeTest : public ::testing::Test {
  public:
   FakeNetworkPipeTest() : fake_clock_(12345) {}
 
  protected:
   virtual void SetUp() {
-    receiver_.reset(new MockReceiver());
+    receiver_.reset(new UnitTestReceiver());
     ON_CALL(*receiver_, DeliverPacket(_, _, _, _))
         .WillByDefault(Return(PacketReceiver::DELIVERY_OK));
+    receiver_impl_.reset(new UnitTestImplReceiver());
   }
 
   virtual void TearDown() {
   }
 
-  void SendPackets(FakeNetworkPipe* pipe, int number_packets, int kPacketSize) {
-    rtc::scoped_ptr<uint8_t[]> packet(new uint8_t[kPacketSize]);
+  void SendPackets(FakeNetworkPipe* pipe, int number_packets, int packet_size) {
+    RTC_DCHECK_GE(packet_size, static_cast<int>(sizeof(int)));
+    rtc::scoped_ptr<uint8_t[]> packet(new uint8_t[packet_size]);
     for (int i = 0; i < number_packets; ++i) {
-      pipe->SendPacket(packet.get(), kPacketSize);
+      // Set a sequence number for the packets by
+      // using the first bytes in the packet.
+      memcpy(packet.get(), &i, sizeof(int));
+      pipe->SendPacket(packet.get(), packet_size);
     }
   }
 
-  int PacketTimeMs(int capacity_kbps, int kPacketSize) const {
-    return 8 * kPacketSize / capacity_kbps;
+  int PacketTimeMs(int capacity_kbps, int packet_size) const {
+    return 8 * packet_size / capacity_kbps;
   }
 
   SimulatedClock fake_clock_;
-  rtc::scoped_ptr<MockReceiver> receiver_;
+  rtc::scoped_ptr<UnitTestReceiver> receiver_;
+  rtc::scoped_ptr<UnitTestImplReceiver> receiver_impl_;
 };
 
 void DeleteMemory(uint8_t* data, int length) { delete [] data; }
 
 // Test the capacity link and verify we get as many packets as we expect.
 TEST_F(FakeNetworkPipeTest, CapacityTest) {
   FakeNetworkPipe::Config config;
   config.queue_length_packets = 20;
   config.link_capacity_kbps = 80;
   rtc::scoped_ptr<FakeNetworkPipe> pipe(
@@ skipping 223 matching lines @@
     EXPECT_CALL(*receiver_, DeliverPacket(_, _, _, _)).Times(1);
     pipe->Process();
   }
 
   // Check that all the packets were sent.
   EXPECT_EQ(static_cast<size_t>(2 * kNumPackets), pipe->sent_packets());
   fake_clock_.AdvanceTimeMilliseconds(pipe->TimeUntilNextProcess());
   EXPECT_CALL(*receiver_, DeliverPacket(_, _, _, _)).Times(0);
   pipe->Process();
 }
+
+// At first don't disallow reordering and then allow reordering.

[philipel, 2016/01/20 15:25:23]

Woops, commend fixed.

+TEST_F(FakeNetworkPipeTest, DisallowReorderingThenAllowReordering) {
+  FakeNetworkPipe::Config config;
+  config.queue_length_packets = 1000;
+  config.link_capacity_kbps = 800;
+  config.queue_delay_ms = 100;
+  config.delay_standard_deviation_ms = 10;
+  rtc::scoped_ptr<FakeNetworkPipe> pipe(
+      new FakeNetworkPipe(&fake_clock_, config));
+  pipe->SetReceiver(receiver_impl_.get());
+
+  // Add 1000 packets of 1000 bytes, = 10 kB.

[stefan-webrtc, 2016/01/25 13:16:57]

Remove ","

[philipel, 2016/01/26 10:50:47]

Removed comment all together.

+  const uint32_t kNumPackets = 100;
+  const int kPacketSize = 10;

[stefan-webrtc, 2016/01/25 13:16:57]

How does this add 1000 packets of 1000 bytes? Looks like 100 packets of 10
bytes. :)

[philipel, 2016/01/26 10:50:47]

No idea :)

+  SendPackets(pipe.get(), kNumPackets, kPacketSize);
+  fake_clock_.AdvanceTimeMilliseconds(1000);
+  pipe->Process();
+
+  // Confirm that all packets have been delivered in order.
+  EXPECT_EQ(receiver_impl_->delivered_sequence_numbers_.size(), kNumPackets);
+  int last_seq_num = -1;
+  for(int seq_num : receiver_impl_->delivered_sequence_numbers_) {
+    EXPECT_GT(seq_num, last_seq_num);
+    last_seq_num = seq_num;
+  }
+
+  config.allow_reordering = true;
+  pipe->SetConfig(config);
+  SendPackets(pipe.get(), kNumPackets, kPacketSize);
+  fake_clock_.AdvanceTimeMilliseconds(1000);
+  receiver_impl_->delivered_sequence_numbers_.clear();
+  pipe->Process();
+
+  // Confirm that all packets have been delivered
+  // and that reordering has occured.
+  EXPECT_EQ(receiver_impl_->delivered_sequence_numbers_.size(), kNumPackets);

[stefan-webrtc, 2016/01/25 13:16:57]

Expectation first, i.e., EXPECT_EQ(kNumPackets, ...);

+  bool reordering_has_occured = false;
+  last_seq_num = -1;
+  for(int seq_num : receiver_impl_->delivered_sequence_numbers_) {
+    if(last_seq_num > seq_num) {
+      reordering_has_occured = true;
+      break;
+    }
+    last_seq_num = seq_num;
+  }
+  EXPECT_TRUE(reordering_has_occured);

[stefan-webrtc, 2016/01/25 13:16:57]

Can this be flaky, or can you make sure it always succeeds by seeding the random
generator? Make sure the FakeNetworkPipe uses the Random class and not rand()
since rand() generates different sequences on different platforms.

[philipel, 2016/01/26 10:50:47]

Can't say for sure if it's flaky. The random function is never seeded so it
always start with the same seed, but I'm not sure if these test cases run in
isolation or if they share state (rand internal state) between test cases.

Anyhow, I have now changed in the FakeNetworkPipe to use the Random class and it
is now possible its internal random generator on construction.

[stefan-webrtc, 2016/01/26 11:50:46]

Good. rand() doesn't generate the same sequences on all platforms, so it's a lot
better to use Random.

+}
 }  // namespace webrtc