Consistently suggest ephemeral port for QUIC client

net/quic/port_suggester_unittest.cc

Index: net/quic/port_suggester_unittest.cc
diff --git a/net/quic/port_suggester_unittest.cc b/net/quic/port_suggester_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..4413aac94cf9d673e361d517a3f476c08c96a75d
--- /dev/null
+++ b/net/quic/port_suggester_unittest.cc
@@ -0,0 +1,110 @@
+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/quic/port_suggester.h"
+
+#include <set>
+
+#include "base/basictypes.h"
+#include "net/base/host_port_pair.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+namespace test {
+
+class PortSuggesterTest : public ::testing::Test {
+ protected:
+  PortSuggesterTest()
+      : entropy_(3456789),
+        min_ephemeral_port_(1025),
+        max_ephemeral_port_(65535) {
+  }
+
+  uint64 entropy_;
+  int min_ephemeral_port_;
+  int max_ephemeral_port_;
+};
+
+TEST_F(PortSuggesterTest, SmallRangeTest) {
+  // When the range is small (one wide), we always get that as our answer.
+  scoped_refptr<PortSuggester> port_suggester =
+      new PortSuggester(HostPortPair("www.example.com", 443), entropy_);
+  // Test this for a few different (small) ranges.
+  for (int port = 2000; port < 2010; ++port) {
+    // Use |port| for both |min| and |max| delimiting the suggestion range.
+    EXPECT_EQ(port, port_suggester->SuggestPort(port, port));
+  }
+}
+
+TEST_F(PortSuggesterTest, SuggestAllPorts) {
+  // We should eventually fill out any range, but we'll just ensure that we
+  // fill out a small range of ports.
+  scoped_refptr<PortSuggester> port_suggester =
+      new PortSuggester(HostPortPair("www.example.com", 443), entropy_);
+  std::set<int> ports;
+  const int port_range = 20;
+  int insertion_limit = 200;  // We should be done by then.

[Ryan Hamilton, 2013/12/07 01:46:14]

nit: if port_range is const, should insertion limit be const, too?

[jar (doing other things), 2013/12/07 03:12:18]

Done.

+  for (int i = 0; i < insertion_limit; ++i) {
+    ports.insert(port_suggester->SuggestPort(min_ephemeral_port_,
+        min_ephemeral_port_ + port_range - 1));
+    if (ports.size() == static_cast<size_t>(port_range)) {

[Ryan Hamilton, 2013/12/07 01:46:14]

instead of casting, how about making port_range a size_t?

[jar (doing other things), 2013/12/07 03:12:18]

...then I'd have to cast when I pass the range into the SuggestPort() function,
that takes two ints :-/.

[Ryan Hamilton, 2013/12/07 04:52:22]

Are you sure that's right?  In my experience it's not a problem to pass in
unsigned values to signed functions.  The problem comes when you *compare*
signed to unsigned.  Can you switch and see what the compiler says?  (We hit
this a lot in QUIC, and I *thought* that I understood the rules, but who knows!
:>)

[Ryan Hamilton, 2013/12/07 23:21:54]

Ping. 

[jar (doing other things), 2013/12/08 01:38:36]

Unsigned is consider the larger type.  Passing a large type unsigned into a
smaller signed parameter induces a warning... but maybe we don't enable it
(anymore??).

I made the change... and ran the try bots... and I'll see what the various
settings say.

If it can stay green, I'd much rather avoid the cast.

[Ryan Hamilton, 2013/12/08 04:00:29]

*nod*  As far as I know, we do not warn/error on narrowing conversions, only on
signed/unsigned comparison.  But every time I think I know what's going on, I
learn something new, so the bots will tell :>
 

+      break;
+    }
+  }
+  EXPECT_EQ(static_cast<size_t>(port_range), ports.size());
+}
+
+TEST_F(PortSuggesterTest, AvoidDuplication) {
+  // When the range is large, duplicates are rare, but we'll ask for a few

[Ryan Hamilton, 2013/12/07 01:46:14]

There have been a few threads on chromium-dev about making sure that "random"
tests don't occasionally fail.  It looks like these tests are fine, because
you're seeding with a fixed constant, the behavior is totally consistent.  Is
that right?

[jar (doing other things), 2013/12/07 03:12:18]

Exactly.  All values and algorithms are fixed...  It can fail only if the
algorithms changed a lot... and since we're using standards to hash... than it
unlikely.

These tests just show that we're not doing anything dumb, ignoring args, not
calling a hash function, etc.

[Ryan Hamilton, 2013/12/07 04:52:22]

Great!

[jar (doing other things), 2013/12/08 01:38:36]

There is a tiny chance that some big-endian  issue may bite us, as we move the
hash into a uint64 (or similar via unions).  In that case, we'll get a busted
test... and need to find some set of tests and entropy that will work on both
architectures... but it shouldn't be hard to find such.

[Ryan Hamilton, 2013/12/08 04:00:29]

SGTM

+  // suggestions and make sure they are unique.
+  scoped_refptr<PortSuggester> port_suggester =
+      new PortSuggester(HostPortPair("www.example.com", 443), entropy_);
+  std::set<int> ports;
+  const int port_count = 200;
+  for (int i = 0; i < port_count; ++i) {
+    ports.insert(port_suggester->SuggestPort(min_ephemeral_port_,
+                                             max_ephemeral_port_));
+  }
+  EXPECT_EQ(static_cast<size_t>(port_count), ports.size());

[Ryan Hamilton, 2013/12/07 01:46:14]

ditto about the cast

[jar (doing other things), 2013/12/07 03:12:18]

Done.

+}
+
+TEST_F(PortSuggesterTest, ConsistentPorts) {
+  // For given hostname, port, and entropy, we should always get the same
+  // suggestions.
+  scoped_refptr<PortSuggester> port_suggester1 =
+      new PortSuggester(HostPortPair("www.example.com", 443), entropy_);
+  scoped_refptr<PortSuggester> port_suggester2 =
+      new PortSuggester(HostPortPair("www.example.com", 443), entropy_);
+  for (int test_count = 20; test_count > 0; --test_count) {
+    EXPECT_EQ(port_suggester1->SuggestPort(min_ephemeral_port_,
+                                           min_ephemeral_port_),
+              port_suggester2->SuggestPort(min_ephemeral_port_,
+                                           min_ephemeral_port_));
+  }
+}
+
+TEST_F(PortSuggesterTest, DifferentHostPorts) {

[Ryan Hamilton, 2013/12/07 01:46:14]

nit: Since you also vary the entropy in the tests, should you add Entropy to the
test name?

[jar (doing other things), 2013/12/07 03:12:18]

Done.

+  // When we have different hosts, port, or entropy, we probably won't collide.
+  scoped_refptr<PortSuggester> port_suggester[] = {
+      new PortSuggester(HostPortPair("www.example.com", 80), entropy_),
+      new PortSuggester(HostPortPair("www.example.ORG", 80), entropy_),

[wtc, 2013/12/07 01:03:38]

Nit: just curious why you capitalize "org"?

[jar (doing other things), 2013/12/07 03:12:18]

To make it easy to see that was the difference from the first line.

+      new PortSuggester(HostPortPair("www.example.com", 81), entropy_),
+      new PortSuggester(HostPortPair("www.example.com", 80), entropy_ + 1),
+  };
+
+  std::set<int> ports;
+  const int port_count = 40;
+  size_t insertion_count = 0;
+  for (size_t j = 0; j < arraysize(port_suggester); ++j) {
+    for (int i = 0; i < port_count; ++i) {
+     ports.insert(port_suggester[j]->SuggestPort(min_ephemeral_port_,
+                                                 max_ephemeral_port_));
+     ++insertion_count;
+    }
+  }
+  EXPECT_EQ(insertion_count, ports.size());
+}
+
+}  // namespace test
+}  // namespace net