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] Unskip rules table auto-refresh Cypress tests #163451

Merged
merged 2 commits into from
Aug 11, 2023

Conversation

maximpn
Copy link
Contributor

@maximpn maximpn commented Aug 8, 2023

Addresses: #154694

Summary

This PR unskips rules table auto-refresh tests.

Details

Rules table auto-refresh Cypress tests were outdated so it was hard to determine the original flakiness reason. To make it working the following has been done

  • mockGlobalClock() moved above visit() to mock the time before visiting the page. It didn't look like working so the test had to wait for 60 seconds (our refresh interval)
  • removed unnecessary waiting waitForRulesTableToBeLoaded() as the test should work without it
  • removed setRowsPerPageTo(5) as the test doesn't look related to the page size
  • AutoRefreshButtonComponent was refactored in attempt to fix popover closing related flakiness but it doesn't seem to help but the refactoring looks improving readability so leaving as a part of this PR
  • auto-refresh popover's behavior has been changed. The trigger button gets disabled if there is a rule selected. Clicking the disabled button won't make the auto-refresh popover to appear so this part was changed.

Encountered issues

  • EuiPopover should close by clicking the trigger button, clicking outside or pressing Esc button. The latter two cause flakiness for some reason. I spent quite some time to investigate the reason but without success. It looks like there is a race condition related to EuiFocusTrap but it doesn't look to be an issue outside Cypress.
  • waitForRulesTableToBeLoaded() basically does nothing after changes in the rules table. Looking at tis contents
export const waitForRulesTableToBeLoaded = () => {
  // Wait up to 5 minutes for the rules to load as in CI containers this can be very slow
  cy.get(RULES_TABLE_INITIAL_LOADING_INDICATOR, { timeout: 300000 }).should('not.exist');
};

reveals that RULES_TABLE_INITIAL_LOADING_INDICATOR defined as [data-test-subj="initialLoadingPanelAllRulesTable"] doesn't exist anymore in our codebase so this function instantly returns. The selector will be fixed in a separate PR.

  • invoking mockGlobalClock() during the test leads to appearing a lot of Executing a cancelled action error messages.

Flaky test runner

50 runs succeeded

@maximpn maximpn added test skipped-test release_note:skip Skip the PR/issue when compiling release notes impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Rule Management Security Solution Detection Rule Management area Team:Detection Rule Management Security Detection Rule Management Team backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.10.0 labels Aug 8, 2023
@maximpn maximpn self-assigned this Aug 8, 2023
@maximpn maximpn requested review from a team as code owners August 8, 2023 22:34
@maximpn maximpn requested a review from xcrzx August 8, 2023 22:34
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@@ -11,7 +11,7 @@
"cypress:open": "TZ=UTC node ./scripts/start_cypress_parallel open --spec './cypress/e2e/**/*.cy.ts' --config-file ./cypress/cypress.config.ts --ftr-config-file ../../../../../../x-pack/test/security_solution_cypress/cli_config",
"cypress:open:ccs": "yarn cypress:open --config specPattern=./cypress/ccs_e2e/**/*.cy.ts",
"cypress:open:upgrade": "yarn cypress:open --config specPattern=./cypress/upgrade_e2e/**/*.cy.ts",
"cypress:run": "yarn cypress:run:reporter --browser chrome --spec './cypress/e2e/{,!(investigations,explore)/**/}*.cy.ts'; status=$?; yarn junit:merge && exit $status",
"cypress:run": "yarn cypress:run:reporter --browser chrome --spec './cypress/e2e/**/rules_table_auto_refresh.cy.ts'; status=$?; yarn junit:merge && exit $status",
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to undo this change

@maximpn maximpn marked this pull request as draft August 9, 2023 16:07
@maximpn maximpn force-pushed the unskip-rules-table-autorefresh-tests branch from b36d45b to bc18d4b Compare August 10, 2023 09:56
@maximpn maximpn force-pushed the unskip-rules-table-autorefresh-tests branch from bc18d4b to 443a04b Compare August 10, 2023 10:45
@maximpn maximpn removed the request for review from xcrzx August 10, 2023 11:14
@maximpn maximpn marked this pull request as ready for review August 10, 2023 12:33
@maximpn maximpn requested a review from jpdjere August 10, 2023 15:03
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 15.6MB 15.6MB +14.0B

History

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

cc @maximpn

Copy link
Contributor

@jpdjere jpdjere left a comment

Choose a reason for hiding this comment

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

LGTM 👍

And thanks for the refactor of the AutoRefreshButtonComponent component. It sure is more readable now.

@maximpn maximpn merged commit 589ef22 into elastic:main Aug 11, 2023
@maximpn maximpn deleted the unskip-rules-table-autorefresh-tests branch August 11, 2023 12:07
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.9 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 163451

Questions ?

Please refer to the Backport tool documentation

@banderror
Copy link
Contributor

@maximpn @jpdjere ^^^ It looks like backporting fixes of flakiness is going to be difficult because of changes in the tests folder structure. Do you think we could still backport or apply fixes manually to the 8.9 branch, or would it be too much effort?

@jpdjere
Copy link
Contributor

jpdjere commented Aug 11, 2023

@maximpn @banderror Up to now the PRs to fix flakiness have been relatively small, I think we can apply the fixes manually or even cherry-pick the commits, in some cases.

WDYT Maxim?

jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 11, 2023
* main: (64 commits)
  [ML] Transforms: Fix privileges check. (elastic#163687)
  [Log Explorer] Add test suite for Dataset Selector (elastic#163079)
  [Security Solution][Endpoint] Add API checks to Endpoint Policy create/update for checking `endpointPolicyProtections` is enabled (elastic#163429)
  [Security Solution] Fix flaky test: x-pack/test/detection_engine_api_integration/security_and_spaces/update_prebuilt_rules_package/update_prebuilt_rules_package·ts - update_prebuilt_rules_package should allow user to install prebuilt rules from scratch, then install new rules and upgrade existing rules from the new package (elastic#163241)
  [Security Solution] expandable flyout - replace feature flag with advanced settings toggle (elastic#161614)
  [DOCS] Adds the release notes for the 8.9.1 release. (elastic#163578)
  [FTR] Implement browser network condition utils (elastic#163633)
  [Security Solution] Unskip rules table auto-refresh Cypress tests (elastic#163451)
  [Security Solution] Re-enable fixed rule snoozing Cypress test (elastic#160037)
  [Flaky Test elastic#111821] Mock `moment` to avoid midnight TZ issues (elastic#163157)
  Document interactive setup (elastic#163619)
  [Lens] Align decoration color with text color for layer actions (elastic#163630)
  [Lens] Relax counter field checks for saved visualizations with unsupported operations (elastic#163515)
  [Security Solution][Endpoint] Removes pMap and uses a for loop instead (elastic#163509)
  [Enterprise Search] Update Workplace Search connectors doclink (elastic#163676)
  Update APM (main) (elastic#163623)
  [Serverless] Partially fix lens/maps/visualize breadcrumbs missing title  (elastic#163476)
  [Flaky elastic#118272] Unskip tests (elastic#163319)
  [APM] Make service group saved objects exportable (elastic#163569)
  [Observability AI Assistant] Action menu item (elastic#163463)
  ...
maximpn added a commit that referenced this pull request Aug 21, 2023
…ts (#163451) (#164062)

# Backport

This will backport the following commits from `main` to `8.9`:
- [[Security Solution] Unskip rules table auto-refresh Cypress tests
(#163451)](#163451)

<!--- Backport version: 8.9.8 -->

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

<!--BACKPORT [{"author":{"name":"Maxim
Palenov","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-08-11T12:07:35Z","message":"[Security
Solution] Unskip rules table auto-refresh Cypress tests
(#163451)\n\n**Addresses:**
https://github.com/elastic/kibana/issues/154694\r\n\r\n##
Summary\r\n\r\nThis PR unskips rules table auto-refresh tests.\r\n\r\n##
Details\r\n\r\nRules table auto-refresh Cypress tests were outdated so
it was hard to determine the original flakiness reason. To make it
working the following has been done\r\n\r\n- `mockGlobalClock()` moved
above `visit()` to mock the time before visiting the page. It didn't
look like working so the test had to wait for 60 seconds (our refresh
interval)\r\n- removed unnecessary waiting
`waitForRulesTableToBeLoaded()` as the test should work without it\r\n-
removed `setRowsPerPageTo(5)` as the test doesn't look related to the
page size\r\n- `AutoRefreshButtonComponent ` was refactored in attempt
to fix popover closing related flakiness but it doesn't seem to help but
the refactoring looks improving readability so leaving as a part of this
PR\r\n- auto-refresh popover's behavior has been changed. The trigger
button gets disabled if there is a rule selected. Clicking the disabled
button won't make the auto-refresh popover to appear so this part was
changed.\r\n\r\n### Encountered issues\r\n\r\n- `EuiPopover` should
close by clicking the trigger button, clicking outside or pressing `Esc`
button. The latter two cause flakiness for some reason. I spent quite
some time to investigate the reason but without success. It looks like
there is a race condition related to
[EuiFocusTrap](https://github.com/elastic/eui/blob/main/src/components/focus_trap/focus_trap.tsx)
but it doesn't look to be an issue outside Cypress.\r\n-
`waitForRulesTableToBeLoaded()` basically does nothing after changes in
the rules table. Looking at tis contents\r\n\r\n```ts\r\nexport const
waitForRulesTableToBeLoaded = () => {\r\n // Wait up to 5 minutes for
the rules to load as in CI containers this can be very slow\r\n
cy.get(RULES_TABLE_INITIAL_LOADING_INDICATOR, { timeout: 300000
}).should('not.exist');\r\n};\r\n```\r\n\r\nreveals that
`RULES_TABLE_INITIAL_LOADING_INDICATOR` defined as
`[data-test-subj=\"initialLoadingPanelAllRulesTable\"]` doesn't exist
anymore in our codebase so this function instantly returns. The selector
will be fixed in a separate PR.\r\n\r\n- invoking `mockGlobalClock()`
during the test leads to appearing a lot of `Executing a cancelled
action` error messages.\r\n\r\n\r\n### Flaky test runner\r\n\r\n[50 runs
succeeded](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2842)","sha":"589ef228fb9c2525a49d6abc5db64eb98b2c381e","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["test","skipped-test","release_note:skip","impact:high","Team:Detections
and Resp","Team: SecuritySolution","Feature:Rule
Management","Team:Detection Rule
Management","backport:prev-minor","v8.10.0"],"number":163451,"url":"https://github.com/elastic/kibana/pull/163451","mergeCommit":{"message":"[Security
Solution] Unskip rules table auto-refresh Cypress tests
(#163451)\n\n**Addresses:**
https://github.com/elastic/kibana/issues/154694\r\n\r\n##
Summary\r\n\r\nThis PR unskips rules table auto-refresh tests.\r\n\r\n##
Details\r\n\r\nRules table auto-refresh Cypress tests were outdated so
it was hard to determine the original flakiness reason. To make it
working the following has been done\r\n\r\n- `mockGlobalClock()` moved
above `visit()` to mock the time before visiting the page. It didn't
look like working so the test had to wait for 60 seconds (our refresh
interval)\r\n- removed unnecessary waiting
`waitForRulesTableToBeLoaded()` as the test should work without it\r\n-
removed `setRowsPerPageTo(5)` as the test doesn't look related to the
page size\r\n- `AutoRefreshButtonComponent ` was refactored in attempt
to fix popover closing related flakiness but it doesn't seem to help but
the refactoring looks improving readability so leaving as a part of this
PR\r\n- auto-refresh popover's behavior has been changed. The trigger
button gets disabled if there is a rule selected. Clicking the disabled
button won't make the auto-refresh popover to appear so this part was
changed.\r\n\r\n### Encountered issues\r\n\r\n- `EuiPopover` should
close by clicking the trigger button, clicking outside or pressing `Esc`
button. The latter two cause flakiness for some reason. I spent quite
some time to investigate the reason but without success. It looks like
there is a race condition related to
[EuiFocusTrap](https://github.com/elastic/eui/blob/main/src/components/focus_trap/focus_trap.tsx)
but it doesn't look to be an issue outside Cypress.\r\n-
`waitForRulesTableToBeLoaded()` basically does nothing after changes in
the rules table. Looking at tis contents\r\n\r\n```ts\r\nexport const
waitForRulesTableToBeLoaded = () => {\r\n // Wait up to 5 minutes for
the rules to load as in CI containers this can be very slow\r\n
cy.get(RULES_TABLE_INITIAL_LOADING_INDICATOR, { timeout: 300000
}).should('not.exist');\r\n};\r\n```\r\n\r\nreveals that
`RULES_TABLE_INITIAL_LOADING_INDICATOR` defined as
`[data-test-subj=\"initialLoadingPanelAllRulesTable\"]` doesn't exist
anymore in our codebase so this function instantly returns. The selector
will be fixed in a separate PR.\r\n\r\n- invoking `mockGlobalClock()`
during the test leads to appearing a lot of `Executing a cancelled
action` error messages.\r\n\r\n\r\n### Flaky test runner\r\n\r\n[50 runs
succeeded](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2842)","sha":"589ef228fb9c2525a49d6abc5db64eb98b2c381e"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/163451","number":163451,"mergeCommit":{"message":"[Security
Solution] Unskip rules table auto-refresh Cypress tests
(#163451)\n\n**Addresses:**
https://github.com/elastic/kibana/issues/154694\r\n\r\n##
Summary\r\n\r\nThis PR unskips rules table auto-refresh tests.\r\n\r\n##
Details\r\n\r\nRules table auto-refresh Cypress tests were outdated so
it was hard to determine the original flakiness reason. To make it
working the following has been done\r\n\r\n- `mockGlobalClock()` moved
above `visit()` to mock the time before visiting the page. It didn't
look like working so the test had to wait for 60 seconds (our refresh
interval)\r\n- removed unnecessary waiting
`waitForRulesTableToBeLoaded()` as the test should work without it\r\n-
removed `setRowsPerPageTo(5)` as the test doesn't look related to the
page size\r\n- `AutoRefreshButtonComponent ` was refactored in attempt
to fix popover closing related flakiness but it doesn't seem to help but
the refactoring looks improving readability so leaving as a part of this
PR\r\n- auto-refresh popover's behavior has been changed. The trigger
button gets disabled if there is a rule selected. Clicking the disabled
button won't make the auto-refresh popover to appear so this part was
changed.\r\n\r\n### Encountered issues\r\n\r\n- `EuiPopover` should
close by clicking the trigger button, clicking outside or pressing `Esc`
button. The latter two cause flakiness for some reason. I spent quite
some time to investigate the reason but without success. It looks like
there is a race condition related to
[EuiFocusTrap](https://github.com/elastic/eui/blob/main/src/components/focus_trap/focus_trap.tsx)
but it doesn't look to be an issue outside Cypress.\r\n-
`waitForRulesTableToBeLoaded()` basically does nothing after changes in
the rules table. Looking at tis contents\r\n\r\n```ts\r\nexport const
waitForRulesTableToBeLoaded = () => {\r\n // Wait up to 5 minutes for
the rules to load as in CI containers this can be very slow\r\n
cy.get(RULES_TABLE_INITIAL_LOADING_INDICATOR, { timeout: 300000
}).should('not.exist');\r\n};\r\n```\r\n\r\nreveals that
`RULES_TABLE_INITIAL_LOADING_INDICATOR` defined as
`[data-test-subj=\"initialLoadingPanelAllRulesTable\"]` doesn't exist
anymore in our codebase so this function instantly returns. The selector
will be fixed in a separate PR.\r\n\r\n- invoking `mockGlobalClock()`
during the test leads to appearing a lot of `Executing a cancelled
action` error messages.\r\n\r\n\r\n### Flaky test runner\r\n\r\n[50 runs
succeeded](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2842)","sha":"589ef228fb9c2525a49d6abc5db64eb98b2c381e"}}]}]
BACKPORT-->

Co-authored-by: Kibana Machine <[email protected]>
@jpdjere jpdjere added the flake-docs Temp label to gather PRs used to create dev docs label Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Rule Management Security Solution Detection Rule Management area flake-docs Temp label to gather PRs used to create dev docs impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. release_note:skip Skip the PR/issue when compiling release notes skipped-test 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.9.2 v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants