Skip to content

test: Correct the cause recorded for the acknowledged-alert test and drop the redundant action - #2316

Merged
eirsep merged 1 commit into
opensearch-project:mainfrom
eirsep:fix-2315-review
Oct 30, 2026
Merged

eirsep merged 1 commit into
opensearch-project:mainfrom
eirsep:fix-2315-review

Conversation

@eirsep

@eirsep eirsep commented Oct 28, 2026

Copy link
Copy Markdown
Member

Description

Follow-up to #2315, addressing @lezzago's review. That PR recorded the wrong cause and carried a
second change that was not needed.

The explanation was wrong. PER_EXECUTION actions do advance lastNotificationTime — that branch
adds every deduped, new and completed alert to alertsToUpdate
(BucketLevelMonitorRunner.kt:395-409). An all-PER_EXECUTION trigger was never the problem, and the
comment #2315 left in the test said it was.

The actual cause is that randomActionsForBucketLevelTrigger(min = 1) returned an empty list, and
randomBucketLevelTrigger replaces an empty actions list with freshly generated ones
(TestHelpers.kt:370):

actions = if (actions.isEmpty()) randomActionsForBucketLevelTrigger(destinationId = destinationId) else actions

Those regenerated actions never pass through the rewrite the test applies, so a PER_ALERT policy
without COMPLETED reaches the trigger. At the seed from the original failure they were
[PerAlertActionScope{DEDUPED}, PerAlertActionScope{}].

Change

Making min a floor — the other half of #2315 — fixes this on its own, so the PER_ALERT action that PR
prepended is removed. Besides being unnecessary, it guaranteed a PER_ALERT action carrying COMPLETED
on every run, which stopped this test from ever exercising an all-PER_EXECUTION trigger: it narrowed
coverage to buy nothing. It also duplicated the ActionExecutionPolicy expression within one statement.

The test comment now states the real cause, and the helper's KDoc warns that an empty result is not
harmless for callers that post-process it, since that is the trap behind the original failure.

Testing

With the prepend removed:

result
seed from the original failing CI job (F63F1F740575292F, locale ki, timezone ACT) pass
10 further random seeds 10/10 pass

ktlint clean. No production code is touched.

Still open

maxOf(min, 1) means an explicit min = 0 can no longer yield zero actions, which the PR bot flagged
on #2315. Given TestHelpers.kt:370 treats an empty list as "regenerate", zero actions is not
reachable through randomBucketLevelTrigger either way. Making it genuinely reachable means stopping
that silent regeneration, which changes every caller's action distribution and belongs in its own
change rather than here.

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…the redundant action

Addresses review feedback on opensearch-project#2315. The comment that PR added named the wrong cause and the fix
carried a second change that was not needed.

PER_EXECUTION actions do advance lastNotificationTime: that branch adds every deduped, new and
completed alert to alertsToUpdate (BucketLevelMonitorRunner.kt:395-409), so an all-PER_EXECUTION
trigger was never the problem.

The actual cause is that randomActionsForBucketLevelTrigger(min = 1) returned an empty list, and
randomBucketLevelTrigger replaces an empty actions list with freshly generated ones (TestHelpers.kt:370).
Those never pass through the rewrite the test applies, so a PER_ALERT policy without COMPLETED reaches
the trigger; at the seed from the original failure the regenerated actions were
[PerAlertActionScope{DEDUPED}, PerAlertActionScope{}].

Making min a floor therefore fixes this on its own. The PER_ALERT action opensearch-project#2315 prepended is removed:
it was justified by the wrong explanation, and because it guaranteed a PER_ALERT action carrying
COMPLETED on every run, it stopped this test from ever exercising an all-PER_EXECUTION trigger. It also
duplicated the ActionExecutionPolicy expression within one statement.

Verified with the prepend removed: the test passes at the seed from the original failing CI job
(F63F1F740575292F, locale ki, timezone ACT) and on 10 further random seeds, 10/10. ktlint clean.

The KDoc now also warns that an empty result is not harmless for callers that post-process it, which
is the trap that produced the original failure.

Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@codecov

codecov Bot commented Oct 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.71%. Comparing base (778e7ce) to head (3855ec0).
⚠️ Report is 254 commits behind head on main.

❌ Your project check has failed because the head coverage (67.71%) is below the target coverage (70.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2316      +/-   ##
============================================
- Coverage     67.72%   67.71%   -0.02%     
- Complexity      105     1795    +1690     
============================================
  Files           160      169       +9     
  Lines         10343    14269    +3926     
  Branches       1522     2077     +555     
============================================
+ Hits           7005     9662    +2657     
- Misses         2672     3728    +1056     
- Partials        666      879     +213     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines +1526 to +1529
// The rewrite below has to leave a non-empty list. randomBucketLevelTrigger replaces an empty
// actions list with freshly generated ones, and those never pass through this rewrite, so a
// PER_ALERT policy without COMPLETED can reach the trigger and leave the completed alert's
// lastNotificationTime untouched.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Regenerated actions can also lack DEDUPED (e.g. PerAlertActionScope{NEW}), and then the test fails earlier, at "Active alert was not updated", rather than at the completed-alert check. Maybe: "...so a PER_ALERT policy missing DEDUPED or COMPLETED can reach the trigger and leave lastNotificationTime untouched."

// actions list with freshly generated ones, and those never pass through this rewrite, so a
// PER_ALERT policy without COMPLETED can reach the trigger and leave the completed alert's
// lastNotificationTime untouched.
val actions = randomActionsForBucketLevelTrigger(min = 1).map {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The non-empty guarantee the comment above depends on lives in the helper's maxOf(min, 1), which the description suggests revisiting. An assertTrue(actions.isNotEmpty()) after building actions would make this test fail clearly instead of flaking again if that helper changes.

@eirsep
eirsep merged commit f9b2214 into opensearch-project:main Oct 30, 2026
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants