[Security Solution] Fix some Prebuilt Rules Cypress tests not running in CI #191978

nikitaindik · 2024-09-03T12:55:23Z

Resolves: #192256

Summary

This PR re-enables two Cypress test files that didn't run on CI: update_workflow.cy.ts and prebuilt_rules_preview.cy.ts. It also fixes failing tests in prebuilt_rules_preview.cy.ts.

Changes

Renamed update_workflow.ts -> update_workflow.cy.ts. It didn't run on CI because it wasn't picked up by a glob here.
prebuilt_rules_preview.cy.ts:
- Moved { tags: ['@ess', '@serverless'] } to the top-level describe block instead of having it in a variable that is used in every describe. Apparently the tool we use to parse tags doesn't recognize tags in variables anymore, so this test didn't run in either ESS or Serverless pipelines.
- Removed describe('All environments' ... wrappers since they don't add any value anymore. Didn't remove any actual tests.
- Reverted a change from this PR that added a backdrop to the modal which doesn't allow user to switch rules without closing the modal. We have a test that checks that such switching is possible and this test started to fail once I reactivated the test file.
- Fixed selectors that grab filters in the Overview tab. The old ones stopped working. Probably because of a change to the filters component that is built by another team.

Correct behaviour: Switching between rules with flyout open

Schermopname.2024-09-05.om.16.54.44.mov

… the outer describe

elasticmachine · 2024-09-05T15:30:59Z

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

elasticmachine · 2024-09-05T15:31:00Z

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

elasticmachine · 2024-09-05T15:31:01Z

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

alexwizp · 2024-09-06T06:14:27Z

...tion/public/detection_engine/rule_management/components/rule_details/rule_details_flyout.tsx

@@ -196,7 +196,7 @@ export const RuleDetailsFlyout = ({
      paddingSize="l"
      data-test-subj={dataTestSubj}
      aria-labelledby={prebuiltRulesFlyoutTitleId}
-      ownFocus


I am not sure that this is the correct change. This table is partially (or completely) covered by the open Flyout, which means, from an a11y perspective, we should not allow setting focus on it.

Let's ask EUI. @cee-chen / @1Copenut please give us your input.

I see the importance of the functionality for quick preview, but also I think we need to follow a consistent style for when it is appropriate to use ownFocus and when it is not.

@nikitaindik I have the same concern. It's better to follow current UX across Kibana. I'm pretty sure the test you mentioned was written some time ago when the flyout didn't own focus by default.

EuiFlyout does set ownFocus={true} by default, so unless this component now explicitly sets ownFocus={false} this component should be behaving the same way as before.

Ah sorry disregard my previous unhelpful comment, I was looking only at the line highlighted and not at the full diff, I see this is setting ownFocus={false}.

ownFocus={false} flyouts still move/trap focus, so the flyout content still remains accessible for keyboard users, although the content below it does not: https://eui.elastic.co/#/layout/flyout#without-ownfocus

Alexey, does the above documentation help explain ownFocus usage?

@cee-chen Yep, that's exactly what I mean. That case doesn't seem like something where we should set ownFocus={false}. The default behavior will provide the best UX

Thanks for your input folks! I have tried both options myself and found that when tabbing using the keyboard the focus seems to behave the same. With either ownFocus={false} or ownFocus={true} it stays within the flyout and top page menu. Please tell me if I misunderstood your concern.

Also I have discussed this with our PO (@approksiu) and design team today, showed them both options - with and without the backdrop. And we have concluded that for our use case (quickly switching between rules) it would be desirable if there's no backdrop and table items are still clickable with a mouse.

I certainly support the unification of UX, but this is your decision. Please keep in mind that this will lead to the following issues:

When switching between table elements, the focus will not move to the EuiFlyout as expected. In fact, the focus goes to the element under EuiFlyout, which makes keyboard navigation very problematic.

There may be incorrect behaviour of the main content scrolling when using keyboard navigation inside the EuiFlyout. See same problem we have here fix: [Obs Infrastructure > Hosts][KEYBOARD]: Host flyout has buggy scrolling when I expand the State dropdown menu #192372 . I've tested for your case and issue is here.

I'm not insisting, I just want to highlight and explain why we removed ownFocus={false}

@approksiu just fyi

Thanks for highlighting these 2 issues @alexwizp! I have discussed these with the team and we've decided to revert to the default behaviour (with the dark backdrop). Made the change in this commit: d1cadfa

kibana-ci · 2024-09-13T10:28:05Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 60e5779

Failed CI Steps

Metrics [docs]

✅ unchanged

History

💔 Build #233824 failed 167e24f5dbc861c975c447515bbbdfc4406678b6
💔 Build #233686 failed 4f91329
💔 Build #233330 failed a7dfa6a
💛 Build #233251 was flaky 477898f

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

cc @nikitaindik

maximpn

LGTM

@ess

… in CI (elastic#191978) **Resolves: elastic#192256 ## Summary This PR re-enables two Cypress test files that didn't run on CI: `update_workflow.cy.ts` and `prebuilt_rules_preview.cy.ts`. It also fixes failing tests in `prebuilt_rules_preview.cy.ts`. ### Changes - Renamed `update_workflow.ts` -> `update_workflow.cy.ts`. It didn't run on CI because it wasn't picked up by a glob [here](https://github.com/elastic/kibana/blob/main/x-pack/test/security_solution_cypress/package.json#L14). - `prebuilt_rules_preview.cy.ts`: - Moved `{ tags: ['@ess', '@serverless'] }` to the top-level `describe` block instead of having it in a variable that is used in every `describe`. Apparently the tool we use to parse tags doesn't recognize tags in variables anymore, so this test didn't run in either ESS or Serverless pipelines. - Removed `describe('All environments' ... ` wrappers since they don't add any value anymore. Didn't remove any actual tests. - Reverted a change from this [PR](elastic#181427) that added a backdrop to the modal which doesn't allow user to switch rules without closing the modal. We have a [test](https://github.com/elastic/kibana/blob/main/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/rule_management/prebuilt_rules/prebuilt_rules_preview.cy.ts#L1182) that checks that such switching is possible and this test started to fail once I reactivated the test file. - Fixed selectors that grab filters in the Overview tab. The old ones stopped working. Probably because of a change to the filters component that is built by another team. #### Correct behaviour: Switching between rules with flyout open https://github.com/user-attachments/assets/da4a0902-657c-45fe-adc1-eb44ad0de798 (cherry picked from commit c65c2ae)

@ess

… in CI (elastic#191978) **Resolves: elastic#192256 ## Summary This PR re-enables two Cypress test files that didn't run on CI: `update_workflow.cy.ts` and `prebuilt_rules_preview.cy.ts`. It also fixes failing tests in `prebuilt_rules_preview.cy.ts`. ### Changes - Renamed `update_workflow.ts` -> `update_workflow.cy.ts`. It didn't run on CI because it wasn't picked up by a glob [here](https://github.com/elastic/kibana/blob/main/x-pack/test/security_solution_cypress/package.json#L14). - `prebuilt_rules_preview.cy.ts`: - Moved `{ tags: ['@ess', '@serverless'] }` to the top-level `describe` block instead of having it in a variable that is used in every `describe`. Apparently the tool we use to parse tags doesn't recognize tags in variables anymore, so this test didn't run in either ESS or Serverless pipelines. - Removed `describe('All environments' ... ` wrappers since they don't add any value anymore. Didn't remove any actual tests. - Reverted a change from this [PR](elastic#181427) that added a backdrop to the modal which doesn't allow user to switch rules without closing the modal. We have a [test](https://github.com/elastic/kibana/blob/main/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/rule_management/prebuilt_rules/prebuilt_rules_preview.cy.ts#L1182) that checks that such switching is possible and this test started to fail once I reactivated the test file. - Fixed selectors that grab filters in the Overview tab. The old ones stopped working. Probably because of a change to the filters component that is built by another team. #### Correct behaviour: Switching between rules with flyout open https://github.com/user-attachments/assets/da4a0902-657c-45fe-adc1-eb44ad0de798 (cherry picked from commit c65c2ae)

kibanamachine · 2024-09-13T12:05:54Z

💚 All backports created successfully

Status	Branch	Result
✅	8.15
✅	8.x

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

Manual backport

To create the backport manually run:

node scripts/backport --pr 191978

Questions ?

Please refer to the Backport tool documentation

@ess

… in CI (elastic#191978) **Resolves: elastic#192256 ## Summary This PR re-enables two Cypress test files that didn't run on CI: `update_workflow.cy.ts` and `prebuilt_rules_preview.cy.ts`. It also fixes failing tests in `prebuilt_rules_preview.cy.ts`. ### Changes - Renamed `update_workflow.ts` -> `update_workflow.cy.ts`. It didn't run on CI because it wasn't picked up by a glob [here](https://github.com/elastic/kibana/blob/main/x-pack/test/security_solution_cypress/package.json#L14). - `prebuilt_rules_preview.cy.ts`: - Moved `{ tags: ['@ess', '@serverless'] }` to the top-level `describe` block instead of having it in a variable that is used in every `describe`. Apparently the tool we use to parse tags doesn't recognize tags in variables anymore, so this test didn't run in either ESS or Serverless pipelines. - Removed `describe('All environments' ... ` wrappers since they don't add any value anymore. Didn't remove any actual tests. - Reverted a change from this [PR](elastic#181427) that added a backdrop to the modal which doesn't allow user to switch rules without closing the modal. We have a [test](https://github.com/elastic/kibana/blob/main/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/rule_management/prebuilt_rules/prebuilt_rules_preview.cy.ts#L1182) that checks that such switching is possible and this test started to fail once I reactivated the test file. - Fixed selectors that grab filters in the Overview tab. The old ones stopped working. Probably because of a change to the filters component that is built by another team. #### Correct behaviour: Switching between rules with flyout open https://github.com/user-attachments/assets/da4a0902-657c-45fe-adc1-eb44ad0de798 (cherry picked from commit c65c2ae)

@ess

…running in CI (#191978) (#192852) # Backport This will backport the following commits from `main` to `8.15`: - [[Security Solution] Fix some Prebuilt Rules Cypress tests not running in CI (#191978)](#191978)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Nikita Indik <nikita.indik@elastic.co>

@ess

…unning in CI (#191978) (#192853) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution] Fix some Prebuilt Rules Cypress tests not running in CI (#191978)](#191978)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Nikita Indik <nikita.indik@elastic.co>

nikitaindik added the release_note:skip Skip the PR/issue when compiling release notes label Sep 3, 2024

nikitaindik self-assigned this Sep 3, 2024

nikitaindik force-pushed the cypress-tests-not-running-on-ci branch from ba3a284 to b146546 Compare September 5, 2024 10:10

nikitaindik added 4 commits September 5, 2024 16:48

Rename update_workflow.ts -> update_workflow.cy.ts

e7f69bf

prebuilt_rules_preview.cy.ts: move @ess and @serverless tags to…

91a7f0e

… the outer describe

Update element selectors to fix tests in prebuilt_rules_preview.ts

08c8ee8

Remove flyout backdrop to let the user switch between table items

fb10273

nikitaindik force-pushed the cypress-tests-not-running-on-ci branch from b146546 to fb10273 Compare September 5, 2024 14:48

nikitaindik mentioned this pull request Sep 5, 2024

fix: [Rules > Add Elastic rules][SCREEN READER]: Rule non-modal flyout must announce itself on render and manage focus #181427

Merged

nikitaindik added 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 v8.16.0 labels Sep 5, 2024

nikitaindik marked this pull request as ready for review September 5, 2024 15:30

nikitaindik requested a review from a team as a code owner September 5, 2024 15:30

nikitaindik requested a review from maximpn September 5, 2024 15:30

alexwizp reviewed Sep 6, 2024

View reviewed changes

This was referenced Sep 6, 2024

[Security Solution] Some Prebuilt Rules Cypress tests don't run in CI #192256

Closed

[Security Solution] Extend upgrade prebuilt rules context with conflict resolution functionality #191721

Merged

alexwizp mentioned this pull request Sep 10, 2024

fix: [Obs Infrastructure > Hosts][KEYBOARD]: Host flyout has buggy scrolling when I expand the State dropdown menu #192372

Merged

Merge branch 'main' into cypress-tests-not-running-on-ci

477898f

nikitaindik marked this pull request as draft September 10, 2024 09:24

nikitaindik marked this pull request as ready for review September 10, 2024 20:57

nikitaindik added 3 commits September 11, 2024 10:39

Merge branch 'main' into cypress-tests-not-running-on-ci

a7dfa6a

Merge branch 'main' into cypress-tests-not-running-on-ci

4f91329

Revert the flyout backdrop change, remove the related test

d1cadfa

nikitaindik force-pushed the cypress-tests-not-running-on-ci branch from 167e24f to d1cadfa Compare September 13, 2024 09:24

Merge branch 'main' into cypress-tests-not-running-on-ci

60e5779

nikitaindik added the backport:prev-major Backport to (8.x, 8.17, 8.16, 8.15) the previous major branch and other branches in development label Sep 13, 2024

maximpn approved these changes Sep 13, 2024

View reviewed changes

nikitaindik merged commit c65c2ae into elastic:main Sep 13, 2024
48 of 49 checks passed

kibanamachine added the v9.0.0 label Sep 13, 2024

kibanamachine mentioned this pull request Sep 13, 2024

[8.15] [Security Solution] Fix some Prebuilt Rules Cypress tests not running in CI (#191978) #192852

Merged

kibanamachine mentioned this pull request Sep 13, 2024

[8.x] [Security Solution] Fix some Prebuilt Rules Cypress tests not running in CI (#191978) #192853

Merged

nikitaindik mentioned this pull request Sep 13, 2024

[8.x] [Security Solution] Fix some Prebuilt Rules Cypress tests not running in CI (#191978) #192855

Closed

nikitaindik added the v8.15.2 label Sep 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Fix some Prebuilt Rules Cypress tests not running in CI #191978

[Security Solution] Fix some Prebuilt Rules Cypress tests not running in CI #191978

nikitaindik commented Sep 3, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Sep 5, 2024

elasticmachine commented Sep 5, 2024

elasticmachine commented Sep 5, 2024

alexwizp Sep 6, 2024 •

edited

Loading

alexwizp Sep 6, 2024

maximpn Sep 6, 2024

cee-chen Sep 6, 2024

cee-chen Sep 6, 2024

alexwizp Sep 9, 2024

nikitaindik Sep 10, 2024

alexwizp Sep 11, 2024 •

edited

Loading

nikitaindik Sep 13, 2024

kibana-ci commented Sep 13, 2024 •

edited

Loading

maximpn left a comment

kibanamachine commented Sep 13, 2024 •

edited by nikitaindik

Loading

[Security Solution] Fix some Prebuilt Rules Cypress tests not running in CI #191978

[Security Solution] Fix some Prebuilt Rules Cypress tests not running in CI #191978

Conversation

nikitaindik commented Sep 3, 2024 • edited by kibanamachine Loading

Summary

Changes

Correct behaviour: Switching between rules with flyout open

elasticmachine commented Sep 5, 2024

elasticmachine commented Sep 5, 2024

elasticmachine commented Sep 5, 2024

alexwizp Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

alexwizp Sep 6, 2024

Choose a reason for hiding this comment

maximpn Sep 6, 2024

Choose a reason for hiding this comment

cee-chen Sep 6, 2024

Choose a reason for hiding this comment

cee-chen Sep 6, 2024

Choose a reason for hiding this comment

alexwizp Sep 9, 2024

Choose a reason for hiding this comment

nikitaindik Sep 10, 2024

Choose a reason for hiding this comment

alexwizp Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

nikitaindik Sep 13, 2024

Choose a reason for hiding this comment

kibana-ci commented Sep 13, 2024 • edited Loading

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

History

maximpn left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 13, 2024 • edited by nikitaindik Loading

💚 All backports created successfully

Manual backport

Questions ?

nikitaindik commented Sep 3, 2024 •

edited by kibanamachine

Loading

alexwizp Sep 6, 2024 •

edited

Loading

alexwizp Sep 11, 2024 •

edited

Loading

kibana-ci commented Sep 13, 2024 •

edited

Loading

kibanamachine commented Sep 13, 2024 •

edited by nikitaindik

Loading