[Security Solution] Get rid of `goToRuleDetails()` in favor of `goToRuleDetailsOf()` #165011

maximpn · 2023-08-28T16:23:59Z

Addresses: #164513

Summary

This PR gets rid of Rules Management goToRuleDetails() Cypress helper replacing it with goToRuleDetailsOf().

Details

The following has been done

goToRuleDetails() replaced with goToRuleDetailsOf() where it makes sense (rule creation, scenarios validating something on the rules management table and then on the rule details page and etc).
For quite a number of tests expectations are done on the rule details page but it's opened via rules management table page. In fact this unnecessary step may be flaky as rules table has auto resfresh feature and it should be ideally disabled as well as this requires loading of lazy UI resources and increases test's execution time. It can be avoided by directly opening the details page via
```
createRule(getNewRule()).then((rule) =>
  visitWithoutDateRange(ruleDetailsUrl(rule.body.id))
);
```
goToRuleDetails() was removed from such tests in favor of opening the rule details page directly.

Does opening rule detail page directly make the tests less stable?

Yes, it can give such side effect but it will help to either find and fix some bugs in our code or make our tests more robust by adding an universal page ready expectation.

While working on this PR I had to fix a few problems on the rule details page caused by getting rid of unnecessary opening rule details page via rules management table but it fact it revealed rule details page's problems

If a test needs first thing to enable a rule like editing test for custom query rule the test may fail as -1 is sent as rule id. In fact it's a page loading problem. Rule switch become available while the rule isn't loaded yet and it can be easily reproduced in prod if the connection is slow. Disabling rule switch while rule isn't loaded yet solves the problem.
Rule exceptions tests turned to be flaky. Edit exception list item context menu closes on its own sometimes. In fact useEffect causing HTTP request fired much more often than it should due to exception list types defined in place and regenerated each render causing the list item to be in loading state while the request is in progress. It was fixed by moving the arrays to stable constants.

Flaky test runner

Security Solution Cypress tests (150 runs) (there is no indication of negative impact)

elasticmachine · 2023-08-29T10:28:47Z

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

elasticmachine · 2023-08-29T10:28:48Z

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

jpdjere · 2023-09-01T15:06:55Z

...ugins/security_solution/public/detection_engine/rule_details_ui/pages/rule_details/index.tsx

@@ -603,6 +610,7 @@ const RuleDetailsPageComponent: React.FC<DetectionEngineComponentProps> = ({
                        <RuleSwitch
                          id={rule?.id ?? '-1'}
                          isDisabled={
+                            !rule?.id ||


Great catch! :+1

jpdjere · 2023-09-01T15:16:10Z

...curity_solution_cypress/cypress/e2e/detection_response/rule_creation/custom_query_rule.cy.ts

@@ -316,7 +318,7 @@ describe('Custom query rules', { tags: ['@ess', '@brokenInServerless'] }, () =>
          const initialNumberOfRules = rules.length;
          const expectedNumberOfRulesAfterDeletion = initialNumberOfRules - 1;

-          goToTheRuleDetailsOf('New Rule Test');
+          goToRuleDetailsOf('New Rule Test');


Can we create a variable with the test details and use RULE.name here?

jpdjere · 2023-09-01T15:22:31Z

...ity_solution_cypress/cypress/e2e/detection_response/rule_creation/indicator_match_rule.cy.ts

+          duplicateFirstRule();
+          goBackToRuleDetails();
+          goBackToRulesTable();
+          checkDuplicatedRule('Indicator rule duplicate test [Duplicate]');


Same here, can we do somthing like:

checkDuplicatedRule(`${RULE_NAME} [Duplicate]`);

jpdjere · 2023-09-01T15:24:17Z

x-pack/test/security_solution_cypress/cypress/e2e/exceptions/entry/flyout_validation.cy.ts

-    login();
-    visitWithoutDateRange(DETECTIONS_RULE_MANAGEMENT_URL);
-    goToRuleDetails();
-    cy.get(RULE_STATUS).should('have.text', '—');


This test is skipped anyway, but are we sure this assertion:

cy.get(RULE_STATUS).should('have.text', '—');

Is not important and safe to be removed?

It's hard to say as the test is skipped. I'd say it waits for the page to be loaded and we ideally need to incapsulate it in a helper function but I revert this line just in case.

jpdjere · 2023-09-01T15:26:51Z

x-pack/test/security_solution_cypress/cypress/tasks/rule_details.ts

@@ -106,8 +106,8 @@ export const goToEndpointExceptionsTab = () => {
 };

 export const openEditException = (index = 0) => {
-  cy.get(EXCEPTION_ITEM_ACTIONS_BUTTON).eq(index).click({ force: true });
-  cy.get(EDIT_EXCEPTION_BTN).eq(index).click({ force: true });
+  cy.get(EXCEPTION_ITEM_ACTIONS_BUTTON).eq(index).click();


Great to see we don't need this force anymore

jpdjere

Rules Management LGTM! And thanks for this refactor, super valuable; great fixes.

e40pud

DE changes LGTM, left a few comments

e40pud · 2023-09-05T09:38:29Z

...ugins/security_solution/public/detection_engine/rule_details_ui/pages/rule_details/index.tsx

@@ -603,6 +610,7 @@ const RuleDetailsPageComponent: React.FC<DetectionEngineComponentProps> = ({
                        <RuleSwitch
                          id={rule?.id ?? '-1'}
                          isDisabled={
+                            !rule ||


Maybe we could show some loading state instead of the rule's details UI while rule is not available?

Global loader is considered as bad UX nowadays and skeleton loading is much more preferable. We display as much as we can as early as possible to avoid the loader flickering. If you still think it's worth to show some loader instead I suggest to discuss it out of scope of this PR.

e40pud · 2023-09-05T09:51:58Z

x-pack/test/security_solution_cypress/cypress/tasks/alerts_detection_rules.ts

-};
-
-export const goToTheRuleDetailsOf = (ruleName: string) => {
+export const goToRuleDetailsOf = (ruleName: string) => {
  cy.contains(RULE_NAME, ruleName).click();


Just curious how this will work if we have several rules with exactly same name or if we have two rules named "Test Rule" and "Test Rule 1" and we go to details of the rule "Test Rule". Won't it potentially find another rule in those cases?

Just trying to understand if we can safely use same names in tests

@e40pud you are right about the pitfalls. If goToRuleDetailsOf() is used the rule names should be unique and distinguishable. If there are Test Rule and Test Rule 1 rules then goToRuleDetailsOf('Test Rule') opens the first rule in the table. So it has to be take into account.

In the case of exactly same names goToRuleDetailsOf() shouldn't be used. it rather resembles how users distinguish rules. Despite id identifies rules uniquely it takes much more effort than just compare names so obviously user expect unique rule names.

e40pud · 2023-09-05T09:54:48Z

...curity_solution_cypress/cypress/e2e/detection_response/rule_creation/custom_query_rule.cy.ts

-      it('Only modifies rule active status on enable/disable', () => {
-        enablesRule();
+      const editedRuleData = getEditedRule();
+      const expectedEditedtags = editedRuleData.tags?.join('');


We could rename expectedEditedtags into expectedEditedTags to follow the rest of the names

kibana-ci · 2023-09-07T19:48:11Z

💔 Build Failed

Failed CI Steps

Serverless Observability Tests

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`	12.6MB	12.6MB	+17.0B

History

💔 Build #157364 failed f7a8dbddb91853716404f8e5f77799433e9e3009
💔 Build #157345 failed 60eadb27f269c2299e3c0b1e6ac6026d819b00ff
💔 Build #157052 failed db04d4614e9cb9d1deb1e8160802c69689c3475a
💔 Build #157039 failed bfb695fe7bd837d2356657ec68366d664ae16f5c
💔 Build #157008 failed f52278441bcaff1879dfc31f0c4b470a9b388cd8

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

cc @maximpn

maximpn self-assigned this Aug 28, 2023

maximpn force-pushed the refactor-go-to-tule-details branch 3 times, most recently from 9765fac to dd44054 Compare August 29, 2023 05:36

maximpn changed the title ~~[Security Solution] Refactor goToRuleDetails~~ [Security Solution] Get rid of goToRuleDetails() in favor of goToRuleDetailsOf() Aug 29, 2023

maximpn requested a review from MadameSheema August 29, 2023 10:28

maximpn marked this pull request as ready for review August 29, 2023 10:28

maximpn requested review from a team as code owners August 29, 2023 10:28

maximpn requested a review from jpdjere August 29, 2023 10:28

maximpn changed the title ~~[Security Solution] Get rid of goToRuleDetails() in favor of goToRuleDetailsOf()~~ [Security Solution] Get rid of goToRuleDetails() in favor of goToRuleDetailsOf() Aug 29, 2023

maximpn force-pushed the refactor-go-to-tule-details branch from 739ad46 to 8343dab Compare August 31, 2023 10:48

jpdjere reviewed Sep 1, 2023

View reviewed changes

jpdjere approved these changes Sep 1, 2023

View reviewed changes

maximpn force-pushed the refactor-go-to-tule-details branch from a098da1 to 1ee4b72 Compare September 4, 2023 14:53

e40pud approved these changes Sep 5, 2023

View reviewed changes

maximpn force-pushed the refactor-go-to-tule-details branch 5 times, most recently from db04d46 to 60eadb2 Compare September 7, 2023 16:13

maximpn enabled auto-merge (squash) September 7, 2023 16:53

maximpn added 16 commits September 7, 2023 20:38

replace goToRuleDetails with goToTheRulesDetailsOf

d972717

rename goToTheRuleDetailsOf to goToRuleDetailsOf

9e05592

fix problems after rebase

07f692c

add missing package dependencies

f87c76a

fix duplication tests

83a3b2c

rearrange Kibana cleaning to reduce test flakiness

9742a08

use cleanKibana instead of esArchiverResetKibana task

c100e28

check only for rule to be not null

2cf4308

reuse rule data between tests

1e1eb6d

revert rule status check

be0c759

extract tested rule data into a variable

6b683f0

improve tests stability

553c856

revert usage of before/after instead of beforeEach/afterEach

014b375

reset the state in beforeEach

40aa281

fix a misprint

ede7228

fix endpoint exceptions tests after a merge conflict

b788ee0

maximpn force-pushed the refactor-go-to-tule-details branch from f7a8dbd to b788ee0 Compare September 7, 2023 19:04

maximpn merged commit 79c3fae into elastic:main Sep 7, 2023

kibanamachine added the backport:skip This commit does not require backporting label Sep 7, 2023

maximpn deleted the refactor-go-to-tule-details branch September 7, 2023 20:27

maximpn mentioned this pull request Sep 7, 2023

[Security Solution] Replace goToRuleDetails with goToTheRuleDetailsOf in e2e tests #164513

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Get rid of `goToRuleDetails()` in favor of `goToRuleDetailsOf()` #165011

[Security Solution] Get rid of `goToRuleDetails()` in favor of `goToRuleDetailsOf()` #165011

maximpn commented Aug 28, 2023 •

edited

Loading

elasticmachine commented Aug 29, 2023

elasticmachine commented Aug 29, 2023

jpdjere Sep 1, 2023

jpdjere Sep 1, 2023

jpdjere Sep 1, 2023

jpdjere Sep 1, 2023

maximpn Sep 2, 2023 •

edited

Loading

jpdjere Sep 1, 2023

jpdjere left a comment

e40pud left a comment

e40pud Sep 5, 2023

maximpn Sep 5, 2023

e40pud Sep 5, 2023

maximpn Sep 5, 2023

e40pud Sep 5, 2023

kibana-ci commented Sep 7, 2023 •

edited

Loading

[Security Solution] Get rid of goToRuleDetails() in favor of goToRuleDetailsOf() #165011

[Security Solution] Get rid of goToRuleDetails() in favor of goToRuleDetailsOf() #165011

Conversation

maximpn commented Aug 28, 2023 • edited Loading

Summary

Details

Does opening rule detail page directly make the tests less stable?

Flaky test runner

elasticmachine commented Aug 29, 2023

elasticmachine commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximpn Sep 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpdjere left a comment

Choose a reason for hiding this comment

e40pud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Sep 7, 2023 • edited Loading

💔 Build Failed

Failed CI Steps

Metrics [docs]

Async chunks

History

[Security Solution] Get rid of `goToRuleDetails()` in favor of `goToRuleDetailsOf()` #165011

[Security Solution] Get rid of `goToRuleDetails()` in favor of `goToRuleDetailsOf()` #165011

maximpn commented Aug 28, 2023 •

edited

Loading

maximpn Sep 2, 2023 •

edited

Loading

kibana-ci commented Sep 7, 2023 •

edited

Loading