[3086] Addressed review comments

Added virtual method for validating state handler map, added additional unit tests and commentary.

src/bin/d2/d2_update_mgr.cc

@@ -77,7 +77,7 @@ D2UpdateMgr::checkFinishedTransactions() {
     // NOTE: One must use postfix increments of the iterator on the calls
     // to erase.  This replaces the old iterator which becomes invalid by the
     // erase with a the next valid iterator.  Prefix incrementing will not
-    // work. 
+    // work.
     TransactionList::iterator it = transaction_list_.begin();
     while (it != transaction_list_.end()) {
         NameChangeTransactionPtr trans = (*it).second;
@@ -108,12 +108,6 @@ void D2UpdateMgr::pickNextJob() {
         if (!hasTransaction(found_ncr->getDhcid())) {
             queue_mgr_->dequeueAt(index);
             makeTransaction(found_ncr);
-
-#if 0
-            // this will run it up to its first IO
-            trans->startTransaction();
-#endif
-
             return;
         }
     }

src/bin/d2/nc_trans.cc

@@ -37,7 +37,6 @@ const int NameChangeTransaction::NO_MORE_SERVERS_EVT;
 const int NameChangeTransaction::IO_COMPLETED_EVT;
 const int NameChangeTransaction::UPDATE_OK_EVT;
 const int NameChangeTransaction::UPDATE_FAILED_EVT;
-const int NameChangeTransaction::CANCEL_TRANSACTION_EVT;
 const int NameChangeTransaction::ALL_DONE_EVT;
 
 const int NameChangeTransaction::DERIVED_EVENTS;
@@ -80,18 +79,16 @@ NameChangeTransaction::startTransaction() {
     // Initialize the state handler map first.
     initStateHandlerMap();
 
+    // Test validity of the handler map. This provides an opportunity to
+    // sanity check the map prior to attempting to execute the model.
+    verifyStateHandlerMap();
+
     // Set the current state to READY and enter the run loop.
     setState(READY_ST);
     runStateModel(START_TRANSACTION_EVT);
 }
 
 void
-NameChangeTransaction::cancelTransaction() {
-    //@todo It is up to the deriving state model to handle this event.
-    runStateModel(CANCEL_TRANSACTION_EVT);
-}
-
-void
 NameChangeTransaction::operator()(DNSClient::Status status) {
     // Stow the completion status and re-enter the run loop with the event
     // set to indicate IO completed.
@@ -166,11 +163,6 @@ NameChangeTransaction::setDnsUpdateStatus(const DNSClient::Status& status) {
 }
 
 void
-NameChangeTransaction::setDnsUpdateResponse(D2UpdateMessagePtr& response) {
-    dns_update_response_ = response;
-}
-
-void
 NameChangeTransaction::setForwardChangeCompleted(const bool value) {
     forward_change_completed_ = value;
 }
@@ -206,7 +198,11 @@ NameChangeTransaction::getReverseDomain() {
 }
 
 void
-NameChangeTransaction::initServerSelection(DdnsDomainPtr& domain) {
+NameChangeTransaction::initServerSelection(const DdnsDomainPtr& domain) {
+    if (!domain) {
+        isc_throw(NameChangeTransactionError,
+                  "initServerSelection called with an empty domain");
+    }
     current_server_list_ = domain->getServers();
     next_server_pos_ = 0;
     current_server_.reset();
@@ -219,8 +215,8 @@ NameChangeTransaction::selectNextServer() {
         current_server_  = (*current_server_list_)[next_server_pos_];
         dns_update_response_.reset(new
                                    D2UpdateMessage(D2UpdateMessage::INBOUND));
-        // @todo  Prototype is set on DNSClient constructor.  We need
-        // to progate a configruation value downward, probably starting
+        // @todo  Protocol is set on DNSClient constructor.  We need
+        // to propagate a configuration value downward, probably starting
         // at global, then domain, then server
         // Once that is supported we need to add it here.
         dns_client_.reset(new DNSClient(dns_update_response_ , this,

src/bin/d2/nc_trans.h

@@ -57,7 +57,7 @@ typedef std::map<unsigned int, StateHandler> StateHandlerMap;
 /// of DNS server(s) needed. It is responsible for knowing what conversations
 /// it must have with which servers and in the order necessary to fulfill the
 /// request. Upon fulfillment of the request, the transaction's work is complete
-/// it is destroyed.
+/// and it is destroyed.
 ///
 /// Fulfillment of the request is carried out through the performance of the
 /// transaction's state model.  Using a state driven implementation accounts
@@ -121,10 +121,22 @@ public:
     static const int NEW_ST = 0;
     /// @brief State from which a transaction is started.
     static const int READY_ST = 1;
-    /// @brief State in which forward DNS server  selection is done.
+    /// @brief State in which forward DNS server selection is done.
+    ///
+    /// Within this state, the actual selection of the next forward server
+    /// to use is conducted.  Upon conclusion of this state the next server
+    /// is either selected or it should transition out with NO_MORE_SERVERS_EVT
+    /// event.
     static const int SELECTING_FWD_SERVER_ST = 2;
+
     /// @brief State in which reverse DNS server  selection is done.
+    ///
+    /// Within this state, the actual selection of the next reverse server
+    /// to use is conducted.  Upon conclusion of this state the next server
+    /// is either selected or it should transition out with NO_MORE_SERVERS_EVT
+    /// event.
     static const int SELECTING_REV_SERVER_ST = 3;
+
     /// @brief Final state, all work has been performed.
     static const int DONE_ST = 4;
 
@@ -163,11 +175,8 @@ public:
     /// the failure is given by the DNSClient return status and the response
     /// packet (if one was received).
     static const int UPDATE_FAILED_EVT = 8;
-    /// @brief Issued when the transaction should be cancelled.
-    /// @todo - still on the fence about this one.
-    static const int CANCEL_TRANSACTION_EVT = 9;
     /// @brief Issued when the state model has no more work left to do.
-    static const int ALL_DONE_EVT = 10;
+    static const int ALL_DONE_EVT = 9;
 
     /// @define Value at which custom events in a derived class should begin.
     static const int DERIVED_EVENTS = 100;
@@ -182,7 +191,7 @@ public:
     /// @param forward_domain is the domain to use for forward DNS updates
     /// @param reverse_domain is the domain to use for reverse DNS updates
     ///
-    /// @throw NameChangeTransaction error if given an null request,
+    /// @throw NameChangeTransactionError if given an null request,
     /// if forward change is enabled but forward domain is null, if
     /// reverse change is enabled but reverse domain is null.
     NameChangeTransaction(isc::asiolink::IOService& io_service,
@@ -190,6 +199,7 @@ public:
                           DdnsDomainPtr& forward_domain,
                           DdnsDomainPtr& reverse_domain);
 
+    /// @brief Destructor
     virtual ~NameChangeTransaction();
 
     /// @brief Begins execution of the transaction.
@@ -200,9 +210,6 @@ public:
     /// parameter of START_TRANSACTION_EVT.
     void startTransaction();
 
-    /// @todo - Not sure about this yet.
-    void cancelTransaction();
-
     /// @brief Serves as the DNSClient IO completion event handler.
     ///
     /// This is the implementation of the method inherited by our derivation
@@ -225,11 +232,39 @@ protected:
     /// Implementations should use the addToMap() method add entries to
     /// the map.
     /// @todo This method should be pure virtual but until there are
-    /// derivations for the update manager to use we will provide an
+    /// derivations for the update manager to use, we will provide a
     /// temporary empty, implementation.  If we make it pure virtual now
     /// D2UpdateManager will not compile.
     virtual void initStateHandlerMap() {};
 
+
+    /// @brief Validates the contents of the state handler map.
+    ///
+    /// This method is invoked immediately after initStateHandlerMap and
+    /// provides an opportunity for derivations to verify that the map
+    /// is correct.  If the map is determined to be invalid this method
+    /// should throw a NameChangeTransactionError.
+    ///
+    /// The simplest implementation would include a call to getStateHandler,
+    /// for each state the derivation supports.  For example, a implementation
+    /// which included three states, READY_ST, DO_WORK_ST, and DONE_ST could
+    /// implement this function as follows:
+    ///
+    /// @code
+    ///    void verifyStateHandlerMap() {
+    ///        getStateHandler(READY_ST);
+    ///        getStateHandler(DO_WORK_ST);
+    ///        getStateHandler(DONE_ST);
+    ///    }
+    /// @endcode
+    ///
+    /// @todo This method should be pure virtual but until there are
+    /// derivations for the update manager to use, we will provide a
+    /// temporary empty, implementation.  If we make it pure virtual now
+    /// D2UpdateManager will not compile.
+    /// @throw NameChangeTransactionError if the map is invalid.
+    virtual void verifyStateHandlerMap() {};
+
     /// @brief Adds an entry to the state handler map.
     ///
     /// This method attempts to add an entry to the handler map which maps
@@ -257,7 +292,7 @@ protected:
     ///
     /// @throw NameChangeTransactionError if the map already contains an entry
     /// for the given state.
-    void addToMap(unsigned int idx, StateHandler handler);
+    void addToMap(unsigned int state, StateHandler handler);
 
     /// @brief Processes events through the state model
     ///
@@ -345,7 +380,7 @@ protected:
     ///
     /// @param domain is the domain from which server selection is to be
     /// conducted.
-    void initServerSelection(DdnsDomainPtr& domain);
+    void initServerSelection(const DdnsDomainPtr& domain);
 
     /// @brief Selects the next server in the current server list.
     ///
@@ -387,7 +422,7 @@ public:
     ///
     /// This is the current status of the NameChangeRequest, not to
     /// be confused with the state of the transaction.  Once the transaction
-    /// is reached it's conclusion, the request will end up with a final
+    /// is reached its conclusion, the request will end up with a final
     /// status.
     ///
     /// @return A dhcp_ddns::NameChangeStatus representing the current
@@ -396,16 +431,14 @@ public:
 
     /// @brief Fetches the forward DdnsDomain.
     ///
-    /// This value is only meaningful if the request calls for a forward change.
-    ///
-    /// @return A pointer reference to the forward DdnsDomain
+    /// @return A pointer reference to the forward DdnsDomain.  If the
+    /// the request does not include a forward change, the pointer will empty.
     DdnsDomainPtr& getForwardDomain();
 
     /// @brief Fetches the reverse DdnsDomain.
     ///
-    /// This value is only meaningful if the request calls for a reverse change.
-    ///
-    /// @return A pointer reference to the reverse DdnsDomain
+    /// @return A pointer reference to the reverse DdnsDomain.  If the
+    /// the request does not include a reverse change, the pointer will empty.
     DdnsDomainPtr& getReverseDomain();
 
     /// @brief Fetches the transaction's current state.

src/bin/d2/tests/nc_trans_unittests.cc

@@ -27,7 +27,7 @@ namespace {
 /// @brief Test derivation of NameChangeTransaction for exercising state
 /// model mechanics.
 ///
-/// This class faciliates testing by making non-public methods accessible so
+/// This class facilitates testing by making non-public methods accessible so
 /// they can be invoked directly in test routines.  It implements a very
 /// rudimentary state model, sufficient to test the state model mechanics
 /// supplied by the base class.
@@ -35,7 +35,10 @@ class NameChangeStub : public NameChangeTransaction {
 public:
 
     // NameChangeStub states
-    static const int DO_WORK_ST = DERIVED_STATES + 1;
+    static const int DUMMY_ST = DERIVED_STATES + 1;
+
+    static const int DO_WORK_ST = DERIVED_STATES + 2;
+
 
     // NameChangeStub events
     static const int START_WORK_EVT = DERIVED_EVENTS + 1;
@@ -48,17 +51,29 @@ public:
                    DdnsDomainPtr forward_domain,
                    DdnsDomainPtr reverse_domain)
         : NameChangeTransaction(io_service, ncr, forward_domain,
-                                reverse_domain) {
+                                reverse_domain), dummy_called_(false) {
     }
 
     /// @brief Destructor
     virtual ~NameChangeStub() {
     }
 
+    bool getDummyCalled() {
+        return (dummy_called_);
+    }
+
+    void clearDummyCalled() {
+        dummy_called_ = false;
+    }
+
+    void dummyHandler() {
+       dummy_called_ = true;
+    }
+
     /// @brief State handler for the READY_ST.
     ///
     /// Serves as the starting state handler, it consumes the
-    /// START_TRANSACTION_EVT "transitioing" to the state, DO_WORK_ST and
+    /// START_TRANSACTION_EVT "transitioning" to the state, DO_WORK_ST and
     /// sets the next event to START_WORK_EVT.
     void readyHandler() {
         switch(getNextEvent()) {
@@ -76,7 +91,7 @@ public:
     /// @brief State handler for the DO_WORK_ST.
     ///
     /// Simulates a state that starts some form of asynchronous work.
-    /// When next event is START_WROK_EVT it sets the status to pending
+    /// When next event is START_WORK_EVT it sets the status to pending
     /// and signals the state model must "wait" for an event by setting
     /// next event to NOP_EVT.
     ///
@@ -122,13 +137,19 @@ public:
         addToMap(READY_ST,
             boost::bind(&NameChangeStub::readyHandler, this));
 
-         addToMap(DO_WORK_ST,
+        addToMap(DO_WORK_ST,
             boost::bind(&NameChangeStub::doWorkHandler, this));
 
-         addToMap(DONE_ST,
+        addToMap(DONE_ST,
             boost::bind(&NameChangeStub::doneHandler, this));
     }
 
+    void verifyStateHandlerMap() {
+        getStateHandler(READY_ST);
+        getStateHandler(DO_WORK_ST);
+        getStateHandler(DONE_ST);
+    }
+
     // Expose the protected methods to be tested.
     using NameChangeTransaction::addToMap;
     using NameChangeTransaction::getStateHandler;
@@ -140,6 +161,15 @@ public:
     using NameChangeTransaction::selectNextServer;
     using NameChangeTransaction::getCurrentServer;
     using NameChangeTransaction::getDNSClient;
+    using NameChangeTransaction::setNcrStatus;
+    using NameChangeTransaction::setDnsUpdateStatus;
+    using NameChangeTransaction::getDnsUpdateResponse;
+    using NameChangeTransaction::getForwardChangeCompleted;
+    using NameChangeTransaction::getReverseChangeCompleted;
+    using NameChangeTransaction::setForwardChangeCompleted;
+    using NameChangeTransaction::setReverseChangeCompleted;
+
+    bool dummy_called_;
 };
 
 const int NameChangeStub::DO_WORK_ST;
@@ -155,6 +185,8 @@ typedef boost::shared_ptr<NameChangeStub> NameChangeStubPtr;
 class NameChangeTransactionTest : public ::testing::Test {
 public:
     isc::asiolink::IOService io_service_;
+    DdnsDomainPtr forward_domain_;
+    DdnsDomainPtr reverse_domain_;
 
     virtual ~NameChangeTransactionTest() {
     }
@@ -167,7 +199,7 @@ public:
             " \"change_type\" : 0 , "
             " \"forward_change\" : true , "
             " \"reverse_change\" : true , "
-            " \"fqdn\" : \"walah.walah.org.\" , "
+            " \"fqdn\" : \"example.com.\" , "
             " \"ip_address\" : \"192.168.2.1\" , "
             " \"dhcid\" : \"0102030405060708\" , "
             " \"lease_expires_on\" : \"20130121132405\" , "
@@ -179,25 +211,22 @@ public:
         DnsServerInfoStoragePtr servers(new DnsServerInfoStorage());
         DnsServerInfoPtr server;
 
-        DdnsDomainPtr forward_domain;
-        DdnsDomainPtr reverse_domain;
-
         ncr = dhcp_ddns::NameChangeRequest::fromJSON(msg_str);
 
         // make forward server list
-        server.reset(new DnsServerInfo("forward.server.org",
+        server.reset(new DnsServerInfo("forward.example.com",
                                        isc::asiolink::IOAddress("1.1.1.1")));
         servers->push_back(server);
-        forward_domain.reset(new DdnsDomain("*", "", servers));
+        forward_domain_.reset(new DdnsDomain("*", "", servers));
 
         // make reverse server list
         servers->clear();
-        server.reset(new DnsServerInfo("reverse.server.org",
+        server.reset(new DnsServerInfo("reverse.example.com",
                                        isc::asiolink::IOAddress("2.2.2.2")));
         servers->push_back(server);
-        reverse_domain.reset(new DdnsDomain("*", "", servers));
+        reverse_domain_.reset(new DdnsDomain("*", "", servers));
         return (NameChangeStubPtr(new NameChangeStub(io_service_, ncr,
-                                  forward_domain, reverse_domain)));
+                                  forward_domain_, reverse_domain_)));
 
     }
 
@@ -219,7 +248,7 @@ TEST(NameChangeTransaction, construction) {
         " \"change_type\" : 0 , "
         " \"forward_change\" : true , "
         " \"reverse_change\" : true , "
-        " \"fqdn\" : \"walah.walah.org.\" , "
+        " \"fqdn\" : \"example.com.\" , "
         " \"ip_address\" : \"192.168.2.1\" , "
         " \"dhcid\" : \"0102030405060708\" , "
         " \"lease_expires_on\" : \"20130121132405\" , "
@@ -260,87 +289,209 @@ TEST(NameChangeTransaction, construction) {
                                           forward_domain, reverse_domain));
 
     // Verify that an empty forward domain is allowed when the requests does
-    // include a forward change.
+    // not include a forward change.
     ncr->setForwardChange(false);
+    ncr->setReverseChange(true);
     EXPECT_NO_THROW(NameChangeTransaction(io_service, ncr,
                                           empty_domain, reverse_domain));
 
     // Verify that an empty reverse domain is allowed when the requests does
-    // include a reverse change.
+    // not include a reverse change.
+    ncr->setForwardChange(true);
     ncr->setReverseChange(false);
     EXPECT_NO_THROW(NameChangeTransaction(io_service, ncr,
-                                          empty_domain, empty_domain));
+                                          forward_domain, empty_domain));
 }
 
-/// @brief Test the basic mechanics of state model execution.
-/// It first verifies basic state handle map fucntionality, and then
-/// runs the NameChangeStub state model through from start to finish.
-TEST_F(NameChangeTransactionTest, stateModelTest) {
+/// @brief General testing of member accessors.
+/// Most if not all of these are also tested as a byproduct off larger tests.
+TEST_F(NameChangeTransactionTest, accessors) {
     NameChangeStubPtr name_change;
     ASSERT_NO_THROW(name_change = makeCannedTransaction());
 
-    // Verify that getStateHandler will throw when, handler map is empty.
+    // Verify that fetching the NameChangeRequest works.
+    dhcp_ddns::NameChangeRequestPtr ncr = name_change->getNcr();
+    ASSERT_TRUE(ncr);
+
+    // Verify that getTransactionKey works.
+    EXPECT_EQ(ncr->getDhcid(), name_change->getTransactionKey());
+
+    // Verify that NcrStatus can be set and retrieved.
+    EXPECT_NO_THROW(name_change->setNcrStatus(dhcp_ddns::ST_FAILED));
+    EXPECT_EQ(dhcp_ddns::ST_FAILED, ncr->getStatus());
+
+    // Verify that the forward domain can be retrieved.
+    ASSERT_TRUE(name_change->getForwardDomain());
+    EXPECT_EQ(forward_domain_, name_change->getForwardDomain());
+
+    // Verify that the reverse domain can be retrieved.
+    ASSERT_TRUE(name_change->getReverseDomain());
+    EXPECT_EQ(reverse_domain_, name_change->getReverseDomain());
+
+    // Neither of these have direct setters, but are tested under server
+    // selection.
+    EXPECT_FALSE(name_change->getDNSClient());
+    EXPECT_FALSE(name_change->getCurrentServer());
+
+    // Previous state should be set by setState.
+    EXPECT_NO_THROW(name_change->setState(NameChangeTransaction::READY_ST));
+    EXPECT_NO_THROW(name_change->setState(NameChangeStub::DO_WORK_ST));
+    EXPECT_EQ(NameChangeTransaction::READY_ST, name_change->getPrevState());
+    EXPECT_EQ(NameChangeStub::DO_WORK_ST, name_change->getState());
+
+    // Last event should be set by setNextEvent.
+    EXPECT_NO_THROW(name_change->setNextEvent(NameChangeStub::
+                                              START_WORK_EVT));
+    EXPECT_NO_THROW(name_change->setNextEvent(NameChangeTransaction::
+                                              IO_COMPLETED_EVT));
+    EXPECT_EQ(NameChangeStub::START_WORK_EVT, name_change->getLastEvent());
+    EXPECT_EQ(NameChangeTransaction::IO_COMPLETED_EVT,
+              name_change->getNextEvent());
+
+    // Verify that DNS update status can be set and retrieved.
+    EXPECT_NO_THROW(name_change->setDnsUpdateStatus(DNSClient::TIMEOUT));
+    EXPECT_EQ(DNSClient::TIMEOUT, name_change->getDnsUpdateStatus());
+
+    // Verify that the DNS update response can be retrieved. 
+    EXPECT_FALSE(name_change->getDnsUpdateResponse());
+
+    // Verify that the forward change complete flag can be set and fetched.
+    EXPECT_NO_THROW(name_change->setForwardChangeCompleted(true));
+    EXPECT_TRUE(name_change->getForwardChangeCompleted());
+
+    // Verify that the reverse change complete flag can be set and fetched.
+    EXPECT_NO_THROW(name_change->setReverseChangeCompleted(true));
+    EXPECT_TRUE(name_change->getReverseChangeCompleted());
+}
+
+/// @brief Tests the fundamental methods used for state handler mapping.
+/// Verifies the ability to search for and add entries in the state handler map.
+TEST_F(NameChangeTransactionTest, basicStateMapping) {
+    NameChangeStubPtr name_change;
+    ASSERT_NO_THROW(name_change = makeCannedTransaction());
+
+    // Verify that getStateHandler will throw when, state cannot be found.
     EXPECT_THROW(name_change->getStateHandler(NameChangeTransaction::READY_ST),
                  NameChangeTransactionError);
 
     // Verify that we can add a handler to the map.
     ASSERT_NO_THROW(name_change->addToMap(NameChangeTransaction::READY_ST,
-                           boost::bind(&NameChangeStub::readyHandler,
-                                       name_change.get())));
+                                          boost::bind(&NameChangeStub::
+                                                      dummyHandler,
+                                                      name_change.get())));
 
     // Verify that we can find the handler by its state.
-    EXPECT_NO_THROW(name_change->getStateHandler(NameChangeTransaction::
+    StateHandler retreived_handler;
+    EXPECT_NO_THROW(retreived_handler =
+                    name_change->getStateHandler(NameChangeTransaction::
                                                  READY_ST));
 
+    // Verify that retrieved handler executes the correct method.
+    name_change->clearDummyCalled();
+
+    ASSERT_NO_THROW((retreived_handler)());
+    EXPECT_TRUE(name_change->getDummyCalled());
+
     // Verify that we cannot add a duplicate.
     EXPECT_THROW(name_change->addToMap(NameChangeTransaction::READY_ST,
-                           boost::bind(&NameChangeStub::readyHandler,
-                                       name_change.get())),
+                                       boost::bind(&NameChangeStub::
+                                                   readyHandler,
+                                                   name_change.get())),
                  NameChangeTransactionError);
 
     // Verify that we can still find the handler by its state.
     EXPECT_NO_THROW(name_change->getStateHandler(NameChangeTransaction::
                                                  READY_ST));
+}
 
-
-    // Get a fresh transaction.
+/// @brief Tests state map initialization and validation.
+/// This tests the basic concept of state map initialization and verification
+/// by manually invoking the map methods normally called by startTransaction.
+TEST_F(NameChangeTransactionTest, stubStateMapInit) {
+    NameChangeStubPtr name_change;
     ASSERT_NO_THROW(name_change = makeCannedTransaction());
 
-    // Manually call checkHandlerMap to ensure our test map populates.
-    // This is the method startTranscation invokes.
+    // Verify that the map validation throws prior to the map being
+    // initialized.
+    ASSERT_THROW(name_change->verifyStateHandlerMap(),
+                 NameChangeTransactionError);
+
+    // Call initStateHandlerMap to initialize the state map.
     ASSERT_NO_THROW(name_change->initStateHandlerMap());
 
-    // Verify that we can find all the handlers by their state.
-    EXPECT_NO_THROW(name_change->getStateHandler(NameChangeTransaction::
-                                                 READY_ST));
-    EXPECT_NO_THROW(name_change->getStateHandler(NameChangeStub::DO_WORK_ST));
-    EXPECT_NO_THROW(name_change->getStateHandler(NameChangeTransaction::
-                                                 DONE_ST));
+    // Verify that the map validation succeeds now that the map is initialized.
+    ASSERT_NO_THROW(name_change->verifyStateHandlerMap());
+}
 
+/// @brief Tests that invalid states are handled gracefully.
+/// This test verifies that attempting to execute a state which has no handler
+/// results in a failed transaction.
+TEST_F(NameChangeTransactionTest, invalidState) {
+    NameChangeStubPtr name_change;
+    ASSERT_NO_THROW(name_change = makeCannedTransaction());
 
-    // Default value for state is NEW_ST.  Attempting to run the model
-    // with an invalid state will result in status of ST_FAILED.
+    // Verfiy that to running the model with a state that has no handler,
+    // will result in failed transaction (status of ST_FAILED).
+    // First, verify state is NEW_ST and that NEW_ST has no handler.
+    // that the transaction failed:
     ASSERT_EQ(NameChangeTransaction::NEW_ST, name_change->getState());
+    ASSERT_THROW(name_change->getStateHandler(NameChangeTransaction::NEW_ST),
+                 NameChangeTransactionError);
+
+    // Now call runStateModel() which should not throw.
     EXPECT_NO_THROW(name_change->runStateModel(NameChangeTransaction::
                                                START_TRANSACTION_EVT));
 
+    // Verify that the transaction has failed.
     EXPECT_EQ(dhcp_ddns::ST_FAILED, name_change->getNcrStatus());
+}
 
-    // Get a fresh transaction.
+/// @brief Tests that invalid events are handled gracefully.
+/// This test verifies that submitting an invalid event to the state machine
+/// results in a failed transaction.
+TEST_F(NameChangeTransactionTest, invalidEvent) {
+    NameChangeStubPtr name_change;
     ASSERT_NO_THROW(name_change = makeCannedTransaction());
 
-    // Launch the transaction properly by calling startTranscation.
-    // Verify that this transitions through to state of DO_WORK_ST,
-    // last event is START_WORK_EVT, next event is NOP_EVT, and
-    // NCR status is ST_PENDING.
+    // First, lets execute the state model to a known valid point, by
+    // calling startTransaction.
     ASSERT_NO_THROW(name_change->startTransaction());
 
+    // Verify we are in the state of DO_WORK_ST.
+    EXPECT_EQ(NameChangeStub::DO_WORK_ST, name_change->getState());
+
+    // Verity that submitting an invalid event to a valid state, results
+    // in a failed transaction without a throw (Current state is DO_WORK_ST,
+    // during which START_TRANSACTION_EVT, is invalid).
+    EXPECT_NO_THROW(name_change->runStateModel(NameChangeTransaction::
+                                               START_TRANSACTION_EVT));
+
+    // Verify that the transaction has failed.
+    EXPECT_EQ(dhcp_ddns::ST_FAILED, name_change->getNcrStatus());
+}
+
+/// @brief Test the basic mechanics of state model execution.
+/// This test exercises the ability to execute state model from state to
+/// finish, including the handling of a asynchronous IO operation.
+TEST_F(NameChangeTransactionTest, stateModelTest) {
+    NameChangeStubPtr name_change;
+    ASSERT_NO_THROW(name_change = makeCannedTransaction());
+
+    // Launch the transaction by calling startTransaction.  The state model
+    // should run up until the "IO" operation is initiated in DO_WORK_ST.
+
+    ASSERT_NO_THROW(name_change->startTransaction());
+
+    // Verify that we are now in state of DO_WORK_ST, the last event was
+    // START_WORK_EVT, the next event is NOP_EVT, and NCR status is ST_PENDING.
     EXPECT_EQ(NameChangeStub::DO_WORK_ST, name_change->getState());
     EXPECT_EQ(NameChangeStub::START_WORK_EVT, name_change->getLastEvent());
     EXPECT_EQ(NameChangeTransaction::NOP_EVT, name_change->getNextEvent());
     EXPECT_EQ(dhcp_ddns::ST_PENDING, name_change->getNcrStatus());
 
-    // Simulate completion of DNSClient exchange by invoking the callback.
+    // Simulate completion of DNSClient exchange by invoking the callback, as
+    // DNSClient would.  This should cause the state model to progress through
+    // completion.
     EXPECT_NO_THROW((*name_change)(DNSClient::SUCCESS));
 
     // Verify that the state model has progressed through to completion:
@@ -351,12 +502,17 @@ TEST_F(NameChangeTransactionTest, stateModelTest) {
     EXPECT_EQ(NameChangeTransaction::NOP_EVT, name_change->getNextEvent());
 }
 
-/// @brief Tests server selection methods
+/// @brief Tests server selection methods.
+/// Each transaction has a list of one or more servers for each DNS direction
+/// it is required to update.  The transaction must be able to start at the
+/// beginning of a server list and cycle through them one at time, as needed,
+/// when a DNS exchange fails due to an IO error.  This test verifies the
+/// ability to iteratively select a server from the list as the current server.
 TEST_F(NameChangeTransactionTest, serverSelectionTest) {
     NameChangeStubPtr name_change;
     ASSERT_NO_THROW(name_change = makeCannedTransaction());
 
-    // Verify that the forward domain and its servers can be retrieved.
+    // Verify that the forward domain and its list of servers can be retrieved.
     DdnsDomainPtr& domain = name_change->getForwardDomain();
     ASSERT_TRUE(domain);
     DnsServerInfoStoragePtr servers = domain->getServers();
@@ -366,72 +522,99 @@ TEST_F(NameChangeTransactionTest, serverSelectionTest) {
     int num_servers = servers->size();
     ASSERT_TRUE(num_servers > 0);
 
+    // Verify that we can initialize server selection.  This "resets" the
+    // selection process to start over using the list of servers in the
+    // given domain.
     ASSERT_NO_THROW(name_change->initServerSelection(domain));
 
+    // The server selection process determines the current server,
+    // instantiates a new DNSClient, and a DNS response message buffer.
+    //  We need to save the values before each selection, so we can verify
+    // they are correct after each selection.
+    DnsServerInfoPtr prev_server = name_change->getCurrentServer();
     DNSClientPtr prev_client = name_change->getDNSClient();
     D2UpdateMessagePtr prev_response = name_change->getDnsUpdateResponse();
-    DnsServerInfoPtr prev_server = name_change->getCurrentServer();
 
     // Iteratively select through the list of servers.
     int passes = 0;
     while (name_change->selectNextServer()) {
+        // Get the new values after the selection has been made.
         DnsServerInfoPtr server = name_change->getCurrentServer();
         DNSClientPtr client = name_change->getDNSClient();
         D2UpdateMessagePtr response = name_change->getDnsUpdateResponse();
+
+        // Verify that the new values are not empty.
         EXPECT_TRUE(server);
         EXPECT_TRUE(client);
         EXPECT_TRUE(response);
 
+        // Verify that the new values are indeed new.
         EXPECT_NE(server, prev_server);
         EXPECT_NE(client, prev_client);
         EXPECT_NE(response, prev_response);
 
+        // Remember the selected values for the next pass.
         prev_server = server;
         prev_client = client;
         prev_response = response;
 
-        passes++;
+        ++passes;
     }
 
-    // Verify that the numer of passes made equal the number of servers.
+    // Verify that the number of passes made equal the number of servers.
     EXPECT_EQ (passes, num_servers);
 
     // Repeat the same test using the reverse domain.
+    // Verify that the reverse domain and its list of servers can be retrieved.
     domain = name_change->getReverseDomain();
     ASSERT_TRUE(domain);
-
     servers = domain->getServers();
     ASSERT_TRUE(servers);
 
+    // Get the number of entries in the server list.
     num_servers = servers->size();
     ASSERT_TRUE(num_servers > 0);
 
+    // Verify that we can initialize server selection.  This "resets" the
+    // selection process to start over using the list of servers in the
+    // given domain.
     ASSERT_NO_THROW(name_change->initServerSelection(domain));
 
+    // The server selection process determines the current server,
+    // instantiates a new DNSClient, and a DNS response message buffer.
+    // We need to save the values before each selection, so we can verify
+    // they are correct after each selection.
+    prev_server = name_change->getCurrentServer();
     prev_client = name_change->getDNSClient();
     prev_response = name_change->getDnsUpdateResponse();
-    prev_server = name_change->getCurrentServer();
 
+    // Iteratively select through the list of servers.
     passes = 0;
     while (name_change->selectNextServer()) {
+        // Get the new values after the selection has been made.
         DnsServerInfoPtr server = name_change->getCurrentServer();
         DNSClientPtr client = name_change->getDNSClient();
         D2UpdateMessagePtr response = name_change->getDnsUpdateResponse();
+
+        // Verify that the new values are not empty.
         EXPECT_TRUE(server);
         EXPECT_TRUE(client);
         EXPECT_TRUE(response);
 
+        // Verify that the new values are indeed new.
         EXPECT_NE(server, prev_server);
         EXPECT_NE(client, prev_client);
         EXPECT_NE(response, prev_response);
 
+        // Remember the selected values for the next pass.
         prev_server = server;
         prev_client = client;
         prev_response = response;
 
-        passes++;
+        ++passes;
     }
 
+    // Verify that the number of passes made equal the number of servers.
     EXPECT_EQ (passes, num_servers);
 }