Allow packets to be reordered in the fake network pipe.

webrtc/test/fake_network_pipe_unittest.cc

Index: webrtc/test/fake_network_pipe_unittest.cc
diff --git a/webrtc/test/fake_network_pipe_unittest.cc b/webrtc/test/fake_network_pipe_unittest.cc
index ff18993829896fe4488ca18c8cdad0fd95fe5982..d50b3bedfa032f20524d94eabb7f126e39d6b98e 100644
--- a/webrtc/test/fake_network_pipe_unittest.cc
+++ b/webrtc/test/fake_network_pipe_unittest.cc
@@ -23,48 +23,71 @@ 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; }
@@ -308,4 +331,52 @@ TEST_F(FakeNetworkPipeTest, ChangingCapacityWithPacketsInPipeTest) {
   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