BIND 10 #3359: DB backends: Share unit-tests between databases (memfile, mysql, postgres)

[BIND 10 Development]

#3359: DB backends: Share unit-tests between databases (memfile, mysql, postgres)
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcpdb        |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea0.9-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  8             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  2
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => marcin


Comment:

 Replying to [comment:5 marcin]:
 > Reviewed 032ae89cd0ca9d04703555611aece3d3b791565f
 >
 > I removed a couple of trivial errors from the code. Please pull the
 latest commit from the branch.
 Thanks a lot.

 > Just a couple of remaining nits:
 >
 > '''src/lib/dhcpsrv/tests/lease_mgr_unittest.cc'''
 > The reopen function in the abstract class is virtual, so I made an
 occurence in this file virtual too. It is not a must, but looks cleaner.
 Thanks.

 > '''src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc'''
 > addGetDelete6: It would be useful if there was a comment before an
 invocation of:
 > {{{
 > testAddGetDelete6(true);
 > }}}
 > to explain what the !''true!'' value is?
 Added comment.

 > What is (2) in getLease4ClientIdSize?
 I have no idea, I just copied it over :) I *vaguely* recall that I used to
 number various getLease4() functions, but that quickly became useless as
 we now have 6 different getLease(s)4() methods. So removed (2).

 > It should be explained why this test: DISABLED_getLeases6DuidIaid is
 disabled. Same refers to DISABLED_updateLease and DISABLED_updateLease6.
 Good point. For every DISABLED test there's a @todo that describes what
 has to be completed before the test can be reenabled. Most cases are down
 to a single getLease6() method being not implemented. Two tests are about
 memfile not checking lengths of a field and not throwing when it is
 exceeded. See comment below.

 > Question regarding not-applicable test. Do you think it would be
 reasonable to apply some constraints on the length of the variable length
 data fields in the memfile data structures?
 Ultimately yes, but I would consider it a low priority for now. We already
 have checks for hostname coming over wire. It may be a nice improvement
 for beta or 0.9 golden.

 > This test getLease6DuidIaidSubnetIdSize lacks description.
 Added.

 > '''src/lib/dhcpsrv/tests/test_utils.cc'''
 > I removed brief descriptions from the tests in the .cc file, because
 they already have the documentation in the header file. It would be hard
 to keep both descriptions consistent so there should be just one
 description in the header.
 Thanks.

 > '''src/lib/dhcpsrv/tests/test_utils.h'''
 > I don't really like the fact that we put generic test fixture classes
 into files named !''test_utils.h!''. First, the utility nature of these
 files suggests that it holds convenience functions, like convert one type
 to the other etc. The !''!GenericLeaseMgrTest!'' is a test fixture class
 implementation and it contains actual test cases. I think it deserves
 being in a separate file. It is hard to find this class without greping
 the code, if it is in the test_utils.h.
 Moved to new files: generic_lease_mgr_unittest.{cc|h}

 > It would be more readable if brief comments for the member variables
 weren't inline, but each description preceded the member declaration.
 Cleaned up.

 Thanks a lot for your quick review.

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