[3359] Changes after review:

 - GenericLeaseMgrTest class moved to a separate file
 - @todo added for disabled tests in memfile (with explanations)
 - Added several comments

src/lib/dhcpsrv/tests/Makefile.am

@@ -59,6 +59,7 @@ libdhcpsrv_unittests_SOURCES += dbaccess_parser_unittest.cc
 libdhcpsrv_unittests_SOURCES += lease_unittest.cc
 libdhcpsrv_unittests_SOURCES += lease_mgr_factory_unittest.cc
 libdhcpsrv_unittests_SOURCES += lease_mgr_unittest.cc
+libdhcpsrv_unittests_SOURCES += generic_lease_mgr_unittest.cc
 libdhcpsrv_unittests_SOURCES += memfile_lease_mgr_unittest.cc
 libdhcpsrv_unittests_SOURCES += dhcp_parsers_unittest.cc
 if HAVE_MYSQL

src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h

+// Copyright (C) 2014 Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#ifndef GENERIC_LEASE_MGR_UNITTEST_H
+#define GENERIC_LEASE_MGR_UNITTEST_H
+
+#include <dhcpsrv/lease_mgr.h>
+#include <gtest/gtest.h>
+#include <vector>
+
+namespace isc {
+namespace dhcp {
+namespace test {
+
+/// @brief Test Fixture class with utility functions for LeaseMgr backends
+///
+/// It contains utility functions, like dummy lease creation.
+/// All concrete LeaseMgr test classes should be derived from it.
+class GenericLeaseMgrTest : public ::testing::Test {
+public:
+
+    /// @brief Default constructor.
+    GenericLeaseMgrTest();
+
+    /// @brief Virtual destructor.
+    virtual ~GenericLeaseMgrTest();
+
+    /// @brief Reopen the database
+    ///
+    /// Closes the database and re-opens it. It must be implemented
+    /// in derived classes.
+    virtual void reopen() = 0;
+
+    /// @brief Initialize Lease4 Fields
+    ///
+    /// Returns a pointer to a Lease4 structure.  Different values are put into
+    /// the lease according to the address passed.
+    ///
+    /// This is just a convenience function for the test methods.
+    ///
+    /// @param address Address to use for the initialization
+    ///
+    /// @return Lease4Ptr.  This will not point to anything if the
+    ///         initialization failed (e.g. unknown address).
+    Lease4Ptr initializeLease4(std::string address);
+
+    /// @brief Initialize Lease6 Fields
+    ///
+    /// Returns a pointer to a Lease6 structure.  Different values are put into
+    /// the lease according to the address passed.
+    ///
+    /// This is just a convenience function for the test methods.
+    ///
+    /// @param address Address to use for the initialization
+    ///
+    /// @return Lease6Ptr.  This will not point to anything if the initialization
+    ///         failed (e.g. unknown address).
+    Lease6Ptr initializeLease6(std::string address);
+
+    /// @brief Check Leases present and different
+    ///
+    /// Checks a vector of lease pointers and ensures that all the leases
+    /// they point to are present and different.  If not, a GTest assertion
+    /// will fail.
+    ///
+    /// @param leases Vector of pointers to leases
+    /// @tparam Type of the leases held in the vector: @c Lease4 or
+    /// @c Lease6.
+    template <typename T>
+    void checkLeasesDifferent(const std::vector<T>& leases) const;
+
+    /// @brief Creates leases for the test
+    ///
+    /// Creates all leases for the test and checks that they are different.
+    ///
+    /// @return vector<Lease4Ptr> Vector of pointers to leases
+    std::vector<Lease4Ptr> createLeases4();
+
+    /// @brief Creates leases for the test
+    ///
+    /// Creates all leases for the test and checks that they are different.
+    ///
+    /// @return vector<Lease6Ptr> Vector of pointers to leases
+    std::vector<Lease6Ptr> createLeases6();
+
+    /// @brief checks that addLease, getLease4(addr) and deleteLease() works
+    void testBasicLease4();
+
+    /// @brief Test lease retrieval using client id.
+    void testGetLease4ClientId();
+
+    /// @brief Test lease retrieval when leases with NULL client id are present.
+    void testGetLease4NullClientId();
+
+    /// @brief Test lease retrieval using HW address.
+    void testGetLease4HWAddr1();
+
+    /// @brief Check GetLease4 methods - access by Hardware Address
+    ///
+    /// Adds leases to the database and checks that they can be accessed using
+    /// HWAddr information.
+    void testGetLease4HWAddr2();
+
+    /// @brief Test lease retrieval using client id, HW address and subnet id.
+    void testGetLease4ClientIdHWAddrSubnetId();
+
+    // @brief Get lease4 by hardware address (2)
+    //
+    // Check that the system can cope with getting a hardware address of
+    // any size.
+    void testGetLease4HWAddrSize();
+
+    /// @brief Check GetLease4 methods - access by Hardware Address & Subnet ID
+    ///
+    /// Adds leases to the database and checks that they can be accessed via
+    /// a combination of hardware address and subnet ID
+    void testGetLease4HWAddrSubnetId();
+
+    /// @brief Get lease4 by hardware address and subnet ID (2)
+    ///
+    /// Check that the system can cope with getting a hardware address of
+    /// any size.
+    void testGetLease4HWAddrSubnetIdSize();
+
+    /// @brief Check GetLease4 methods - access by Client ID
+    ///
+    /// Adds leases to the database and checks that they can be accessed via
+    /// the Client ID.
+    void testGetLease4ClientId2();
+
+    /// @brief Get Lease4 by client ID (2)
+    ///
+    /// Check that the system can cope with a client ID of any size.
+    void testGetLease4ClientIdSize();
+
+    /// @brief Check GetLease4 methods - access by Client ID & Subnet ID
+    ///
+    /// Adds leases to the database and checks that they can be accessed via
+    /// a combination of client and subnet IDs.
+    void testGetLease4ClientIdSubnetId();
+
+    /// @brief Basic Lease4 Checks
+    ///
+    /// Checks that the addLease, getLease4(by address), getLease4(hwaddr,subnet_id),
+    /// updateLease4() and deleteLease (IPv4 address) can handle NULL client-id.
+    /// (client-id is optional and may not be present)
+    ///
+    /// @todo: check if it does overlap with @ref testGetLease4NullClientId()
+    void testLease4NullClientId();
+
+    /// @brief Basic Lease6 Checks
+    ///
+    /// Checks that the addLease, getLease6 (by address) and deleteLease (with an
+    /// IPv6 address) works.
+    void testBasicLease6();
+
+    /// @brief Test that IPv6 lease can be added, retrieved and deleted.
+    ///
+    /// This method checks basic IPv6 lease operations. There's check_t1_t2
+    /// parameter that controls whether the backend supports storing T1, T2
+    /// parameters. memfile supports it, while MySQL doesn't. If T1,T2
+    /// storage is not supported, the expected values are 0.
+    ///
+    /// @param check_t1_t2 controls whether T1,T2 timers should be checked
+    void testAddGetDelete6(bool check_t1_t2);
+
+    /// @brief Check GetLease6 methods - access by DUID/IAID
+    ///
+    /// Adds leases to the database and checks that they can be accessed via
+    /// a combination of DUID and IAID.
+    void testGetLeases6DuidIaid();
+
+    /// @brief Check that the system can cope with a DUID of allowed size.
+    void testGetLeases6DuidSize();
+
+    /// @brief Check that getLease6 methods discriminate by lease type.
+    ///
+    /// Adds six leases, two per lease type all with the same duid and iad but
+    /// with alternating subnet_ids.
+    /// It then verifies that all of getLeases6() method variants correctly
+    /// discriminate between the leases based on lease type alone.
+    void testLease6LeaseTypeCheck();
+
+    /// @brief Check GetLease6 methods - access by DUID/IAID/SubnetID
+    ///
+    /// Adds leases to the database and checks that they can be accessed via
+    /// a combination of DIUID and IAID.
+    void testGetLease6DuidIaidSubnetId();
+
+    /// @brief Checks that getLease6() works with different DUID sizes
+    void testGetLease6DuidIaidSubnetIdSize();
+
+    /// @brief Verify that too long hostname for Lease4 is not accepted.
+    ///
+    /// Checks that the it is not possible to create a lease when the hostname
+    /// length exceeds 255 characters.
+    void testLease4InvalidHostname();
+
+    /// @brief Verify that too long hostname for Lease6 is not accepted.
+    ///
+    /// Checks that the it is not possible to create a lease when the hostname
+    /// length exceeds 255 characters.
+    void testLease6InvalidHostname();
+
+    /// @brief Lease4 update test
+    ///
+    /// Checks that the code is able to update an IPv4 lease in the database.
+    void testUpdateLease4();
+
+    /// @brief Lease6 update test
+    ///
+    /// Checks that the code is able to update an IPv6 lease in the database.
+    void testUpdateLease6();
+
+    /// @brief String forms of IPv4 addresses
+    std::vector<std::string>  straddress4_;
+
+    /// @brief IOAddress forms of IPv4 addresses
+    std::vector<isc::asiolink::IOAddress> ioaddress4_;
+
+    /// @brief String forms of IPv6 addresses
+    std::vector<std::string>  straddress6_;
+
+    /// @brief Types of IPv6 Leases
+    std::vector<Lease::Type> leasetype6_;
+
+    /// @brief IOAddress forms of IPv6 addresses
+    std::vector<isc::asiolink::IOAddress> ioaddress6_;
+
+    /// @brief Pointer to the lease manager
+    LeaseMgr* lmptr_;
+};
+
+}; // namespace test
+}; // namespace dhcp
+}; // namespace isc
+
+#endif

src/lib/dhcpsrv/tests/lease_mgr_unittest.cc

@@ -18,6 +18,7 @@
 #include <dhcpsrv/lease_mgr.h>
 #include <dhcpsrv/memfile_lease_mgr.h>
 #include <dhcpsrv/tests/test_utils.h>
+#include <dhcpsrv/tests/generic_lease_mgr_unittest.h>
 
 #include <gtest/gtest.h>
 

src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc

@@ -19,6 +19,7 @@
 #include <dhcpsrv/lease_mgr.h>
 #include <dhcpsrv/memfile_lease_mgr.h>
 #include <dhcpsrv/tests/test_utils.h>
+#include <dhcpsrv/tests/generic_lease_mgr_unittest.h>
 #include <gtest/gtest.h>
 
 #include <iostream>
@@ -75,7 +76,9 @@ TEST_F(MemfileLeaseMgrTest, getTypeAndName) {
 
 // Checks that adding/getting/deleting a Lease6 object works.
 TEST_F(MemfileLeaseMgrTest, addGetDelete6) {
-    testAddGetDelete6(true);
+    testAddGetDelete6(true); // true - check T1,T2 values
+    // memfile is able to preserve those values, but some other
+    // backends can't do that.
 }
 
 /// @brief Basic Lease4 Checks
@@ -146,7 +149,7 @@ TEST_F(MemfileLeaseMgrTest, getLease4ClientId2) {
     testGetLease4ClientId2();
 }
 
-// @brief Get Lease4 by client ID (2)
+// @brief Get Lease4 by client ID
 //
 // Check that the system can cope with a client ID of any size.
 TEST_F(MemfileLeaseMgrTest, getLease4ClientIdSize) {
@@ -165,11 +168,16 @@ TEST_F(MemfileLeaseMgrTest, getLease4ClientIdSubnetId) {
 ///
 /// Adds leases to the database and checks that they can be accessed via
 /// a combination of DUID and IAID.
+/// @todo: test disabled, because Memfile_LeaseMgr::getLeases6(Lease::Type,
+/// const DUID& duid, uint32_t iaid) const is not implemented yet.
 TEST_F(MemfileLeaseMgrTest, DISABLED_getLeases6DuidIaid) {
     testGetLeases6DuidIaid();
 }
 
 // Check that the system can cope with a DUID of allowed size.
+
+/// @todo: test disabled, because Memfile_LeaseMgr::getLeases6(Lease::Type,
+/// const DUID& duid, uint32_t iaid) const is not implemented yet.
 TEST_F(MemfileLeaseMgrTest, DISABLED_getLeases6DuidSize) {
     testGetLeases6DuidSize();
 }
@@ -180,6 +188,8 @@ TEST_F(MemfileLeaseMgrTest, DISABLED_getLeases6DuidSize) {
 /// with alternating subnet_ids.
 /// It then verifies that all of getLeases6() method variants correctly
 /// discriminate between the leases based on lease type alone.
+/// @todo: Disabled, because type parameter in Memfile_LeaseMgr::getLease6
+/// (Lease::Type, const isc::asiolink::IOAddress& addr) const is not used.
 TEST_F(MemfileLeaseMgrTest, DISABLED_lease6LeaseTypeCheck) {
     testLease6LeaseTypeCheck();
 }
@@ -192,6 +202,8 @@ TEST_F(MemfileLeaseMgrTest, getLease6DuidIaidSubnetId) {
     testGetLease6DuidIaidSubnetId();
 }
 
+/// Checks that getLease6(type, duid, iaid, subnet-id) works with different
+/// DUID sizes
 TEST_F(MemfileLeaseMgrTest, getLease6DuidIaidSubnetIdSize) {
     testGetLease6DuidIaidSubnetIdSize();
 }
@@ -199,6 +211,9 @@ TEST_F(MemfileLeaseMgrTest, getLease6DuidIaidSubnetIdSize) {
 /// @brief Lease4 update tests
 ///
 /// Checks that we are able to update a lease in the database.
+/// @todo: Disabled, because memfile does not throw when lease is updated.
+/// We should reconsider if lease{4,6} structures should have a limit
+/// implemented in them.
 TEST_F(MemfileLeaseMgrTest, DISABLED_updateLease4) {
     testUpdateLease4();
 }
@@ -206,6 +221,9 @@ TEST_F(MemfileLeaseMgrTest, DISABLED_updateLease4) {
 /// @brief Lease6 update tests
 ///
 /// Checks that we are able to update a lease in the database.
+/// @todo: Disabled, because memfile does not throw when lease is updated.
+/// We should reconsider if lease{4,6} structures should have a limit
+/// implemented in them.
 TEST_F(MemfileLeaseMgrTest, DISABLED_updateLease6) {
     testUpdateLease6();
 }

src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc

@@ -18,9 +18,9 @@
 #include <dhcpsrv/lease_mgr_factory.h>
 #include <dhcpsrv/mysql_lease_mgr.h>
 #include <dhcpsrv/tests/test_utils.h>
+#include <dhcpsrv/tests/generic_lease_mgr_unittest.h>
 #include <exceptions/exceptions.h>
 
-
 #include <gtest/gtest.h>
 
 #include <algorithm>

src/lib/dhcpsrv/tests/test_utils.h

@@ -43,218 +43,8 @@ detailCompareLease(const Lease6Ptr& first, const Lease6Ptr& second);
 void
 detailCompareLease(const Lease4Ptr& first, const Lease4Ptr& second);
 
-/// @brief Test Fixture class with utility functions for LeaseMgr backends
-///
-/// It contains utility functions, like dummy lease creation.
-/// All concrete LeaseMgr test classes should be derived from it.
-class GenericLeaseMgrTest : public ::testing::Test {
-public:
-
-    /// @brief Default constructor.
-    GenericLeaseMgrTest();
-
-    /// @brief Virtual destructor.
-    virtual ~GenericLeaseMgrTest();
-
-    /// @brief Reopen the database
-    ///
-    /// Closes the database and re-opens it. It must be implemented
-    /// in derived classes.
-    virtual void reopen() = 0;
-
-    /// @brief Initialize Lease4 Fields
-    ///
-    /// Returns a pointer to a Lease4 structure.  Different values are put into
-    /// the lease according to the address passed.
-    ///
-    /// This is just a convenience function for the test methods.
-    ///
-    /// @param address Address to use for the initialization
-    ///
-    /// @return Lease4Ptr.  This will not point to anything if the
-    ///         initialization failed (e.g. unknown address).
-    Lease4Ptr initializeLease4(std::string address);
-
-    /// @brief Initialize Lease6 Fields
-    ///
-    /// Returns a pointer to a Lease6 structure.  Different values are put into
-    /// the lease according to the address passed.
-    ///
-    /// This is just a convenience function for the test methods.
-    ///
-    /// @param address Address to use for the initialization
-    ///
-    /// @return Lease6Ptr.  This will not point to anything if the initialization
-    ///         failed (e.g. unknown address).
-    Lease6Ptr initializeLease6(std::string address);
-
-    /// @brief Check Leases present and different
-    ///
-    /// Checks a vector of lease pointers and ensures that all the leases
-    /// they point to are present and different.  If not, a GTest assertion
-    /// will fail.
-    ///
-    /// @param leases Vector of pointers to leases
-    /// @tparam Type of the leases held in the vector: @c Lease4 or
-    /// @c Lease6.
-    template <typename T>
-    void checkLeasesDifferent(const std::vector<T>& leases) const;
-
-    /// @brief Creates leases for the test
-    ///
-    /// Creates all leases for the test and checks that they are different.
-    ///
-    /// @return vector<Lease4Ptr> Vector of pointers to leases
-    std::vector<Lease4Ptr> createLeases4();
-
-    /// @brief Creates leases for the test
-    ///
-    /// Creates all leases for the test and checks that they are different.
-    ///
-    /// @return vector<Lease6Ptr> Vector of pointers to leases
-    std::vector<Lease6Ptr> createLeases6();
-
-    /// @brief checks that addLease, getLease4(addr) and deleteLease() works
-    void testBasicLease4();
-
-    /// @brief Test lease retrieval using client id.
-    void testGetLease4ClientId();
-
-    /// @brief Test lease retrieval when leases with NULL client id are present.
-    void testGetLease4NullClientId();
-
-    /// @brief Test lease retrieval using HW address.
-    void testGetLease4HWAddr1();
-
-    /// @brief Check GetLease4 methods - access by Hardware Address
-    ///
-    /// Adds leases to the database and checks that they can be accessed using
-    /// HWAddr information.
-    void testGetLease4HWAddr2();
-
-    /// @brief Test lease retrieval using client id, HW address and subnet id.
-    void testGetLease4ClientIdHWAddrSubnetId();
-
-    // @brief Get lease4 by hardware address (2)
-    //
-    // Check that the system can cope with getting a hardware address of
-    // any size.
-    void testGetLease4HWAddrSize();
-
-    /// @brief Check GetLease4 methods - access by Hardware Address & Subnet ID
-    ///
-    /// Adds leases to the database and checks that they can be accessed via
-    /// a combination of hardware address and subnet ID
-    void testGetLease4HWAddrSubnetId();
-
-    /// @brief Get lease4 by hardware address and subnet ID (2)
-    ///
-    /// Check that the system can cope with getting a hardware address of
-    /// any size.
-    void testGetLease4HWAddrSubnetIdSize();
-
-    /// @brief Check GetLease4 methods - access by Client ID
-    ///
-    /// Adds leases to the database and checks that they can be accessed via
-    /// the Client ID.
-    void testGetLease4ClientId2();
-
-    /// @brief Get Lease4 by client ID (2)
-    ///
-    /// Check that the system can cope with a client ID of any size.
-    void testGetLease4ClientIdSize();
-
-    /// @brief Check GetLease4 methods - access by Client ID & Subnet ID
-    ///
-    /// Adds leases to the database and checks that they can be accessed via
-    /// a combination of client and subnet IDs.
-    void testGetLease4ClientIdSubnetId();
-
-    /// @brief Basic Lease4 Checks
-    ///
-    /// Checks that the addLease, getLease4(by address), getLease4(hwaddr,subnet_id),
-    /// updateLease4() and deleteLease (IPv4 address) can handle NULL client-id.
-    /// (client-id is optional and may not be present)
-    ///
-    /// @todo: check if it does overlap with @ref testGetLease4NullClientId()
-    void testLease4NullClientId();
-
-    /// @brief Basic Lease6 Checks
-    ///
-    /// Checks that the addLease, getLease6 (by address) and deleteLease (with an
-    /// IPv6 address) works.
-    void testBasicLease6();
-
-    /// @brief Test that IPv6 lease can be added, retrieved and deleted.
-    ///
-    /// This method checks basic IPv6 lease operations. There's check_t1_t2
-    /// parameter that controls whether the backend supports storing T1, T2
-    /// parameters. memfile supports it, while MySQL doesn't. If T1,T2
-    /// storage is not supported, the expected values are 0.
-    ///
-    /// @param check_t1_t2 controls whether T1,T2 timers should be checked
-    void testAddGetDelete6(bool check_t1_t2);
-
-    /// @brief Check GetLease6 methods - access by DUID/IAID
-    ///
-    /// Adds leases to the database and checks that they can be accessed via
-    /// a combination of DUID and IAID.
-    void testGetLeases6DuidIaid();
-
-    /// @brief Check that the system can cope with a DUID of allowed size.
-    void testGetLeases6DuidSize();
-
-    /// @brief Check that getLease6 methods discriminate by lease type.
-    ///
-    /// Adds six leases, two per lease type all with the same duid and iad but
-    /// with alternating subnet_ids.
-    /// It then verifies that all of getLeases6() method variants correctly
-    /// discriminate between the leases based on lease type alone.
-    void testLease6LeaseTypeCheck();
-
-    /// @brief Check GetLease6 methods - access by DUID/IAID/SubnetID
-    ///
-    /// Adds leases to the database and checks that they can be accessed via
-    /// a combination of DIUID and IAID.
-    void testGetLease6DuidIaidSubnetId();
-
-    /// @brief Checks that getLease6() works with different DUID sizes
-    void testGetLease6DuidIaidSubnetIdSize();
-
-    /// @brief Verify that too long hostname for Lease4 is not accepted.
-    ///
-    /// Checks that the it is not possible to create a lease when the hostname
-    /// length exceeds 255 characters.
-    void testLease4InvalidHostname();
-
-    /// @brief Verify that too long hostname for Lease6 is not accepted.
-    ///
-    /// Checks that the it is not possible to create a lease when the hostname
-    /// length exceeds 255 characters.
-    void testLease6InvalidHostname();
-
-    /// @brief Lease4 update test
-    ///
-    /// Checks that the code is able to update an IPv4 lease in the database.
-    void testUpdateLease4();
-
-    /// @brief Lease6 update test
-    ///
-    /// Checks that the code is able to update an IPv6 lease in the database.
-    void testUpdateLease6();
-
-    // Member variables
-    std::vector<std::string>  straddress4_;   ///< String forms of IPv4 addresses
-    std::vector<isc::asiolink::IOAddress> ioaddress4_;  ///< IOAddress forms of IPv4 addresses
-    std::vector<std::string>  straddress6_;   ///< String forms of IPv6 addresses
-    std::vector<Lease::Type> leasetype6_; ///< Lease types
-    std::vector<isc::asiolink::IOAddress> ioaddress6_;  ///< IOAddress forms of IPv6 addresses
-
-    LeaseMgr* lmptr_;                     ///< Pointer to the lease manager
-};
-
-};
-};
-};
+}; // namespace test
+}; // namespace dhcp
+}; // namespace isc
 
 #endif