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.

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][Rule Management] Getting rid off before hook if is not loading an archive #178891

Merged
merged 6 commits into from
Mar 25, 2024

Conversation

MadameSheema
Copy link
Member

@MadameSheema MadameSheema commented Mar 18, 2024

@MadameSheema
Copy link
Member Author

/ci

@MadameSheema MadameSheema self-assigned this Mar 18, 2024
@MadameSheema MadameSheema added v8.14.0 Team:Detection Rule Management Security Detection Rule Management Team release_note:skip Skip the PR/issue when compiling release notes labels Mar 18, 2024
@MadameSheema MadameSheema marked this pull request as ready for review March 18, 2024 18:02
@MadameSheema MadameSheema requested a review from a team as a code owner March 18, 2024 18:02
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

@MadameSheema MadameSheema enabled auto-merge (squash) March 19, 2024 08:54
@maximpn maximpn self-requested a review March 19, 2024 10:30
Copy link
Contributor

@maximpn maximpn left a comment

Choose a reason for hiding this comment

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

@MadameSheema thank you for addressing Cypress retry issue at Rules Management Area 🙏

The changes look good so impact it a bit unclear. Could you compare execution time of x-pack/test/security_solution_cypress/cypress/e2e/detection_response/rule_management/rules_table/rules_table_auto_refresh.cy.ts before and after the changes? It will give us a clear picture how much overhead we got recreating rules before each test.

Btw, createRule doesn't throw in case of failure since failOnStatusCode is set to false.

@MadameSheema
Copy link
Member Author

@maximpn please find below my responses

Btw, createRule doesn't throw in case of failure since failOnStatusCode is set to false.

Yes, but if there is a problem with the rule creation the test itself is going to fail, in that case, Cypress retry is not going to execute the before hook, just code in beforeEach is reexecuted.

The changes look good so impact it a bit unclear. Could you compare execution time of x-pack/test/security_solution_cypress/cypress/e2e/detection_response/rule_management/rules_table/rules_table_auto_refresh.cy.ts before and after the changes? It will give us a clear picture how much overhead we got recreating rules before each test.

58s before my changes and 1:38s after my changes. Please let me know if that is acceptable or not.

Screenshot 2024-03-20 at 10 15 31 Screenshot 2024-03-20 at 10 59 07

Even this might have impact in the execution time, it will give us more reliability in the future.

@banderror banderror added test test_ui_functional Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Mar 21, 2024
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@maximpn maximpn left a comment

Choose a reason for hiding this comment

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

@MadameSheema thank you for posting performance comparison 👍

I see the changes increase execution time twice (roughly speaking) but we still talk about max a few minutes (CI should be slower than local runs). It looks rather a trade off than an incremental improvement and since it's much more important for us to have Cypress retry functionality working reliably I approve your PR.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Defend Workflows Cypress Tests #4 / Response console Execute operations: "after all" hook for ""execute --command" - should execute a command" "after all" hook for ""execute --command" - should execute a command"
  • [job] [logs] Defend Workflows Cypress Tests #4 / Response console Execute operations: "before all" hook for ""execute --command" - should execute a command" "before all" hook for ""execute --command" - should execute a command"

Metrics [docs]

✅ unchanged

History

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

cc @MadameSheema

@MadameSheema MadameSheema merged commit 6f49ab8 into elastic:main Mar 25, 2024
36 checks passed
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 25, 2024
…s not loading an archive (elastic#178891)

## Summary

Addresses: elastic#175022

Update:
[Flaky test suite runner passed
successfully](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5522)

(cherry picked from commit 6f49ab8)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.13

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

1 similar comment
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine added a commit that referenced this pull request Mar 29, 2024
…ok if is not loading an archive (#178891) (#179333)

# Backport

This will backport the following commits from `main` to `8.13`:
- [[Security Solution][Rule Management] Getting rid off before hook if
is not loading an archive
(#178891)](#178891)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Gloria
Hornero","email":"gloria.hornero@elastic.co"},"sourceCommit":{"committedDate":"2024-03-25T11:04:13Z","message":"[Security
Solution][Rule Management] Getting rid off before hook if is not loading
an archive (#178891)\n\n## Summary\r\n\r\nAddresses:
#175022
test suite runner
passed\r\nsuccessfully](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5522)","sha":"6f49ab85e81fe123338a1a0a61266fb7c67606f9","branchLabelMapping":{"^v8.14.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["test","test_ui_functional","release_note:skip","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","v8.14.0","v8.13.1"],"title":"[Security Solution][Rule
Management] Getting rid off before hook if is not loading an
archive","number":178891,"url":"#178891
Solution][Rule Management] Getting rid off before hook if is not loading
an archive (#178891)\n\n## Summary\r\n\r\nAddresses:
#175022
test suite runner
passed\r\nsuccessfully](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5522)","sha":"6f49ab85e81fe123338a1a0a61266fb7c67606f9"}},"sourceBranch":"main","suggestedTargetBranches":["8.13"],"targetPullRequestStates":[{"branch":"main","label":"v8.14.0","branchLabelMappingKey":"^v8.14.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/178891","number":178891,"mergeCommit":{"message":"[Security
Solution][Rule Management] Getting rid off before hook if is not loading
an archive (#178891)\n\n## Summary\r\n\r\nAddresses:
#175022
test suite runner
passed\r\nsuccessfully](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5522)","sha":"6f49ab85e81fe123338a1a0a61266fb7c67606f9"}},{"branch":"8.13","label":"v8.13.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Gloria Hornero <gloria.hornero@elastic.co>
Co-authored-by: Maxim Palenov <maxim.palenov@elastic.co>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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_ui_functional test v8.13.1 v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Rule Management] Remove before and after hooks from Cypress tests
6 participants