[#3278] Addressed review comments

    Minor corrections in ARM and UTs

4 files changed, 82 insertions, 36 deletions

diff --git a/src/hooks/dhcp/perfmon/tests/alarm_parser_unittests.cc b/src/hooks/dhcp/perfmon/tests/alarm_parser_unittests.cc
index dfc0959973..afb4c4a5e7 100644
--- a/src/hooks/dhcp/perfmon/tests/alarm_parser_unittests.cc
+++ b/src/hooks/dhcp/perfmon/tests/alarm_parser_unittests.cc
@@ -183,6 +183,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__,
                 R"(
@@ -202,6 +222,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__,
                 R"(
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 e7b6445612..2e22841f7a 100644
--- a/src/hooks/dhcp/perfmon/tests/duration_key_parser_unittests.cc
+++ b/src/hooks/dhcp/perfmon/tests/duration_key_parser_unittests.cc
@@ -166,19 +166,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__,
             R"(
@@ -413,19 +400,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__,
             R"(
diff --git a/src/hooks/dhcp/perfmon/tests/perfmon_config_unittests.cc b/src/hooks/dhcp/perfmon/tests/perfmon_config_unittests.cc
index ea16a25156..a46d95320f 100644
--- a/src/hooks/dhcp/perfmon/tests/perfmon_config_unittests.cc
+++ b/src/hooks/dhcp/perfmon/tests/perfmon_config_unittests.cc
@@ -194,6 +194,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__,
                 R"(
@@ -230,6 +242,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__,
                 R"(
diff --git a/src/hooks/dhcp/perfmon/tests/perfmon_mgr_unittests.cc b/src/hooks/dhcp/perfmon/tests/perfmon_mgr_unittests.cc
index 30bd5af799..62f2dbe021 100644
--- 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