[3984] Changes after review:

 - Added comments in AllocEngine
 - decline message clarified
 - counters are now unsigned
 - several asserts added in unit-tests
 - missing doxygen entry added

4 files changed, 18 insertions, 12 deletions

diff --git a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc
index 87175f9b01..55ab33e26c 100644
--- a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc
+++ b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc
@@ -737,7 +737,7 @@ public:
     ///
     /// @param remove see description above
     void testReclaimDeclined4(bool remove) {
-        for (int i = 0; i < TEST_LEASES_NUM; ++i) {
+        for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) {
 
             // Mark leases with even indexes as expired.
             if (evenLeaseIndex(i)) {
@@ -770,7 +770,7 @@ public:
         int subnet2_cnt = 0;
 
         // Let's move all leases to declined,expired state.
-        for (int i = 0; i < TEST_LEASES_NUM; ++i) {
+        for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) {
 
             // Move the lease to declined state
             decline(i, 100);
diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc b/src/lib/dhcpsrv/tests/alloc_engine_utils.cc
index 402827e9f7..8b8bbe9f38 100644
--- a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc
+++ b/src/lib/dhcpsrv/tests/alloc_engine_utils.cc
@@ -77,9 +77,9 @@ AllocEngine4Test::testReuseLease4(const AllocEnginePtr& engine,
     ASSERT_TRUE(engine);
 
     if (existing_lease) {
-        // If old lease was specified, we need to add it to the database.
-        // Let's wipe any leases for that address (if any). We ignore
-        // any errors (previous lease may not exist)
+        // If an existing lease was specified, we need to add it to the
+        // database. Let's wipe any leases for that address (if any). We
+        // ignore any errors (previous lease may not exist)
         LeaseMgrFactory::instance().deleteLease(existing_lease->addr_);
 
         // Let's add it.
@@ -114,12 +114,14 @@ Lease4Ptr
 AllocEngine4Test::generateDeclinedLease(const std::string& addr,
                                         time_t probation_period,
                                         int32_t expired) {
+    // There's an assumption that hardware address is always present for IPv4
+    // packet (always non-null). Client-id is optional (may be null).
     HWAddrPtr hwaddr(new HWAddr());
     time_t now = time(NULL);
     Lease4Ptr declined(new Lease4(addr, hwaddr, ClientIdPtr(), 495,
                                   100, 200, now, subnet_->getID()));
     declined->decline(probation_period);
-    declined->cltt_ = time(NULL) - probation_period + expired;
+    declined->cltt_ = now - probation_period + expired;
     return (declined);
 }
 
diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.h b/src/lib/dhcpsrv/tests/alloc_engine_utils.h
index 6c9aa59de2..d00158501f 100644
--- a/src/lib/dhcpsrv/tests/alloc_engine_utils.h
+++ b/src/lib/dhcpsrv/tests/alloc_engine_utils.h
@@ -389,7 +389,7 @@ public:
 
     /// @brief Generic test used for lease allocation and reuse
     ///
-    /// This test inserts old_lease (if specified, may be null) into the
+    /// This test inserts existing_lease (if specified, may be null) into the
     /// LeaseMgr, then conducts lease allocation (pretends that client
     /// sent either Discover or Request, depending on fake_allocation).
     /// Allocated lease is then returned (using result) for further inspection.
@@ -397,6 +397,7 @@ public:
     /// @param alloc_engine allocation engine
     /// @param existing_lease optional lease to be inserted in the database
     /// @param addr address to be requested by client
+    /// @param fake_allocation true = DISCOVER, false = REQUEST
     /// @param exp_result expected result
     /// @param result [out] allocated lease
     void testReuseLease4(const AllocEnginePtr& alloc_engine,
@@ -410,9 +411,9 @@ public:
     ///
     /// expired parameter controls probation period. Positive value
     /// means that the lease will expire in X seconds. Negative means
-    /// that the lease had expired X seconds ago. 0 means it expires now.
+    /// that the lease expired X seconds ago. 0 means it expires now.
     /// Probation period is a parameter that specifies for how long
-    /// a lease will become unavailable after decline.
+    /// a lease will stay unavailable after decline.
     ///
     /// @param addr address of the lease
     /// @param probation_period expressed in seconds
diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc
index 0e5ba491b9..efcb58d91e 100644
--- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc
@@ -1691,6 +1691,7 @@ GenericLeaseMgrTest::testGetExpiredLeases4() {
         int index = static_cast<int>(std::distance(expired_leases.rbegin(), lease));
         // Multiple current index by two, because only leases with even indexes
         // should have been returned.
+        ASSERT_LE(2 * index, leases.size());
         EXPECT_EQ(leases[2 * index]->addr_, (*lease)->addr_);
     }
 
@@ -1723,6 +1724,7 @@ GenericLeaseMgrTest::testGetExpiredLeases4() {
     for (Lease4Collection::iterator lease = expired_leases.begin();
          lease != expired_leases.end(); ++lease) {
         int index = static_cast<int>(std::distance(expired_leases.begin(), lease));
+        ASSERT_LE(2 * index, leases.size());
         EXPECT_EQ(leases[2 * index]->addr_, (*lease)->addr_);
     }
 
@@ -1742,6 +1744,7 @@ GenericLeaseMgrTest::testGetExpiredLeases4() {
     for (Lease4Collection::iterator lease = expired_leases.begin();
          lease != expired_leases.end(); ++lease) {
         int index = static_cast<int>(std::distance(expired_leases.begin(), lease));
+        ASSERT_LE(2 * index, leases.size());
         EXPECT_EQ(leases[2 * index]->addr_, (*lease)->addr_);
     }
 
@@ -2104,7 +2107,7 @@ GenericLeaseMgrTest::testGetDeclinedLeases4() {
             leases[i]->decline(1000);
         }
 
-        // Mark every other lease as expired.
+        // Mark every second lease as expired.
         if (i % 2 == 0) {
             // Set client last transmission time to the value older than the
             // valid lifetime to make it expired. The expiration time also
@@ -2140,8 +2143,8 @@ GenericLeaseMgrTest::testGetDeclinedLeases4() {
     // not pay attention to state, just expiration timers.
     ASSERT_EQ(static_cast<size_t>(leases.size() / 2), expired_leases.size());
 
-    int declined_state = 0;
-    int default_state = 0;
+    unsigned int declined_state = 0;
+    unsigned int default_state = 0;
 
     // The expired leases should be returned from the most to least expired.
     // This matches the reverse order to which they have been added.