[Security Solution] Install/Update Prebuilt Rules Test Implementation refactor #165488

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

(Tick the two open checkboxes in that issue when merging this PR)

Summary

This PR rewrites/refactors Cypress tests for the Installation and Upgrade of Prebuilt Rules implemented in #161687. Most of the changes here address feedback received in that PR - answered those comments there.

RBAC/Authorization: adds tests scenarios for users with full privileges (happy path)
Gets rid of huge util helpers such as assertRuleAvailableForInstallAndInstallOne and rewrites test cases in a more descriptive way, with step by step actions.
Gets rid of complex logic in tests and their helpers - removing if/else logic within them and removing optional flags passed to helpers.
Fixes bulkCreateRuleAssets util and uses it in other helpers to install multiple security-rule assets with a single bulk request to ES.

Additionally: checked installation_and_upgrade.md test plan to make sure it matches with the test in place. Added link to a ticket for a to-do task for the sections:

Rule installation workflow: filtering, sorting, pagination
Rule upgrade workflow: filtering, sorting, pagination

Flaky test runner

~~https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3420~~ 🟢

https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3513

elasticmachine · 2023-09-18T08:20:05Z

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

elasticmachine · 2023-09-18T08:20:05Z

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

xcrzx

Thanks for refactoring the tests, @jpdjere! They're much more readable now 👍 I've left a few comments for additional improvements, but they're pretty minor. Reach out if we need to discuss anything.

xcrzx · 2023-09-20T13:09:26Z

...ress/e2e/detection_response/prebuilt_rules/prebuilt_rules_install_update_authorization.cy.ts

      beforeEach(() => {
+        preventPrebuiltRulesPackageInstallation();


We have already called preventPrebuiltRulesPackageInstallation at the parent beforeEach. Should we remove this one?

xcrzx · 2023-09-20T13:11:44Z

...ress/e2e/detection_response/prebuilt_rules/prebuilt_rules_install_update_authorization.cy.ts

+      // Install one prebuilt rule asset to assert that user can't install it
+      installPrebuiltRuleAssets([RULE_2]);
+
+      // Install one prebuilt rule asset to assert that user can't upgrade it
+      createAndInstallMockedPrebuiltRules([OUTDATED_RULE_1]);
+      // Create a new version of the rule to make it available for upgrade
+      installPrebuiltRuleAssets([UPDATED_RULE_1]);


Some of the installed rules are needed for specific test cases only. Let's move them to corresponding test bodies so they will be easier to read.

xcrzx · 2023-09-20T13:12:16Z

...ress/e2e/detection_response/prebuilt_rules/prebuilt_rules_install_update_authorization.cy.ts

+
+    describe('User with full privileges on Security Solution', () => {
+      beforeEach(() => {
+        preventPrebuiltRulesPackageInstallation();


Same here. We have already called preventPrebuiltRulesPackageInstallation at the parent beforeEach. Should we remove this one?

xcrzx · 2023-09-20T13:16:11Z

...ress/e2e/detection_response/prebuilt_rules/prebuilt_rules_install_update_authorization.cy.ts

+    describe('User with full privileges on Security Solution', () => {
+      beforeEach(() => {
+        preventPrebuiltRulesPackageInstallation();
+        loginPageAsFullPrivilegesUser();


Should we test the workflow using the minimal required privileges instead of using a fully privileged user?

Yes, makes sense. I'm using now the Hunter/ T3 Analyst role, following this table: #81866 (comment)

Looks like the role with the minimum required priviliges for writing rules

xcrzx · 2023-09-20T13:46:58Z

...ess/e2e/detection_response/prebuilt_rules/prebuilt_rules_install_update_error_handling.cy.ts

+        // Make two mock rules available for installation
+        installPrebuiltRuleAssets([RULE_1, RULE_2]);
+        // Navigate to install Elastic rules page
+        addElasticRulesButtonClick();


Please move test actions, such as clicks, from beforeEach directly into the test bodies to enhance readability.

Even if some actions are common across multiple tests, duplicating them can make the tests clearer. Use the beforeEach section mainly for setting up preconditions, not for the main test steps.

xcrzx · 2023-09-20T13:48:14Z

...ess/e2e/detection_response/prebuilt_rules/prebuilt_rules_install_update_error_handling.cy.ts

      beforeEach(() => {
+        cleanKibana();


Same here. We already call cleanKibana() inside the top level beforeEach

xcrzx · 2023-09-20T13:50:54Z

...ess/e2e/detection_response/prebuilt_rules/prebuilt_rules_install_update_error_handling.cy.ts

+        installPrebuiltRuleAssets([UPDATED_RULE_1, UPDATED_RULE_2]);
+        login();
+
+        visitSecurityDetectionRulesPage();


We already call visitSecurityDetectionRulesPage() inside the top level beforeEach

xcrzx · 2023-09-20T14:11:07Z

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

+  for (const rule of rules) {
+    cy.get(RULES_UPDATES_TABLE).contains(rule['security-rule'].name);


Why do we need this check here?

We are checking here that when the update fails, the toast is shown with the right message -confirming the failed upgrade- and subsequently checking that the rule available for update are still present in the Rule Update table

xcrzx · 2023-09-20T14:12:36Z

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

  for (const rule of rules) {
-    cy.get(RULES_UPDATES_TABLE).contains(rule['security-rule'].name);
+    cy.get(rule['security-rule'].name).should('not.exist');
  }


Not sure I understand what this check does. Could you please add a comment?

xcrzx · 2023-09-20T14:16:13Z

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

+    );
+  }
+  for (const rule of rules) {
+    cy.get(getInstallSingleRuleLoadingSpinnerByRuleId(rule['security-rule'].rule_id)).should(


Looks like a typo, we need to check the "upgrade" spinner here

Thanks for catching this! Fixing

banderror · 2023-09-21T10:08:06Z

@xcrzx Do you think this PR is a great improvement compared to the previous implementation? Would we benefit from merging it now and addressing the comments you left in a separate PR? I'm asking this because @jpdjere won't be able to work on this PR until mid-October when he gets back from leave, and any improvements to tests' quality and stability we can merge now will contribute to our 8.11 goals.

xcrzx · 2023-09-22T09:59:22Z

@xcrzx Do you think this PR is a great improvement compared to the previous implementation? Would we benefit from merging it now and addressing the comments you left in a separate PR? I'm asking this because @jpdjere won't be able to work on this PR until mid-October when he gets back from leave, and any improvements to tests' quality and stability we can merge now will contribute to our 8.11 goals.

I think it's worth merging this PR. I would only run it against the flaky runner to ensure we do not introduce any flakiness.

jpdjere · 2023-10-09T14:15:03Z

@xcrzx @banderror Thanks for the review. I rebased, updated and addressed the additional feedback. I'll run the flaky test runner and will merge if everything is green.

…-ref HEAD~1..HEAD --fix'

…-fix'

kibana-ci · 2023-10-16T09:50:32Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: ebd8dba

Failed CI Steps

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`	13.0MB	13.0MB	+198.0B

History

💛 Build #167765 was flaky 38d6c99bd4112c000cf13032801dc548a2f1d973
💔 Build #167700 failed d3424f592a3d07741e4d3650658589ea74421a77
💔 Build #167695 failed 79aadb76cc0a1f4c1ac0d21be9f14a2a2ca05f27
💔 Build #166386 failed b69123b04686d7d2ad640db3b715565cd89d188b
💛 Build #166359 was flaky 81b3e91b7174527dca77935a324adfde39043ac5
💚 Build #159371 succeeded c3a33d7bfcfbbd081e3180767d602cef7c1aa898

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

cc @jpdjere

kibanamachine · 2023-10-17T11:11:22Z

💔 All backports failed

Status	Branch	Result
❌	8.11	Backport failed because of merge conflicts You might need to backport the following PRs to 8.11: - [Security Solution] Adding serverlessQA tag (#167494)

Manual backport

To create the backport manually run:

node scripts/backport --pr 165488

Questions ?

Please refer to the Backport tool documentation

jpdjere · 2023-10-17T15:40:28Z

💚 All backports created successfully

Status	Branch	Result
✅	8.11

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

Questions ?

Please refer to the Backport tool documentation

… refactor (elastic#165488) Fixes: elastic#148192 (Tick the two open checkboxes in that issue when merging this PR) ## Summary This PR rewrites/refactors Cypress tests for the Installation and Upgrade of Prebuilt Rules implemented in elastic#161687. Most of the changes here address feedback received in that PR - answered those comments there. - RBAC/Authorization: adds tests scenarios for users with full privileges (happy path) - Gets rid of huge util helpers such as `assertRuleAvailableForInstallAndInstallOne` and rewrites test cases in a more descriptive way, with step by step actions. - Gets rid of complex logic in tests and their helpers - removing if/else logic within them and removing optional flags passed to helpers. - Fixes `bulkCreateRuleAssets` util and uses it in other helpers to install multiple `security-rule` assets with a single bulk request to ES. Additionally: checked `installation_and_upgrade.md` test plan to make sure it matches with the test in place. Added [link](elastic#166215) to a ticket for a to-do task for the sections: - Rule installation workflow: filtering, sorting, pagination - Rule upgrade workflow: filtering, sorting, pagination ## Flaky test runner ~~https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3420~~ ~~🟢~~ https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3513 --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit 24c008b) # Conflicts: # x-pack/test/security_solution_cypress/package.json

…ntation refactor (#165488) (#169129) # Backport This will backport the following commits from `main` to `8.11`: - [[Security Solution] Install/Update Prebuilt Rules Test Implementation refactor (#165488)](#165488)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)

jpdjere self-assigned this Sep 1, 2023

jpdjere force-pushed the intall-update-test-implementation-refactor branch 4 times, most recently from b2c2751 to d5b1c16 Compare September 14, 2023 22:48

jpdjere mentioned this pull request Sep 14, 2023

[Security Solution] Extend Prebuilt rules install and update workflow test coverage #161687

Merged

1 task

jpdjere marked this pull request as ready for review September 18, 2023 08:19

jpdjere requested a review from a team as a code owner September 18, 2023 08:19

jpdjere requested review from nikitaindik, xcrzx and maximpn September 18, 2023 08:19

jpdjere added Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area v8.11.0 labels Sep 18, 2023

banderror added refactoring test-coverage issues & PRs for improving code test coverage labels Sep 18, 2023

xcrzx reviewed Sep 20, 2023

View reviewed changes

banderror removed request for maximpn and nikitaindik September 21, 2023 10:09

xcrzx self-requested a review September 22, 2023 09:59

xcrzx approved these changes Sep 22, 2023

View reviewed changes

jpdjere force-pushed the intall-update-test-implementation-refactor branch from c3a33d7 to 82bd289 Compare October 9, 2023 14:04

jpdjere and others added 17 commits October 16, 2023 10:34

Fix prebuilt_rules_install_update_authorization

795c1f6

Address further comments

482f26d

fix

d6f5a13

Fix

35edc93

Fix error handling

58d8fe9

Fix

c486103

Address feedback

d6d4a33

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

f3435d8

…-ref HEAD~1..HEAD --fix'

Run flaky test runner

83d42ef

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

e81e1aa

…-fix'

Undo package json changes

1b3684a

Fix

7da2b62

Set dirs for flaky test runner

5692c6b

Revert package json

c6f6b43

package.json

9d62845

revert

aeee4d4

Packagejson

ebd8dba

jpdjere force-pushed the intall-update-test-implementation-refactor branch from e87edcd to ebd8dba Compare October 16, 2023 08:37

jpdjere merged commit 24c008b into elastic:main Oct 17, 2023

jpdjere deleted the intall-update-test-implementation-refactor branch October 17, 2023 11:05

kibanamachine added the v8.12.0 label Oct 17, 2023

jpdjere mentioned this pull request Oct 17, 2023

[8.11] [Security Solution] Install/Update Prebuilt Rules Test Implementation refactor (#165488) #169129

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Install/Update Prebuilt Rules Test Implementation refactor #165488

[Security Solution] Install/Update Prebuilt Rules Test Implementation refactor #165488

jpdjere commented Sep 1, 2023 •

edited by kibanamachine

Loading

elasticmachine commented Sep 18, 2023

elasticmachine commented Sep 18, 2023

xcrzx left a comment

xcrzx Sep 20, 2023 •

edited

Loading

jpdjere Oct 9, 2023

xcrzx Sep 20, 2023

jpdjere Oct 9, 2023

xcrzx Sep 20, 2023

jpdjere Oct 9, 2023

xcrzx Sep 20, 2023

jpdjere Oct 9, 2023

jpdjere Oct 9, 2023

xcrzx Sep 20, 2023

jpdjere Oct 9, 2023

xcrzx Sep 20, 2023

xcrzx Sep 20, 2023

xcrzx Sep 20, 2023

jpdjere Oct 9, 2023

xcrzx Sep 20, 2023

jpdjere Oct 9, 2023

xcrzx Sep 20, 2023

jpdjere Oct 9, 2023

banderror commented Sep 21, 2023

xcrzx commented Sep 22, 2023

jpdjere commented Oct 9, 2023

kibana-ci commented Oct 16, 2023

kibanamachine commented Oct 17, 2023

jpdjere commented Oct 17, 2023

		beforeEach(() => {
		preventPrebuiltRulesPackageInstallation();

		for (const rule of rules) {
		cy.get(RULES_UPDATES_TABLE).contains(rule['security-rule'].name);

[Security Solution] Install/Update Prebuilt Rules Test Implementation refactor #165488

[Security Solution] Install/Update Prebuilt Rules Test Implementation refactor #165488

Conversation

jpdjere commented Sep 1, 2023 • edited by kibanamachine Loading

Summary

Flaky test runner

elasticmachine commented Sep 18, 2023

elasticmachine commented Sep 18, 2023

xcrzx left a comment

Choose a reason for hiding this comment

xcrzx Sep 20, 2023 • edited Loading

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

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

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

Choose a reason for hiding this comment

banderror commented Sep 21, 2023

xcrzx commented Sep 22, 2023

jpdjere commented Oct 9, 2023

kibana-ci commented Oct 16, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

History

kibanamachine commented Oct 17, 2023

💔 All backports failed

Manual backport

Questions ?

jpdjere commented Oct 17, 2023

💚 All backports created successfully

Questions ?

jpdjere commented Sep 1, 2023 •

edited by kibanamachine

Loading

xcrzx Sep 20, 2023 •

edited

Loading