-
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
Geo containment alert sparsity handling: preserve active status for non-updated alerts #85364
Geo containment alert sparsity handling: preserve active status for non-updated alerts #85364
Conversation
Pinging @elastic/kibana-gis (Team:Geo) |
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.
thx for the additions and offline explanations.
For the test coverage, I would actually test that the appropriate calls to scheduleActions
are being made for a given alertInstance.
You could do that by creating a new alertInstanceFactory in each test. Something like:
const alerts = []
const alertInstanceFactory = (instance: string) => (
return {
scheduleActions: (alert) => {
alerts.push({instance, alert});
};
});
const newmap = getActiveEntriesAndGenerateAlerts(..., alertInstanceFactory,...);
expect(alerts).toEqual(...);
^. something like that, details tbd ;)
The reason for this is that this method does two things:
- create the new "previous state"
- generate the alerts
We're testing the first, but not the latter.
Otherwise, good to go, imho
@thomasneirynck To follow up on a few items discussed offline:
Thanks for the feedback! |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Distributable file count
History
To update your PR or re-run it, just comment with: |
… for non-updated alerts (#85364) (#85573) Co-authored-by: Kibana Machine <[email protected]>
Carries through active status for geo containment alerts that don't receive updates. To test, likely requires the faketracks script with the feature omitting capabilities added in this PR.
To test:
true
Note: I would recommend using a boundary dataset with greater shape density than the one shown, otherwise you might find yourself waiting a while before a non-updated feature falls within a shape