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

[BIND 10 Development]

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

 * owner:  tmark => marcin


Comment:

 Replying to [comment:12 marcin]:
 > > > '''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!''.

 Fixed.

 >
 > New doxygen description has misspellings.

 Fixed.

 >
 > > > '''src/bin/d2/nc_trans.h'''
 >
 > sendUpdate: there is no description of the parameter passed to this
 function.

 Got it.
 >
 > getIOService: There is no documentation for the getIOService function.
 >

 Fixed.

 > > >
 > > > '''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.
 >

 I am deferring this to 3241.  That ticket refactors this code a good bit.

 > > >
 > > > 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++) {
 > ...
 > }
 > }}}
 >
 Sigh. Done.

 > 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.
 >

 Given the possibility that curr_server might be an empty pointer, I
 modified
 the tests to output the number of servers and number of selections at the
 point of failure:

 {{{
         ASSERT_NO_THROW(name_add->selectingRevServerHandler())
               << " num_servers: " << num_servers << " selections: " << i;
 }}}

 I also changed some EXPECTs to ASSERTs.

 >
 > > > '''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.

 Thanks.

 >
 > 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?
 >

 Yes and yes.


 > 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.

 I'm not sure I see the point. We really only care about prior to the
 threshold and crossing it. In between is well just loops.

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

 Got it.

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

 They don't need it. It was helpful while debugging the sendUpdate tests to
 see the logger output and I left them in.  I have removed them.

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