BIND 10 #3087: Develop NameAddTransaction class for b10-dhcp-ddns

[BIND 10 Development]

#3087: Develop NameAddTransaction class for b10-dhcp-ddns
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp-ddns     |                    Milestone:
            Keywords:  Task# 4.2     |  Sprint-DHCP-20131204
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tmark


Comment:

 > > '''src/bin/d2/nc_add.cc'''
 > > addingFwdAddrsHandler: The IO_COMPLETED_EVT event is handled by this
 function (in fact, not only by this function) but this event is not
 emitted from within the class. It would be useful to have a reference to
 the place in the code where event is triggered. Some little comment which
 refers to NameChangeTransaction::operator() would do the trick.
 > >
 >
 > Commentary added, and then repeated for the forward and reverse replace
 handlers.
 >
 > > '''src/bin/d2/nc_add.h'''
 > > readyHandler: Typo !''START_EVT.EADY_ST!''.

 New doxygen description has misspellings.

 > > '''src/bin/d2/nc_trans.h'''

 sendUpdate: there is no description of the parameter passed to this
 function.

 getIOService: There is no documentation for the getIOService function.

 > >
 > > '''src/bin/d2/tests/nc_add_unittests.cc'''
 > >
 > > The commentary for functions inside of !NameAddStub could be extended.
 At least mention what fakeResponse, selectFwdServer and selectRevServer is
 doing. Especially, the fakeResponse deserves the commentary.

 I may have overlooked something but there is still no commentary for
 fakeResponse, selectFwdServer and selectRevServer.

 > >
 > > General comments to all the test: it is probably the only test suite
 where we use underscores to name tests. Is it really neccesary?
 > >
 >
 > I compromised and removed all but one underscore on the handler tests.
 The
 > tests are all named <hander>_<scenario>.  It helps me keep track this
 way.

 ok

 In the new loops you could use ++i instead of i++:
 {{{
 for (int i = 0; i < num_servers; i++) {
 ...
 }
 }}}

 Suggestion: for the tests containing a loop with multiple servers, it is
 convenient from the debugging standpoint if the error message contains the
 server address. So for example:
 {{{
     for (int i = 0; i < num_servers; i++) {
         DnsServerInfoPtr curr_server = name_add->getCurrentServer();
         // Run selectingRevServerHandler.
         EXPECT_NO_THROW(name_add->selectingRevServerHandler())
             << "failed to select server " <<
 curr_server.getIpAddress().toText();
 ...

 }
 }}}
 It is not a must for this ticket. Just something for consideration.


 > > '''src/bin/d2/tests/nc_trans_unittests.cc'''
 > > Neither sendUpdate, retryTransition nor setUpdateAttempts are unit
 tested. I understand that it may be more involved to test sendUpdate so
 perhaps the reference should be added to unit tests that actually test its
 behaviour (in nc_add_unittests?) to say that it is unit tested there. In
 any case it would be the most appropriate to test sendUpdate in this file.
 > >
 > >
 >
 > There are unit tests for all of these now.  The sendUpdate unit tests
 required
 > a fair amount of effort but it is a good thing you commented on it.  It
 > uncovered a few things that I have fixed.

 I like the !FauxServer class. It is an elegant way make a distinction
 between the server and a client in the tests.

 retryTransition: Instead of this...
 {{{
     EXPECT_TRUE(name_change->getUpdateAttempts() <
                 NameChangeTransaction::MAX_UPDATE_TRIES_PER_SERVER);

 }}}

 you could use

 {{{
     EXPECT_LT(name_change->getUpdateAttempts(),
               NameChangeTransaction::MAX_UPDATE_TRIES_PER_SERVER);

 }}}
 Also, shouldn't it be ASSERT instead?

 I wonder if this test could use a loop to do as many retries as possible
 (instead of doing one retryand calling setUpdateAttempts). Just a thought.

 !NameChangeTransactionTest.sendUpdateCorruptResponse: Typo in the
 description !''respons!'', instead of !''response!''.

 Why do you need to call:
 {{{
 isc::log::initLogger
 }}}
 ?
 There are multiple tests that do that.

-- 
Ticket URL: <http://bind10.isc.org/ticket/3087#comment:12>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development