From: Thomas Markwalder Date: Tue, 3 Sep 2013 17:41:18 +0000 (-0400) Subject: [3086] Addressed review comments X-Git-Tag: bind10-1.2.0beta1-release~227^2~1 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=92adc1fb42e8966816f96b4e6521bd2a18cead1f;p=thirdparty%2Fkea.git [3086] Addressed review comments Added virtual method for validating state handler map, added additional unit tests and commentary. --- diff --git a/src/bin/d2/d2_update_mgr.cc b/src/bin/d2/d2_update_mgr.cc index c49805029f..b88b415bbe 100644 --- a/src/bin/d2/d2_update_mgr.cc +++ b/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; } } diff --git a/src/bin/d2/nc_trans.cc b/src/bin/d2/nc_trans.cc index c8f0ec872a..ae106ef651 100644 --- a/src/bin/d2/nc_trans.cc +++ b/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,17 +79,15 @@ 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 @@ -165,11 +162,6 @@ NameChangeTransaction::setDnsUpdateStatus(const DNSClient::Status& status) { dns_update_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, diff --git a/src/bin/d2/nc_trans.h b/src/bin/d2/nc_trans.h index 7cbf947e0d..f71bc7f296 100644 --- a/src/bin/d2/nc_trans.h +++ b/src/bin/d2/nc_trans.h @@ -57,7 +57,7 @@ typedef std::map 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. diff --git a/src/bin/d2/tests/nc_trans_unittests.cc b/src/bin/d2/tests/nc_trans_unittests.cc index 9f524a41a9..777c19f462 100644 --- a/src/bin/d2/tests/nc_trans_unittests.cc +++ b/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 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); }