-
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
[Security Solution] Unskip remaining Cypress tests from RAC rules migration #122661
Conversation
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
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! Thanks for addressing these skips and the added details in the description @madirey! 🙂
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.
💯 Thank you for closing the loop, here! It's nice to have these back. 🙌
…ration (elastic#122661) * Unskip indicator match timeline test * Unskip fields_browser tests * Enable alert_summary tests * add cti feed enrichment * Fix accessibility text in indicator match cypress test * Adjust fields_browser test to account for removed field * Correct indicator_match row renderer text in cypress test * Revert "Enable alert_summary tests" This reverts commit 05d549e. (cherry picked from commit 59be3ca)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…ration (#122661) (#122938) * Unskip indicator match timeline test * Unskip fields_browser tests * Enable alert_summary tests * add cti feed enrichment * Fix accessibility text in indicator match cypress test * Adjust fields_browser test to account for removed field * Correct indicator_match row renderer text in cypress test * Revert "Enable alert_summary tests" This reverts commit 05d549e. (cherry picked from commit 59be3ca) Co-authored-by: Madison Caldwell <madison.rey.caldwell@gmail.com>
const filterInput = 'host.geo.c'; | ||
|
||
filterFieldsBrowser(filterInput); | ||
|
||
cy.get(FIELDS_BROWSER_SELECTED_CATEGORY_COUNT).should('have.text', '5'); | ||
cy.get(FIELDS_BROWSER_SELECTED_CATEGORY_COUNT).should('have.text', '4'); |
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.
@madirey this change is not correct, we are still having 5 fields, this test was catching a legitimate bug.
The problem here is that when we are opening the timeline the new alert's index is not selected by default when it should, that index is the one that contains the missing field.
We should fix the issue and change the number of the test to 5 again, I'll open a ticket with the bug and assign to the alerts team since I'm assuming that as the issue was introduced when the rule registry changes were merged is the alerts team the one that should take care of it. Please note that this change impacts one of new explore team functionalities.
ping @spong @rylnd @stephmilovic
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.
@MadameSheema I only see 4 fields in the ECS field mapping. Is there an additional non-ECS one?
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.
https://www.elastic.co/guide/en/ecs/current/ecs-geo.html
As you can see, the test is looking at geo fields that start with c. there are 5
You are only looking at ecs mapping for Data View before it has alerts mappings because alerts index has not been initialized yet. Alerts index has extra field, if i recall correctly geo.continent_code
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.
@stephmilovic @MadameSheema The new alerts mapping doesn't contain the geo.continent_code
field so I believe it will only show up on upgrades (since the field exists in the legacy .siem-signals index, which is aliased). New installs will have only 4 fields. Our team will be updating the ECS mappings soon, which should result in that field being added. We should be able to update the test at that point.
## Summary New ECS FieldMap was generated in #123012, however since it only contained changes to `Rule Registry` code the `Security Solution` Cypress tests were not run, and thus did not catch this field change. See #122661 (comment) for details. Confirmed w/ @madirey that expected value is indeed `5` now that `host.geo.continent_code` has been [added](https://github.com/elastic/kibana/pull/123012/files#diff-a1647ccb73ef26c8c8b6aefd87084504b146af72fcb088ccacad93fcaad15b69R1524-R1528). Some failing PR's from `main`: #123357 #121644 #123352 ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
…123429) ## Summary New ECS FieldMap was generated in elastic#123012, however since it only contained changes to `Rule Registry` code the `Security Solution` Cypress tests were not run, and thus did not catch this field change. See elastic#122661 (comment) for details. Confirmed w/ @madirey that expected value is indeed `5` now that `host.geo.continent_code` has been [added](https://github.com/elastic/kibana/pull/123012/files#diff-a1647ccb73ef26c8c8b6aefd87084504b146af72fcb088ccacad93fcaad15b69R1524-R1528). Some failing PR's from `main`: elastic#123357 elastic#121644 elastic#123352 ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios (cherry picked from commit d6917fc)
…#123433) ## Summary New ECS FieldMap was generated in #123012, however since it only contained changes to `Rule Registry` code the `Security Solution` Cypress tests were not run, and thus did not catch this field change. See #122661 (comment) for details. Confirmed w/ @madirey that expected value is indeed `5` now that `host.geo.continent_code` has been [added](https://github.com/elastic/kibana/pull/123012/files#diff-a1647ccb73ef26c8c8b6aefd87084504b146af72fcb088ccacad93fcaad15b69R1524-R1528). Some failing PR's from `main`: #123357 #121644 #123352 ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios (cherry picked from commit d6917fc) Co-authored-by: Garrett Spong <spong@users.noreply.github.com>
Summary
This PR unskips the remaining timeline-related Cypress tests that were skipped as part of the RAC rules migration.
5
to4
.Checklist
Delete any items that are not applicable to this PR.
For maintainers