Opened 6 years ago

Closed 6 years ago

#3359 closed enhancement (complete)

DB backends: Share unit-tests between databases (memfile, mysql, postgres)

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Kea0.8
Component: database-all Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 1.5
Total Hours: 9.5 Internal?: no

Description

Currently we have many MySQL specific unit-tests, we have some memfile specific tests. There are also limited tests for Postgres database patch.

Since the interface is common for all backends, majority of the tests should be reusable between backends.

Subtickets

Change History (8)

comment:1 Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 0 to 6
  • Owner set to UnAssigned
  • Status changed from new to reviewing
  • Total Hours changed from 0 to 6

Memfile and MySQL specific tests were moved to common base LeaseMgr? test class.

Several tests were added to both Memfile and MySQL. Some tests are failing for memfile. They were disabled, because there are some missing functionalities. There should be a separate ticket for that.

comment:2 Changed 6 years ago by tomek

Ready for review.

comment:3 Changed 6 years ago by tomek

  • Milestone changed from DHCP-Kea-proposed to DHCP-Kea0.9-alpha

Moving tasks related to additional backends to Kea0.9-alpha (as discussed with Marcin, Stephen, Vicky and Jeff in London).

comment:4 Changed 6 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:5 follow-up: Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 6 to 2
  • Owner changed from marcin to tomek
  • Total Hours changed from 6 to 8

Reviewed 032ae89cd0ca9d04703555611aece3d3b791565f

I removed a couple of trivial errors from the code. Please pull the latest commit from the branch.

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.

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?

What is (2) in getLease4ClientIdSize?

It should be explained why this test: DISABLED_getLeases6DuidIaid is disabled. Same refers to DISABLED_updateLease and DISABLED_updateLease6.

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?

This test getLease6DuidIaidSubnetIdSize lacks description.

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.

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.

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

comment:6 in reply to: ↑ 5 Changed 6 years ago by tomek

  • Owner changed from tomek to marcin

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

comment:7 Changed 6 years ago by marcin

  • Owner changed from marcin to tomek

Reviewed 516b7c2ffbd3a0c50eb21edd7210e67846882257

Changes are ok. Did you intend to put a ChangeLog entry for that?

comment:8 Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 2 to 1.5
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 8 to 9.5

Code merged and pushed. Closing ticket.

(Adding 1 extra hour I forgot to add during last time).

Note: See TracTickets for help on using tickets.