[#3278] Addressed review comments

    Minor corrections in ARM and UTs

diff --git a/doc/sphinx/arm/hooks-perfmon.rst b/doc/sphinx/arm/hooks-perfmon.rst
index b5207cc4bcca4fb88a4d61f9adf73165e4df724f..224a0ad4d488701bedcbf8d0e7a498956802d2b8 100644 (file)
--- a/doc/sphinx/arm/hooks-perfmon.rst
+++ b/doc/sphinx/arm/hooks-perfmon.rst
@@ -47,7 +47,7 @@ shown below:
 #. process_completed - Server has constructed the response and is ready to send it
 
 This list is likely to expand over time. It will also be possible for hook developers
-to add their own events. This will be detailed in a future release in t.
+to add their own events. This will be detailed in a future release.
 
 Passive Event Logging
 ~~~~~~~~~~~~~~~~~~~~~
@@ -90,8 +90,10 @@ uniquely identified by a "duration key" which consists of the following values:
 * stop event label - Event that defines the end of the task (e.g. buffer_read, process_completed)
 * subnet id - subnet selected during message processing (or 0 for global durations)
 
-As client queries are responded to their event stacks are used add to the monitored
-durations. When the sampling interval has been reached for a given duration, it is reported.
+Once the server has finished constructing a response to a query, the query's event stack
+is processed into a series of updates to monitored durations.  If upon updating, a
+duration's sample interval is found to have been completed, it is sent to reporting
+and a new sample  interval is begun.
 
 .. Note:
     Monitoring is not yet functional.
@@ -112,7 +114,7 @@ below the low-water mark the alarm is "cleared" and an INFO level log will be em
 API Commands
 ~~~~~~~~~~~~
 
-    Commands to enable or disable monitoring, clear or alter alarms, and fetch duration datax
+    Commands to enable or disable monitoring, clear or alter alarms, and fetch duration data
     are anticipated but not yet supported.
 
 Configuration

diff --git a/src/hooks/dhcp/perfmon/tests/alarm_parser_unittests.cc b/src/hooks/dhcp/perfmon/tests/alarm_parser_unittests.cc
index dfc0959973a0f195338351fda11d3e21681ed557..afb4c4a5e772fe17b47d21d63f1ba306119dd4f4 100644 (file)
--- a/src/hooks/dhcp/perfmon/tests/alarm_parser_unittests.cc
+++ b/src/hooks/dhcp/perfmon/tests/alarm_parser_unittests.cc
@@ -182,6 +182,26 @@ public:
                 )",
                 "'high-water-ms' parameter is not an integer"
             },
+            {
+                // Invalid zero value for high-water-ms
+                __LINE__,
+                R"(
+                    "enable-alarm": true,
+                    "high-water-ms": 0,
+                    "low-water-ms": 25
+                )",
+                "high-water-ms: '0', must be greater than 0"
+            },
+            {
+                // Invalid negative value for high-water-ms
+                __LINE__,
+                R"(
+                    "enable-alarm": true,
+                    "high-water-ms": -1,
+                    "low-water-ms": 25
+                )",
+                "high-water-ms: '-1', must be greater than 0"
+            },
             {
                 // Missing low-water-ms
                 __LINE__,
@@ -201,6 +221,26 @@ public:
                 )",
                 "'low-water-ms' parameter is not an integer"
             },
+            {
+                // Invalid zero value for low-water-ms
+                __LINE__,
+                R"(
+                    "enable-alarm": true,
+                    "high-water-ms": 500,
+                    "low-water-ms": 0
+                )",
+                "low-water-ms: '0', must be greater than 0"
+            },
+            {
+                // Invalid negative value for low-water-ms
+                __LINE__,
+                R"(
+                    "enable-alarm": true,
+                    "high-water-ms": 500,
+                    "low-water-ms": -1
+                )",
+                "low-water-ms: '-1', must be greater than 0"
+            },
             {
                 // Invalid threshold combination
                 __LINE__,

diff --git a/src/hooks/dhcp/perfmon/tests/duration_key_parser_unittests.cc b/src/hooks/dhcp/perfmon/tests/duration_key_parser_unittests.cc
index e7b644561262bc49e4925b74df7119a95d3ea008..2e22841f7a8e070958e447c435d72873abf0e6f9 100644 (file)
--- a/src/hooks/dhcp/perfmon/tests/duration_key_parser_unittests.cc
+++ b/src/hooks/dhcp/perfmon/tests/duration_key_parser_unittests.cc
@@ -165,19 +165,6 @@ TEST_F(DurationKeyParserTest, validScenarios4) {
             })",
             DHCPDISCOVER, DHCPOFFER, "start_here", "stop_there", SUBNET_ID_GLOBAL
         },
-        {
-            // Empty message types should map to DHCP_NOTYPE
-            __LINE__,
-            R"(
-            {
-                "query-type": "",
-                "response-type": "",
-                "start-event": "start_here",
-                "stop-event": "stop_there",
-                "subnet-id": 701
-            })",
-            DHCP_NOTYPE, DHCP_NOTYPE, "start_here", "stop_there", 701
-        },
         {
             // "*" message types should map to DHCP_NOTYPE
             __LINE__,
@@ -412,19 +399,6 @@ TEST_F(DurationKeyParserTest, validScenarios6) {
             })",
             DHCPV6_SOLICIT, DHCPV6_ADVERTISE, "start_here", "stop_there", SUBNET_ID_GLOBAL
         },
-        {
-            // Empty message types should map to DHCPV6_NOTYPE
-            __LINE__,
-            R"(
-            {
-                "query-type": "",
-                "response-type": "",
-                "start-event": "start_here",
-                "stop-event": "stop_there",
-                "subnet-id": 701
-            })",
-            DHCPV6_NOTYPE, DHCPV6_NOTYPE, "start_here", "stop_there", 701
-        },
         {
             // "*" message types should map to DHCPV6_NOTYPE
             __LINE__,

diff --git a/src/hooks/dhcp/perfmon/tests/perfmon_config_unittests.cc b/src/hooks/dhcp/perfmon/tests/perfmon_config_unittests.cc
index ea16a25156352b7a136a3461e7ed06f3862f5778..a46d95320f03aef0812b72cd9abb47591a8acd84 100644 (file)
--- a/src/hooks/dhcp/perfmon/tests/perfmon_config_unittests.cc
+++ b/src/hooks/dhcp/perfmon/tests/perfmon_config_unittests.cc
@@ -193,6 +193,18 @@ public:
                 })",
                 "'enable-monitoring' parameter is not a boolean"
             },
+            {
+                // Invalid type for interval-width-secs
+                __LINE__,
+                R"(
+                {
+                    "enable-monitoring" : false,
+                    "interval-width-secs" : "bogus",
+                    "stats-mgr-reporting" : false,
+                    "alarm-report-secs" : 90
+                })",
+                "'interval-width-secs' parameter is not an integer"
+            },
             {
                 // Value of interval-width-secs is zero
                 __LINE__,
@@ -229,6 +241,18 @@ public:
                 })",
                 "'stats-mgr-reporting' parameter is not a boolean"
             },
+            {
+                // Invalid type for alarm-report-secs
+                __LINE__,
+                R"(
+                {
+                    "enable-monitoring" : false,
+                    "interval-width-secs" : 5,
+                    "stats-mgr-reporting" : false,
+                    "alarm-report-secs" : "bogus"
+                })",
+                "'alarm-report-secs' parameter is not an integer"
+            },
             {
                 // Value of alarm-report-secs is zero
                 __LINE__,

diff --git a/src/hooks/dhcp/perfmon/tests/perfmon_mgr_unittests.cc b/src/hooks/dhcp/perfmon/tests/perfmon_mgr_unittests.cc
index 30bd5af799e4bae3ab20d451182ce28c22c5b657..62f2dbe0216c17f4d1b69feb8b7f88f26b51c205 100644 (file)
--- a/src/hooks/dhcp/perfmon/tests/perfmon_mgr_unittests.cc
+++ b/src/hooks/dhcp/perfmon/tests/perfmon_mgr_unittests.cc
@@ -133,13 +133,11 @@ public:
         ASSERT_EQ(durations->size(), 0);
     }
 
-    /// @brief Exercises PerfMonConfig parameter parsing with valid configuration
-    /// permutations.
-    /// @todo add alarms
+    /// @brief Verifies that PerfMonConfig handles a configuration error properly.
     void testInvalidConfig() {
-        std::string valid_config =
+        std::string invalid_config =
             R"({
-                    "enable-monitoring": false,
+                    "enable-monitoring": true,
                     "interval-width-secs": 5,
                     "stats-mgr-reporting": false,
                     "alarm-report-secs": 600,
@@ -148,15 +146,17 @@ public:
 
         // Convert JSON texts to Element map.
         ConstElementPtr json_elements;
-        ASSERT_NO_THROW_LOG(json_elements = Element::fromJSON(valid_config));
+        ASSERT_NO_THROW_LOG(json_elements = Element::fromJSON(invalid_config));
 
         PerfMonMgrPtr mgr(new PerfMonMgr(family_));
-        ASSERT_NO_THROW_LOG(mgr->configure(json_elements));
+        ASSERT_THROW_MSG(mgr->configure(json_elements), DhcpConfigError,
+                         "PerfMonMgr::configure failed - "
+                         "'alarms' parameter is not a list");
 
         EXPECT_FALSE(mgr->getEnableMonitoring());
-        EXPECT_EQ(mgr->getIntervalDuration(), seconds(5));
-        EXPECT_FALSE(mgr->getStatsMgrReporting());
-        EXPECT_EQ(mgr->getAlarmReportInterval(), seconds(600));
+        EXPECT_EQ(mgr->getIntervalDuration(), seconds(60));
+        EXPECT_TRUE(mgr->getStatsMgrReporting());
+        EXPECT_EQ(mgr->getAlarmReportInterval(), seconds(300));
 
         // Alarm store should exist but be empty.
         EXPECT_TRUE(mgr->getAlarmStore());
@@ -213,4 +213,12 @@ TEST_F(PerfMonMgrTest6, validConfig) {
     testValidConfig();
 }
 
+TEST_F(PerfMonMgrTest4, invalidConfig) {
+    testInvalidConfig();
+}
+
+TEST_F(PerfMonMgrTest6, invalidConfig) {
+    testInvalidConfig();
+}
+
 } // end of anonymous namespace