[2981] Final modifications before review

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -1870,12 +1870,13 @@ TEST_F(Dhcp4ParserTest, NoHooksLibraries) {
     string config = buildHooksLibrariesConfig();
     if (!executeConfiguration(config,
                               "set configuration with no hooks libraries")) {
-        return;
-    }
+        FAIL() << "Unable to execute configuration";
 
-    // No libraries should be loaded at the end of the test.
-    libraries = HooksManager::getLibraryNames();
-    ASSERT_TRUE(libraries.empty());
+    } else {
+        // No libraries should be loaded at the end of the test.
+        libraries = HooksManager::getLibraryNames();
+        EXPECT_TRUE(libraries.empty());
+    }
 }
 
 // Verify parsing fails with one library that will fail validation.
@@ -1906,8 +1907,8 @@ TEST_F(Dhcp4ParserTest, LibrariesSpecified) {
     ASSERT_TRUE(libraries.empty());
 
     // Marker files should not be present.
-    EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, NULL));
-    EXPECT_TRUE(checkMarkerFile(UNLOAD_MARKER_FILE, NULL));
+    EXPECT_FALSE(checkMarkerFileExists(LOAD_MARKER_FILE));
+    EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE));
 
     // Set up the configuration with two libraries and load them.
     string config = buildHooksLibrariesConfig(CALLOUT_LIBRARY_1,
@@ -1917,10 +1918,10 @@ TEST_F(Dhcp4ParserTest, LibrariesSpecified) {
 
     // Expect two libraries to be loaded in the correct order (load marker file
     // is present, no unload marker file).
-     libraries = HooksManager::getLibraryNames();
-     ASSERT_EQ(2, libraries.size());
-     EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "12"));
-     EXPECT_TRUE(checkMarkerFile(UNLOAD_MARKER_FILE, NULL));
+    libraries = HooksManager::getLibraryNames();
+    ASSERT_EQ(2, libraries.size());
+    EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "12"));
+    EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE));
 
     // Unload the libraries.  The load file should not have changed, but
     // the unload one should indicate the unload() functions have been run.

src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc

@@ -123,7 +123,9 @@ TEST_F(CtrlDhcpv4SrvTest, libreload) {
     ASSERT_TRUE(libraries == loaded_libraries);
 
     // ... which also included checking that the marker file created by the
-    // load functions exists.
+    // load functions exists and holds the correct value (of "12" - the
+    // first library appends "1" to the file, the second appends "2"). Also
+    // check that the unload marker file does not yet exist.
     EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "12"));
     EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE));
 

src/bin/dhcp4/tests/marker_file.cc

@@ -34,22 +34,10 @@ checkMarkerFile(const char* name, const char* expected) {
 
     // Is it open?
     if (!file.is_open()) {
-
-        // No.  This is OK if we don't expected is to be present but is
-        // a failure otherwise.
-        if (expected == NULL) {
-            return (true);
-        }
         ADD_FAILURE() << "Unable to open " << name << ". It was expected "
                       << "to be present and to contain the string '"
                       << expected << "'";
         return (false);
-    } else if (expected == NULL) {
-
-        // File is open but we don't expect it to be present.
-        ADD_FAILURE() << "Opened " << name << " but it is not expected to "
-                      << "be present.";
-        return (false);
     }
 
     // OK, is open, so read the data and see what we have.  Compare it
@@ -58,7 +46,8 @@ checkMarkerFile(const char* name, const char* expected) {
     getline(file, content);
 
     string expected_str(expected);
-    EXPECT_EQ(expected_str, content) << "Data was read from " << name;
+    EXPECT_EQ(expected_str, content) << "Marker file " << name
+        << "did not contain the expected data";
     file.close();
 
     return (expected_str == content);

src/bin/dhcp4/tests/marker_file.h.in

@@ -42,9 +42,7 @@ namespace test {
 ///
 /// @param name Name of the marker file.
 /// @param expected Characters expected.  If a marker file is present,
-///        it is expected to contain characters.  Therefore a value of NULL
-///        is used to signify that the marker file is not expected to be
-///        present.
+///        it is expected to contain characters.
 ///
 /// @return true if all tests pass, false if not (in which case a failure
 ///         will have been logged).

src/bin/dhcp6/config_parser.cc

@@ -596,7 +596,7 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
     }
 
     // Now commit any changes that have been validated but not yet committed,
-    // but which can't be rolled back.
+    // and which can't be rolled back.
     if (hooks_parser) {
         hooks_parser->commit();
     }

src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -1985,12 +1985,13 @@ TEST_F(Dhcp6ParserTest, NoHooksLibraries) {
     string config = buildHooksLibrariesConfig();
     if (!executeConfiguration(config,
                               "set configuration with no hooks libraries")) {
-        return;
-    }
+        FAIL() << "Unable to execute configuration";
 
-    // No libraries should be loaded at the end of the test.
-    libraries = HooksManager::getLibraryNames();
-    ASSERT_TRUE(libraries.empty());
+    } else {
+        // No libraries should be loaded at the end of the test.
+        libraries = HooksManager::getLibraryNames();
+        EXPECT_TRUE(libraries.empty());
+    }
 }
 
 // Verify parsing fails with one library that will fail validation.
@@ -2021,8 +2022,8 @@ TEST_F(Dhcp6ParserTest, LibrariesSpecified) {
     ASSERT_TRUE(libraries.empty());
 
     // Marker files should not be present.
-    EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, NULL));
-    EXPECT_TRUE(checkMarkerFile(UNLOAD_MARKER_FILE, NULL));
+    EXPECT_FALSE(checkMarkerFileExists(LOAD_MARKER_FILE));
+    EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE));
 
     // Set up the configuration with two libraries and load them.
     string config = buildHooksLibrariesConfig(CALLOUT_LIBRARY_1,
@@ -2035,7 +2036,7 @@ TEST_F(Dhcp6ParserTest, LibrariesSpecified) {
      libraries = HooksManager::getLibraryNames();
      ASSERT_EQ(2, libraries.size());
      EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "12"));
-     EXPECT_TRUE(checkMarkerFile(UNLOAD_MARKER_FILE, NULL));
+     EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE));
 
     // Unload the libraries.  The load file should not have changed, but
     // the unload one should indicate the unload() functions have been run.

src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc

@@ -123,7 +123,9 @@ TEST_F(CtrlDhcpv6SrvTest, libreload) {
     ASSERT_TRUE(libraries == loaded_libraries);
 
     // ... which also included checking that the marker file created by the
-    // load functions exists.
+    // load functions exists and holds the correct value (of "12" - the
+    // first library appends "1" to the file, the second appends "2"). Also
+    // check that the unload marker file does not yet exist.
     EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "12"));
     EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE));
 

src/bin/dhcp6/tests/marker_file.cc

@@ -34,22 +34,10 @@ checkMarkerFile(const char* name, const char* expected) {
 
     // Is it open?
     if (!file.is_open()) {
-
-        // No.  This is OK if we don't expected is to be present but is
-        // a failure otherwise.
-        if (expected == NULL) {
-            return (true);
-        }
         ADD_FAILURE() << "Unable to open " << name << ". It was expected "
                       << "to be present and to contain the string '"
                       << expected << "'";
         return (false);
-    } else if (expected == NULL) {
-
-        // File is open but we don't expect it to be present.
-        ADD_FAILURE() << "Opened " << name << " but it is not expected to "
-                      << "be present.";
-        return (false);
     }
 
     // OK, is open, so read the data and see what we have.  Compare it
@@ -58,7 +46,8 @@ checkMarkerFile(const char* name, const char* expected) {
     getline(file, content);
 
     string expected_str(expected);
-    EXPECT_EQ(expected_str, content) << "Data was read from " << name;
+    EXPECT_EQ(expected_str, content) << "Marker file " << name
+        << "did not contain the expected data";
     file.close();
 
     return (expected_str == content);

src/bin/dhcp6/tests/marker_file.h.in

@@ -42,9 +42,7 @@ namespace test {
 ///
 /// @param name Name of the marker file.
 /// @param expected Characters expected.  If a marker file is present,
-///        it is expected to contain characters.  Therefore a value of NULL
-///        is used to signify that the marker file is not expected to be
-///        present.
+///        it is expected to contain characters.
 ///
 /// @return true if all tests pass, false if not (in which case a failure
 ///         will have been logged).