From a167d657601c18a700e1ce3939a928e5a88a0c6e Mon Sep 17 00:00:00 2001
From: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>
Date: Wed, 2 Mar 2022 17:44:18 -0800
Subject: [PATCH] Fix flaky tests for Bucket-Level Monitor (#318)

Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
---
 .../opensearch/alerting/MonitorRunnerIT.kt    | 37 ++++++++++++++-----
 .../org/opensearch/alerting/TestHelpers.kt    |  5 ++-
 2 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/alerting/src/test/kotlin/org/opensearch/alerting/MonitorRunnerIT.kt b/alerting/src/test/kotlin/org/opensearch/alerting/MonitorRunnerIT.kt
index 4249495a2..c7dad3743 100644
--- a/alerting/src/test/kotlin/org/opensearch/alerting/MonitorRunnerIT.kt
+++ b/alerting/src/test/kotlin/org/opensearch/alerting/MonitorRunnerIT.kt
@@ -1061,7 +1061,10 @@ class MonitorRunnerIT : AlertingRestTestCase() {
         var alerts = searchAlerts(monitor)
         assertEquals("Alerts not saved", 2, alerts.size)
         alerts.forEach {
-            verifyAlert(it, monitor, ACTIVE)
+            // Given the random configuration of the Bucket-Level Trigger for the test, it's possible to get
+            // an action configuration that leads to no notifications (meaning the field for the Alert is null).
+            // Since testing action execution is not relevant to this test, verifyAlert is asked to ignore it.
+            verifyAlert(it, monitor, ACTIVE, expectNotification = false)
         }
 
         // Delete documents of a particular value
@@ -1107,7 +1110,21 @@ class MonitorRunnerIT : AlertingRestTestCase() {
             params.docCount > 0
         """.trimIndent()
 
-        var trigger = randomBucketLevelTrigger()
+        // For the Actions ensure that there is at least one and any PER_ALERT actions contain ACTIVE, DEDUPED and COMPLETED in its policy
+        // so that the assertions done later in this test don't fail.
+        // The config is being mutated this way to still maintain the randomness in configuration (like including other ActionExecutionScope).
+        val actions = randomActionsForBucketLevelTrigger(min = 1).map {
+            if (it.actionExecutionPolicy?.actionExecutionScope is PerAlertActionScope) {
+                it.copy(
+                    actionExecutionPolicy = ActionExecutionPolicy(
+                        PerAlertActionScope(setOf(AlertCategory.NEW, AlertCategory.DEDUPED, AlertCategory.COMPLETED))
+                    )
+                )
+            } else {
+                it
+            }
+        }
+        var trigger = randomBucketLevelTrigger(actions = actions)
         trigger = trigger.copy(
             bucketSelector = BucketSelectorExtAggregationBuilder(
                 name = trigger.id,
@@ -1163,11 +1180,6 @@ class MonitorRunnerIT : AlertingRestTestCase() {
         assertEquals("Incorrect number of completed alerts", 2, completedAlerts.size)
         val previouslyAcknowledgedAlert = completedAlerts.single { it.aggregationResultBucket?.getBucketKeysHash().equals("test_value_1") }
         val previouslyActiveAlert = completedAlerts.single { it.aggregationResultBucket?.getBucketKeysHash().equals("test_value_2") }
-        // Note: Given the randomization of the Actions and ActionExecutionPolicy for the Bucket-Level Monitor
-        // there is a very small chance we could end up with COMPLETED Alerts that never had lastNotificationTime updated
-        // (This would occur if the Trigger contained Actions with ActionExecutionScope of PER_ALERT that all somehow excluded the
-        // same Alert categories being tested in this test)
-        // In such a rare case, the tests can just be rerun
         assertTrue(
             "Previously acknowledged alert was not updated when it moved to completed",
             previouslyAcknowledgedAlert.lastNotificationTime!! > acknowledgedAlert2.lastNotificationTime
@@ -1593,10 +1605,17 @@ class MonitorRunnerIT : AlertingRestTestCase() {
         }
     }
 
-    private fun verifyAlert(alert: Alert, monitor: Monitor, expectedState: Alert.State = ACTIVE) {
+    private fun verifyAlert(
+        alert: Alert,
+        monitor: Monitor,
+        expectedState: Alert.State = ACTIVE,
+        expectNotification: Boolean = true
+    ) {
         assertNotNull(alert.id)
         assertNotNull(alert.startTime)
-        assertNotNull(alert.lastNotificationTime)
+        if (expectNotification) {
+            assertNotNull(alert.lastNotificationTime)
+        }
         assertEquals("Alert in wrong state", expectedState, alert.state)
         if (expectedState == ERROR) {
             assertNotNull("Missing error message", alert.errorMessage)
diff --git a/alerting/src/test/kotlin/org/opensearch/alerting/TestHelpers.kt b/alerting/src/test/kotlin/org/opensearch/alerting/TestHelpers.kt
index 58671be20..f9cbdfe79 100644
--- a/alerting/src/test/kotlin/org/opensearch/alerting/TestHelpers.kt
+++ b/alerting/src/test/kotlin/org/opensearch/alerting/TestHelpers.kt
@@ -178,10 +178,13 @@ fun randomBucketLevelTrigger(
         name = name,
         severity = severity,
         bucketSelector = bucketSelector,
-        actions = if (actions.isEmpty()) (0..randomInt(10)).map { randomActionWithPolicy(destinationId = destinationId) } else actions
+        actions = if (actions.isEmpty()) randomActionsForBucketLevelTrigger(destinationId = destinationId) else actions
     )
 }
 
+fun randomActionsForBucketLevelTrigger(min: Int = 0, max: Int = 10, destinationId: String = ""): List<Action> =
+    (min..randomInt(max)).map { randomActionWithPolicy(destinationId = destinationId) }
+
 fun randomBucketSelectorExtAggregationBuilder(
     name: String = OpenSearchRestTestCase.randomAlphaOfLength(10),
     bucketsPathsMap: MutableMap<String, String> = mutableMapOf("avg" to "10"),