-
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
[Event Log] Ensure sorting tests are less flaky #64781
Conversation
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
x-pack/test/plugin_api_integration/test_suites/event_log/public_api_integration.ts
Outdated
Show resolved
Hide resolved
FYI, I just skipped this suite so please make sure to unskip the tests in this PR. |
Also, not sure if this addresses #64812 too |
It does not, but the same fix here, to that code, will presumably fix that issue as well. Good to see that's all we needed to fix this! |
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.
LGTM, needs same fix in the test above it though, per Spencer's comment ...
* master: (42 commits) [Ingest] Allow aggent to send metadata compliant with ECS (elastic#64452) [Endpoint] Remove todos, urls to issues (elastic#64833) [Uptime] Remove hard coded value for monitor states histograms (elastic#64396) Feature/send feedback link (elastic#64845) [ML] Moving get filters capability to admin (elastic#64879) Remove edit alert button from alerts list (elastic#64643) [EPM] Handle constant_keyword type in KB index patterns and ES index templates (elastic#64876) [ML] Disable data frame anaylics clone button based on permission (elastic#64830) Dashboard url generator to preserve saved filters from destination dashboard (elastic#64767) add generic typings for SavedObjectMigrationFn (elastic#63943) Allow to define and update a defaultPath for applications (elastic#64498) [Event Log] add rel=primary to saved objects for query targets (elastic#64615) [Lens] Use a size of 5 for first string field in visualization (elastic#64726) [SIEM][Lists] Removes plugin dependencies, adds more unit tests, fixes more TypeScript types [Ingest] Edit datasource UI (elastic#64727) [Lens] Bind all time fields to the time picker (elastic#63874) [Lens] Use suggestion system in chart switcher for subtypes (elastic#64613) Improve alpha messaging (elastic#64692) [Ingest] Allow to enable monitoring of elastic agent (elastic#63598) [Metrics UI] Fix alerting when a filter query is present (elastic#64575) ...
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.
LGTM
@@ -42,6 +42,9 @@ export const logEventRoute = (router: IRouter, eventLogger: IEventLogger, logger | |||
await context.core.savedObjects.client.create('event_log_test', {}, { id }); | |||
logger.info(`created saved object ${id}`); | |||
} | |||
// mark now as start and end |
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.
The only slightly odd thing here is that for the test where we search by end date, all of these events will have an end date - however, IRL, not all events have end dates. We may need to do something in the future - like when searching via end date, add an additional filter to only return events with an end date - but let's wait to see how people use this, to see if something needs to be done.
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.
Yeah, agreed, though this is sort by end rather than search by end.. semantics but it means that the behaviour is inline with how ES queries generally behave.
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.
LGTM
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Creating events in parallel may be causing a slight flakyness, this change staggers creation to ensure this doesn't happen. In addition it turned out the `event.end` field was missing in certain cases, causing the test that sorts by `end` to fail.
…ana into alerting/np-tests-migration * 'alerting/np-tests-migration' of github.com:gmmorris/kibana: [APM] Agent remote config: validation for Java agent configs (elastic#63956) [APM] Fix duplicate index patterns (elastic#64883) Drilldowns (elastic#61219) [Alerting] fix labels and links in PagerDuty action ui and docs (elastic#64032) [Event Log] Ensure sorting tests are less flaky (elastic#64781) update endpoint to restrict removing with datasources (elastic#64978) [Logs UI] [Alerting] Alerts management page enhancements (elastic#64654) Adjust kibana app owning files (elastic#65064) Migrate tutorial resources (elastic#64298) [Logs UI] Tweak copy in log alerts dialog (elastic#64645) [Logs UI] [Alerting] Documentation (elastic#64886) [Logs UI] Add dataset filter to ML module setup screen (elastic#64470) [TSVB] Fixing memory leak (elastic#64918) Bump backport to 5.4.1 (elastic#65041)
Creating events in parallel may be causing a slight flakyness, this change staggers creation to ensure this doesn't happen. In addition it turned out the `event.end` field was missing in certain cases, causing the test that sorts by `end` to fail. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Summary
Creating events in parallel may be causing a slight flakyness, this change staggers creation to ensure this doesn't happen.
In addition it turned out the
event.end
field was missing in certain cases, causing the test that sorts byend
to fail.closes #64723 and #64812
Checklist
Delete any items that are not applicable to this PR.
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsThis was checked for keyboard-only and screenreader accessibilityThis renders correctly on smaller devices using a responsive layout. (You can test this in your browserThis was checked for cross-browser compatibility, including a check against IE11For maintainers