Isolates generic DnsClient from AsyncHostResolver.

net/dns/dns_client_unittest.cc

Index: net/dns/dns_client_unittest.cc
diff --git a/net/dns/dns_client_unittest.cc b/net/dns/dns_client_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..29c83f8fda48429adecd71998587879e8c01294f
--- /dev/null
+++ b/net/dns/dns_client_unittest.cc
@@ -0,0 +1,254 @@
+// Copyright (c) 2011 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_client.h"
+
+#include "base/bind.h"
+#include "base/memory/scoped_ptr.h"
+#include "net/base/net_log.h"
+#include "net/base/sys_addrinfo.h"
+#include "net/dns/dns_response.h"
+#include "net/dns/dns_session.h"
+#include "net/dns/dns_test_util.h"
+#include "net/socket/socket_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+// TODO(szym): test failures (NXDOMAIN, etc.)
+// TODO(szym): test DnsClient::Request::Start with synchronous failure
+// TODO(szym): test suffix search once implemented
+
+namespace net {
+
+namespace {

[mmenke, 2011/12/02 00:53:55]

If you don't need to make the test the friend of the classes you're testing, as
appears to be the case here, you can just stick the entire test in the anonymous
namespace.

[szym, 2011/12/05 23:06:28]

Done.

+const int kTransactionIds[] = {0, 1, 2, 3};
+const int kNumTransactions = arraysize(kTransactionIds);
+}
+
+class DnsClientTest : public testing::Test {
+ public:
+  struct TestRequest {

[mmenke, 2011/12/02 00:53:55]

This is sufficiently big and complicated that I think it should be a class.

[mmenke, 2011/12/02 00:53:55]

"TestRequest" seems like a misnomer, since it's acting more like a HostResolver,
rather than a DnsClient::Request.  TestRequestHandler or TestRequestHelper,
maybe?  I'm open to other ideas.

[szym, 2011/12/05 23:06:28]

Done.

+    void Init(const char* name,
+              uint16 type,
+              const MockWrite& write,
+              const MockRead& read,
+              int answer_count) {
+      // Must include the terminating \x00.
+      qname = std::string(name, strlen(name) + 1);
+      qtype = type;
+      expected_answer_count = answer_count;
+      completed = false;
+      writes.push_back(write);
+      reads.push_back(read);
+      data.reset(new StaticSocketDataProvider(&reads[0], reads.size(),
+                                              &writes[0], writes.size()));
+    }
+
+    void MakeRequest(DnsClient* client) {
+      DCHECK(request.get() == NULL);
+      request.reset(client->CreateRequest(
+          qname,
+          qtype,
+          base::Bind(&TestRequest::OnRequestComplete,
+                     base::Unretained(this)),
+          BoundNetLog()));
+      EXPECT_EQ(qname, request->qname());
+      EXPECT_EQ(qtype, request->qtype());
+      EXPECT_EQ(ERR_IO_PENDING, request->Start());
+    }
+
+    void Cancel() {
+      DCHECK(request.get() != NULL);

[mmenke, 2011/12/02 00:53:55]

ASSERT_TRUE(request.get()) is preferred in unit tests.  Generally more useful on
buildbots, particularly release ones.

[szym, 2011/12/05 23:06:28]

Done, although ASSERT_* does not work in functions returning non-void (e.g.,
GetNextId).

+      request.reset(NULL);
+    }
+
+    void OnRequestComplete(DnsClient::Request* req,
+                           int rv,
+                           const DnsResponse* response) {
+      EXPECT_FALSE(completed);
+      EXPECT_EQ(request.get(), req);
+
+      EXPECT_EQ(OK, rv);
+      EXPECT_EQ(expected_answer_count, response->answer_count());
+
+      DnsRecordParser parser = response->Parser();
+      DnsResourceRecord record;
+      for (int i = 0; i < expected_answer_count; ++i) {
+        EXPECT_TRUE(parser.ParseRecord(&record));
+      }
+      EXPECT_TRUE(parser.AtEnd());
+
+      completed = true;
+    }
+
+    void OnRequestCompleteCancel(DnsClient::Request* req,

[mmenke, 2011/12/02 00:53:55]

I find this function name a little vague.  It could also mean a request
"completed" by being cancelled.  I suggest you either add a comment, or rename
it to "CancelRequestOnComplete" or "CancelOnRequestComplete" or something.

[szym, 2011/12/05 23:06:28]

Done.

+                                 int rv,
+                                 const DnsResponse* response) {
+      EXPECT_FALSE(completed);
+      Cancel();
+    }
+
+    std::string qname;
+    uint16 qtype;
+    std::vector<MockWrite> writes;
+    std::vector<MockRead> reads;
+    scoped_ptr<StaticSocketDataProvider> data;
+    scoped_ptr<DnsClient::Request> request;
+    int expected_answer_count;
+
+    bool completed;
+  };
+
+  DnsClientTest()
+      : transaction_ids_(kTransactionIds,
+                         kTransactionIds + arraysize(kTransactionIds)) {
+    requests_[0].Init(
+        kT0DnsName,
+        kT0Qtype,
+        MockWrite(true, reinterpret_cast<const char*>(kT0QueryDatagram),
+                  arraysize(kT0QueryDatagram)),
+        MockRead(true, reinterpret_cast<const char*>(kT0ResponseDatagram),
+                  arraysize(kT0ResponseDatagram)),
+        arraysize(kT0IpAddresses) + 1);  // +1 for CNAME RR
+
+    requests_[1].Init(
+        kT1DnsName,
+        kT1Qtype,
+        MockWrite(true, reinterpret_cast<const char*>(kT1QueryDatagram),
+                  arraysize(kT1QueryDatagram)),
+        MockRead(true, reinterpret_cast<const char*>(kT1ResponseDatagram),
+                  arraysize(kT1ResponseDatagram)),
+        arraysize(kT1IpAddresses) + 1);  // +1 for CNAME RR
+
+    requests_[2].Init(
+        kT2DnsName,
+        kT2Qtype,
+        MockWrite(true, reinterpret_cast<const char*>(kT2QueryDatagram),
+                  arraysize(kT2QueryDatagram)),
+        MockRead(true, reinterpret_cast<const char*>(kT2ResponseDatagram),
+                  arraysize(kT2ResponseDatagram)),
+        arraysize(kT2IpAddresses) + 1);  // +1 for CNAME RR
+
+    requests_[3].Init(
+        kT3DnsName,
+        kT3Qtype,
+        MockWrite(true, reinterpret_cast<const char*>(kT3QueryDatagram),
+                  arraysize(kT3QueryDatagram)),
+        MockRead(true, reinterpret_cast<const char*>(kT3ResponseDatagram),
+                  arraysize(kT3ResponseDatagram)),
+        arraysize(kT3IpAddresses) + 2);  // +2 for CNAME RR
+
+    MockClientSocketFactory* factory = new MockClientSocketFactory();
+
+    for (int i = 0; i < kNumTransactions; ++i) {
+      factory->AddSocketDataProvider(requests_[i].data.get());
+    }
+
+    DnsConfig config;
+
+    IPEndPoint dns_server;
+    {
+      bool rv = CreateDnsAddress(kDnsIp, kDnsPort, &dns_server);
+      DCHECK(rv);

[mmenke, 2011/12/02 00:53:55]

EXPECT_EQ(OK, rv);

[szym, 2011/12/05 23:06:28]

Done.

+    }
+    config.nameservers.push_back(dns_server);
+
+    DnsSession* session = new DnsSession(
+        config,
+        factory,
+        base::Bind(&DnsClientTest::GetNextId, base::Unretained(this)),
+        NULL /* NetLog */);
+
+    client_.reset(DnsClient::CreateClient(session));
+  }
+
+  int GetNextId(int min, int max) {
+    DCHECK(!transaction_ids_.empty());

[mmenke, 2011/12/02 00:53:55]

EXPECT_FALSE(transaction_ids_.empty());

[szym, 2011/12/05 23:06:28]

Done.

+    int id = transaction_ids_.front();
+    transaction_ids_.pop_front();
+    DCHECK(id >= min && id <= max);

[mmenke, 2011/12/02 00:53:55]

EXPECT_GE(id, min);
EXPECT_LE(id, max);

[szym, 2011/12/05 23:06:28]

Done.

+    return id;
+  }
+
+ protected:
+  TestRequest requests_[kNumTransactions];
+  std::deque<int> transaction_ids_;
+  scoped_ptr<DnsClient> client_;
+};
+
+TEST_F(DnsClientTest, Lookup) {
+  requests_[0].MakeRequest(client_.get());
+
+  // wait until result

[mmenke, 2011/12/02 00:53:55]

nit:  Wait until result.

[szym, 2011/12/05 23:06:28]

Done.

+  MessageLoop::current()->RunAllPending();
+
+  EXPECT_TRUE(requests_[0].completed);
+}
+
+TEST_F(DnsClientTest, ConcurrentLookup) {
+  for (int i = 0; i < kNumTransactions; ++i) {
+    requests_[i].MakeRequest(client_.get());
+  }
+
+  MessageLoop::current()->RunAllPending();
+
+  for (int i = 0; i < kNumTransactions; ++i) {
+    EXPECT_TRUE(requests_[i].completed);
+  }
+}
+
+TEST_F(DnsClientTest, CancelLookup) {
+  for (int i = 0; i < kNumTransactions; ++i) {
+    requests_[i].MakeRequest(client_.get());
+  }
+
+  requests_[0].Cancel();
+  requests_[2].Cancel();
+
+  MessageLoop::current()->RunAllPending();
+
+  EXPECT_FALSE(requests_[0].completed);
+  EXPECT_TRUE(requests_[1].completed);
+  EXPECT_FALSE(requests_[2].completed);
+  EXPECT_TRUE(requests_[3].completed);
+}
+
+TEST_F(DnsClientTest, DestroyClient) {
+  for (int i = 0; i < kNumTransactions; ++i) {
+    requests_[i].MakeRequest(client_.get());
+  }
+
+  // Destroying the client does not affect running requests.
+  client_.reset(NULL);
+
+  MessageLoop::current()->RunAllPending();
+
+  for (int i = 0; i < kNumTransactions; ++i) {
+    EXPECT_TRUE(requests_[i].completed);
+  }
+}
+
+TEST_F(DnsClientTest, DestroyRequestFromCallback) {
+  // Custom callback to cancel the completing request.
+  requests_[0].request.reset(client_->CreateRequest(
+      requests_[0].qname,
+      requests_[0].qtype,
+      base::Bind(&TestRequest::OnRequestCompleteCancel,
+                 base::Unretained(&requests_[0])),
+      BoundNetLog()));
+  requests_[0].request->Start();
+
+  for (int i = 1; i < kNumTransactions; ++i) {
+      requests_[i].MakeRequest(client_.get());

[mmenke, 2011/12/02 00:53:55]

nit:  Too much indentation.

[szym, 2011/12/05 23:06:28]

Done.

+  }
+
+  MessageLoop::current()->RunAllPending();
+
+  EXPECT_FALSE(requests_[0].completed);
+  for (int i = 1; i < kNumTransactions; ++i) {
+    EXPECT_TRUE(requests_[i].completed);
+  }
+}
+

[mmenke, 2011/12/02 00:53:55]

Think it might be nice to have a test for a simulated failure, either due to bad
DNS data, or a network error.

[szym, 2011/12/05 23:06:28]

Done.

+}  // namespace net
+