Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Security Solution] Unskip Serverless Threshold rule creation Cypress test #167083

Merged
merged 5 commits into from
Oct 4, 2023
Merged

[Security Solution] Unskip Serverless Threshold rule creation Cypress test #167083

merged 5 commits into from
Oct 4, 2023

Conversation

maximpn
Copy link
Contributor

@maximpn maximpn commented Sep 22, 2023 •
edited
Loading

Addresses: #161539

Summary

This PR unskips Threshold rule creation Cypress tests.

Details

To unskip the test with minimal changes it required to refactor navigation via breadcrumbs and update rules table breadcrumb selectors to make navigation back to the rules table work seamlessly between ESS and Serverless.

Flaky Test runner

Serverless threshold_rule.cy.ts 150 runs 🟢

@maximpn maximpn added test Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team Serverless labels Sep 22, 2023
@maximpn maximpn self-assigned this Sep 22, 2023
@maximpn maximpn added release_note:skip Skip the PR/issue when compiling release notes v8.11.0 labels Sep 22, 2023
@maximpn maximpn force-pushed the unskip-serverless-threshold-rule-creation-cypress-test branch 2 times, most recently from 37a8444 to 71db29d Compare September 24, 2023 02:53
@maximpn maximpn marked this pull request as ready for review September 25, 2023 15:37
@maximpn maximpn requested a review from a team as a code owner September 25, 2023 15:37
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@maximpn
Copy link
Contributor Author

maximpn commented Sep 25, 2023

@elasticmachine merge upstream

banderror
banderror reviewed Sep 28, 2023
View reviewed changes
Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Thank you @maximpn, reviewed the diff and left some (mostly minor) comments!

...est/security_solution_cypress/cypress/e2e/detection_response/rule_actions/rule_actions.cy.ts Outdated Show resolved Hide resolved
...ution_cypress/cypress/e2e/detection_response/rule_creation/custom_query_rule_data_view.cy.ts Outdated Show resolved Hide resolved
..._solution_cypress/cypress/e2e/detection_response/rule_creation/custom_saved_query_rule.cy.ts Outdated Show resolved Hide resolved
...y_solution_cypress/cypress/e2e/detection_response/rule_creation/event_correlation_rule.cy.ts Outdated Show resolved Hide resolved
x-pack/test/security_solution_cypress/cypress/screens/breadcrumbs.ts Outdated
Comment on lines 8 to 9
export const RULES_TABLE_BREADCRUMB =
'[data-test-subj~="breadcrumb"][title="Detection rules (SIEM)"]';
Copy link
Contributor

Choose a reason for hiding this comment

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

(Some general thoughts) Breadcrumbs tend to change pretty often. Sometimes their structure changes (the tree), sometimes they are renamed. I think they changed at least twice during the last two releases.

So when SPA navigation is important for testing certain scenarios (such as making sure that the Rules table gets refreshed after some change), I think it might be OK-ish to use breadcrumbs for doing it. Although it would be great to use more stable UI elements (when they exist) or ways for doing SPA navigation. In theory, one way could be using the browser History API (.pushState() and .back() methods) directly from tests. There's an article with a short example: https://glebbahmutov.com/blog/cypress-history-api-example/

When SPA navigation is not required for testing something specific, we should be just opening pages by their URLs.

x-pack/test/security_solution_cypress/cypress/screens/breadcrumbs.ts Outdated Show resolved Hide resolved
x-pack/test/security_solution_cypress/cypress/screens/breadcrumbs.ts Outdated Show resolved Hide resolved
x-pack/test/security_solution_cypress/cypress/screens/breadcrumbs.ts Outdated Show resolved Hide resolved
x-pack/test/security_solution_cypress/cypress/tasks/navigation.ts Outdated Show resolved Hide resolved
x-pack/test/security_solution_cypress/cypress/tasks/navigation.ts Outdated Show resolved Hide resolved
@banderror
Copy link
Contributor

@elasticmachine merge upstream

@maximpn maximpn force-pushed the unskip-serverless-threshold-rule-creation-cypress-test branch from 1254fc2 to b1acbdb Compare October 1, 2023 23:34
@maximpn maximpn force-pushed the unskip-serverless-threshold-rule-creation-cypress-test branch from b1acbdb to e1da76d Compare October 3, 2023 02:39
@maximpn maximpn requested a review from banderror October 3, 2023 04:58
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Investigations - Security Solution Cypress Tests #2 / Discover Timeline State Integration saved search should rename the saved search on timeline rename should rename the saved search on timeline rename
  • [job] [logs] FTR Configs #54 / Serverless Common UI - Examples Field formats example "before all" hook for "renders field formats example 1"

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @maximpn

banderror
banderror approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@maximpn maximpn merged commit 3ea2f23 into elastic:main Oct 4, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 4, 2023
@maximpn maximpn deleted the unskip-serverless-threshold-rule-creation-cypress-test branch October 4, 2023 08:37
@watson watson added the Project:Serverless Work as part of the Serverless project for its initial release label Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@banderror banderror banderror approved these changes

@yctercero yctercero yctercero approved these changes

Assignees

@maximpn maximpn

Labels
backport:skip This commit does not require backporting Project:Serverless Work as part of the Serverless project for its initial release release_note:skip Skip the PR/issue when compiling release notes Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. test v8.11.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@maximpn @elasticmachine @banderror @kibana-ci @yctercero @watson @kibanamachine