-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Alerts] Fixing skipped unit tests for metric threshold rule #162189
[Alerts] Fixing skipped unit tests for metric threshold rule #162189
Conversation
Pinging @elastic/actionable-observability (Team: Actionable Observability) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Chris, for the fix! ❤️
Just added one question.
@@ -1394,7 +1394,7 @@ describe.skip('The metric threshold alert type', () => { | |||
await execute(true); | |||
const recentAction = mostRecentAction(instanceID); | |||
expect(recentAction.action).toEqual({ | |||
alertDetailsUrl: 'http://localhost:5601/app/observability/alerts/mock-alert-uuid', | |||
alertDetailsUrl: '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also remove the skip from this test? If not, I will create a separate ticket for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops... I missed that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That test needs a lot more work before we are ready to remove the skip
statement. It's failing beyond just the single test this PR is fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so, here is the ticket for it: #162299
…that's outside the scope of this PR
…xing-test-for-no-data
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
…#162189) ## Summary This PR fixes elastic#155534 by ensuring result for the group by is set before adding it to the evaluation results. I also applied the fix to the new Observability Threshold Rule since we are using it as a starting point for the new combined rule.
Summary
This PR fixes #155534 by ensuring result for the group by is set before adding it to the evaluation results. I also applied the fix to the new Observability Threshold Rule since we are using it as a starting point for the new combined rule.