Unit test that DnsSocketPool copies RandIntCallback

net/dns/dns_socket_pool_unittest.cc

Index: net/dns/dns_socket_pool_unittest.cc
diff --git a/net/dns/dns_socket_pool_unittest.cc b/net/dns/dns_socket_pool_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ba57e8b233f5d43384919efe51919f4e04b6ed70
--- /dev/null
+++ b/net/dns/dns_socket_pool_unittest.cc
@@ -0,0 +1,87 @@
+// Copyright 2017 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/dns/dns_socket_pool.h"
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/memory/weak_ptr.h"
+#include "net/base/rand_callback.h"
+#include "net/socket/client_socket_factory.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+namespace {
+
+class DummyObject {
+ public:
+  DummyObject() : weak_factory_(this) {}
+
+  base::WeakPtr<DummyObject> GetWeakPtr() { return weak_factory_.GetWeakPtr(); }
+
+  bool HasWeakPtrs() const { return weak_factory_.HasWeakPtrs(); }
+
+ private:
+  base::WeakPtrFactory<DummyObject> weak_factory_;

[mmenke, 2017/01/30 19:14:42]

DISALLOW_COPY_AND_ASSIGN

[Julia Tuttle, 2017/01/30 19:35:04]

Done.

+};
+
+class DummyRandIntCallback {
+ public:
+  RandIntCallback MakeCallback() {
+    return base::Bind(&DummyRandIntCallback::GetRandInt, dummy_.GetWeakPtr());
+  }
+
+  bool HasRefs() const { return dummy_.HasWeakPtrs(); }
+
+ private:
+  static int GetRandInt(base::WeakPtr<DummyObject> dummy, int from, int to) {
+    return 4;  // chosen by fair dice roll.
+               // guaranteed to be random.

[mmenke, 2017/01/30 19:14:42]

nit:  Chosen / Guaranteed.

I'd also suggest putting them before the return, rather than after it, as that's
the more common style, particularly for multi-line comments, but that's up to
you.

[Julia Tuttle, 2017/01/30 19:35:04]

It's acceptable per Randall Munroe:

https://xkcd.com/221/

As President of the Internet, I believe he outranks our style guide.

[mmenke, 2017/01/30 19:49:30]

I'm pretty sure the very name "xkcd" violates the style guide.

[asanka, 2017/01/30 20:17:18]

http://dilbert.com/strip/2001-10-25 mandates that the random number be NINE.
NINE can be #defined to be 4 though.

[mmenke, 2017/01/30 20:19:31]

Only if it's a sufficiently large 4, though.

+  }
+
+  DummyObject dummy_;

[mmenke, 2017/01/30 19:14:42]

DISALLOW_COPY_AND_ASSIGN

[Julia Tuttle, 2017/01/30 19:35:04]

Done.

+};

[mmenke, 2017/01/30 18:18:44]

Is all this needed, or does the following work:

int SillyRandCallback(int foo) {
  return foo;
}

// Could put these in the test fixture as well.
int foo = 4;
base::WeakPtrFactory<int> weak_int_factory(&foo);
pool_ = DnsSocketPool::CreateDefault(
    ClientSocketFactory::GetDefaultFactory(),
// Again, could make this a method in the test fixture
    base::Bind(&SillyRandCallback, weak_int_factory.GetWeakPtr()));

EXPECT_TRUE(weak_int_factory_.HasWeakPtrs());

[mmenke, 2017/01/30 18:25:03]

Actually, would probably have to be "int SillyRandCallback(int* foo)"

[Julia Tuttle, 2017/01/30 19:06:44]

It's not *needed*, but I think it's clearer to encapsulate things so it's plain
to see that I'm creating a callback and then checking if there are refs to it.
The "put a weak pointer into the callback args" thing is kind of magic, and
would clutter the test cases themselves.

+
+TEST(DummyRandIntCallbackTest, Referenced) {

[mmenke, 2017/01/30 19:14:42]

I think that all 4 tests are sufficiently weird that they need comments (I'd say
don't bother with the Referenced and Copied tests, since they're just checking
that callbacks work as specced, and that should be tested in base, not in net/).

[Julia Tuttle, 2017/01/30 19:35:04]

I'll comment them. The first two are mostly to ensure that my assumptions about
callbacks are correct, and demonstrate how the test class is supposed to work.

+  DummyRandIntCallback dummy;
+
+  RandIntCallback original = dummy.MakeCallback();
+  const RandIntCallback& reference = original;
+  EXPECT_EQ(4, reference.Run(0, 6));

[mmenke, 2017/01/30 19:14:42]

EXPECT_TRUE(dummy.HasRefs());?

[Julia Tuttle, 2017/01/30 19:35:04]

Oh, sure.

+
+  original.Reset();
+  EXPECT_FALSE(dummy.HasRefs());
+}
+
+TEST(DummyRandIntCallbackTest, Copied) {
+  DummyRandIntCallback dummy;
+
+  RandIntCallback original = dummy.MakeCallback();
+  RandIntCallback copy = original;
+  EXPECT_EQ(4, copy.Run(0, 6));
+
+  original.Reset();
+  EXPECT_TRUE(dummy.HasRefs());
+}
+
+class DnsSocketPoolTest : public ::testing::Test {
+ protected:
+  DummyRandIntCallback dummy_;
+  std::unique_ptr<DnsSocketPool> pool_;
+};
+
+TEST_F(DnsSocketPoolTest, DefaultCopiesCallback) {
+  pool_ = DnsSocketPool::CreateDefault(ClientSocketFactory::GetDefaultFactory(),
+                                       dummy_.MakeCallback());
+  EXPECT_TRUE(dummy_.HasRefs());
+}
+
+TEST_F(DnsSocketPoolTest, NullCopiesCallback) {
+  pool_ = DnsSocketPool::CreateNull(ClientSocketFactory::GetDefaultFactory(),
+                                    dummy_.MakeCallback());
+  EXPECT_TRUE(dummy_.HasRefs());
+}
+
+}  // namespace
+}  // namespace net