[trac494] addressed review comments

Removed some dead code, added two unit tests, changed sendQuery() to the
more descriptive resolve(), and removed a superfluous call to dlog()

src/bin/resolver/resolver.cc

@@ -326,7 +326,6 @@ Resolver::~Resolver() {
     delete checkin_;
     delete dns_lookup_;
     delete dns_answer_;
-    dlog("Deleting the Resolver",true);
 }
 
 void
@@ -439,7 +438,7 @@ void
 ResolverImpl::resolve(const QuestionPtr& question,
     const isc::resolve::ResolverInterface::CallbackPtr& callback)
 {
-    rec_query_->sendQuery(question, callback);
+    rec_query_->resolve(question, callback);
 }
 
 void
@@ -449,7 +448,7 @@ ResolverImpl::processNormalQuery(const Question& question,
                                  DNSServer* server)
 {
     dlog("Processing normal query");
-    rec_query_->sendQuery(question, answer_message, buffer, server);
+    rec_query_->resolve(question, answer_message, buffer, server);
 }
 
 namespace {

src/lib/asiolink/asiolink.cc

@@ -479,7 +479,6 @@ public:
         MessagePtr answer_message, shared_ptr<AddressVector> upstream,
         shared_ptr<AddressVector> upstream_root,
         OutputBufferPtr buffer,
-        //DNSServer* server,
         isc::resolve::ResolverInterface::CallbackPtr cb,
         int timeout,
         unsigned retries) :
@@ -489,7 +488,6 @@ public:
         upstream_(upstream),
         upstream_root_(upstream_root),
         buffer_(buffer),
-        //server_(server->clone()),
         resolvercallback_(cb),
         timeout_(timeout),
         retries_(retries),
@@ -560,19 +558,13 @@ public:
 }
 
 void
-RecursiveQuery::sendQuery(const isc::dns::QuestionPtr& question,
+RecursiveQuery::resolve(const isc::dns::QuestionPtr& question,
     const isc::resolve::ResolverInterface::CallbackPtr callback)
 {
     asio::io_service& io = dns_service_.get_io_service();
 
     MessagePtr answer_message(new Message(Message::RENDER));
     OutputBufferPtr buffer(new OutputBuffer(0));
-    /*
-    answer_message->setOpcode(isc::dns::Opcode::QUERY());
-    isc::resolve::ResolverCallbackDirect* rcd =
-        new isc::resolve::ResolverCallbackDirect(callback,
-                                                 answer_message);
-    */
     
     // It will delete itself when it is done
     new RunningQuery(io, *question, answer_message, upstream_,
@@ -580,10 +572,10 @@ RecursiveQuery::sendQuery(const isc::dns::QuestionPtr& question,
 }
 
 void
-RecursiveQuery::sendQuery(const Question& question,
-                          MessagePtr answer_message,
-                          OutputBufferPtr buffer,
-                          DNSServer* server)
+RecursiveQuery::resolve(const Question& question,
+                        MessagePtr answer_message,
+                        OutputBufferPtr buffer,
+                        DNSServer* server)
 {
     // XXX: eventually we will need to be able to determine whether
     // the message should be sent via TCP or UDP, or sent initially via

src/lib/asiolink/asiolink.h

@@ -575,12 +575,10 @@ public:
     /// failure(). See ResolverInterface::Callback for more information.
     ///
     /// \param question The question being answered <qname/qclass/qtype>
-    /// \param callback Callback object
-    /// \param buffer An output Message into which the final response will be copied
-    /// \param buffer An output buffer into which the intermediate responses will be copied
-    /// \param server A pointer to the \c DNSServer object handling the client
-    void sendQuery(const isc::dns::QuestionPtr& question,
-                   const isc::resolve::ResolverInterface::CallbackPtr callback);
+    /// \param callback Callback object. See
+    ///        \c ResolverInterface::Callback for more information
+    void resolve(const isc::dns::QuestionPtr& question,
+                 const isc::resolve::ResolverInterface::CallbackPtr callback);
 
 
     /// \brief Initiates resolving for the given question.
@@ -590,13 +588,13 @@ public:
     /// object.
     ///
     /// \param question The question being answered <qname/qclass/qtype>
-    /// \param buffer An output Message into which the final response will be copied
+    /// \param answer_message An output Message into which the final response will be copied
     /// \param buffer An output buffer into which the intermediate responses will be copied
     /// \param server A pointer to the \c DNSServer object handling the client
-    void sendQuery(const isc::dns::Question& question,
-                   isc::dns::MessagePtr answer_message,
-                   isc::dns::OutputBufferPtr buffer,
-                   DNSServer* server);
+    void resolve(const isc::dns::Question& question,
+                 isc::dns::MessagePtr answer_message,
+                 isc::dns::OutputBufferPtr buffer,
+                 DNSServer* server);
 private:
     DNSService& dns_service_;
     boost::shared_ptr<std::vector<std::pair<std::string, uint16_t> > >

src/lib/asiolink/tests/asiolink_unittest.cc

@@ -678,7 +678,7 @@ TEST_F(ASIOLinkTest, forwarderSend) {
     Question q(Name("example.com"), RRClass::IN(), RRType::TXT());
     OutputBufferPtr buffer(new OutputBuffer(0));
     MessagePtr answer(new Message(Message::RENDER));
-    rq.sendQuery(q, answer, buffer, &server);
+    rq.resolve(q, answer, buffer, &server);
 
     char data[4096];
     size_t size = sizeof(data);
@@ -726,7 +726,7 @@ TEST_F(ASIOLinkTest, recursiveTimeout) {
     Question question(Name("example.net"), RRClass::IN(), RRType::A());
     OutputBufferPtr buffer(new OutputBuffer(0));
     MessagePtr answer(new Message(Message::RENDER));
-    query.sendQuery(question, answer, buffer, &server);
+    query.resolve(question, answer, buffer, &server);
 
     // Run the test
     io_service_->run();
@@ -773,7 +773,7 @@ TEST_F(ASIOLinkTest, DISABLED_recursiveSendOk) {
     Question q(Name("www.isc.org"), RRClass::IN(), RRType::A());
     OutputBufferPtr buffer(new OutputBuffer(0));
     MessagePtr answer(new Message(Message::RENDER));
-    rq.sendQuery(q, answer, buffer, &server);
+    rq.resolve(q, answer, buffer, &server);
     io_service_->run();
 
     // Check that the answer we got matches the one we wanted
@@ -798,7 +798,7 @@ TEST_F(ASIOLinkTest, DISABLED_recursiveSendNXDOMAIN) {
     Question q(Name("wwwdoesnotexist.isc.org"), RRClass::IN(), RRType::A());
     OutputBufferPtr buffer(new OutputBuffer(0));
     MessagePtr answer(new Message(Message::RENDER));
-    rq.sendQuery(q, answer, buffer, &server);
+    rq.resolve(q, answer, buffer, &server);
     io_service_->run();
 
     // Check that the answer we got matches the one we wanted

src/lib/nsas/nameserver_entry.h

@@ -85,7 +85,6 @@ public:
 };
 
 class ZoneEntry;
-//class ResolverInterface;
 
 /// \brief Nameserver Entry
 ///

src/lib/resolve/tests/Makefile.am

@@ -14,6 +14,7 @@ TESTS += run_unittests
 run_unittests_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES)
 run_unittests_LDFLAGS = $(AM_LDFLAGS) $(GTEST_LDFLAGS)
 run_unittests_SOURCES = run_unittests.cc
+run_unittests_SOURCES += resolver_callback_unittest.cc
 
 run_unittests_LDADD = $(GTEST_LDADD)
 run_unittests_LDADD +=  $(top_builddir)/src/lib/resolve/libresolve.la

src/lib/resolve/tests/resolver_callback_unittest.cc

@@ -0,0 +1,90 @@
+// Copyright (C) 2010  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#include <gtest/gtest.h>
+#include <resolve/resolver_callback.h>
+#include <asiolink/asiolink.h>
+
+using namespace isc::resolve;
+
+// Dummy subclass for DNSServer*
+// We want to check if resume is called
+// Since the server will get cloned(), we want the clones to share
+// our bools for whether resume got called and with what value
+class DummyServer : public asiolink::DNSServer {
+public:
+    DummyServer(DummyServer* orig) {
+        resume_called_ = orig->getResumeCalled();
+        resume_value_ = orig->getResumeValue();
+    }
+    DummyServer(bool* resume_called, bool* resume_value) :
+        resume_called_(resume_called), resume_value_(resume_value)
+    {}
+    
+    bool* getResumeCalled() { return resume_called_; }
+    bool* getResumeValue() { return resume_value_; }
+    
+    DNSServer* clone() {
+        DummyServer* n = new DummyServer(this);
+        return n;
+    }
+
+    void resume(bool value) {
+        *resume_called_ = true;
+        *resume_value_ = value;
+    }
+
+private:
+    bool* resume_called_;
+    bool* resume_value_;
+};
+
+class ResolverCallbackServerTest : public ::testing::Test {
+public:
+    ResolverCallbackServerTest() : resume_called_(false),
+                                   resume_value_(false) {
+        server_ = new DummyServer(&resume_called_, &resume_value_);
+        callback_ = new ResolverCallbackServer(server_);
+    };
+
+    ~ResolverCallbackServerTest() {
+        delete callback_;
+        delete server_;
+    }
+
+    DummyServer* getServer() { return server_; }
+    ResolverCallbackServer* getCallback() { return callback_; }
+    bool getResumeCalled() { return resume_called_; }
+    bool getResumeValue() { return resume_value_; }
+
+private:
+    DummyServer* server_;
+    ResolverCallbackServer* callback_;
+    bool resume_called_;
+    bool resume_value_;
+};
+
+TEST_F(ResolverCallbackServerTest, testSuccess) {
+    EXPECT_FALSE(getResumeCalled());
+    getCallback()->success(isc::dns::MessagePtr());
+    EXPECT_TRUE(getResumeCalled());
+    EXPECT_TRUE(getResumeValue());
+}
+
+TEST_F(ResolverCallbackServerTest, testFailure) {
+    EXPECT_FALSE(getResumeCalled());
+    getCallback()->failure();
+    EXPECT_TRUE(getResumeCalled());
+    EXPECT_FALSE(getResumeValue());
+}