-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Update e2e tests to work with detection rules installed from a Fleet package #142311
Conversation
💚 CLA has been signed |
7081396
to
35a1707
Compare
dd3ee11
to
2e5019c
Compare
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Files by Code Ownerelastic/security-detections-response
elastic/security-detections-response-alerts
elastic/security-detections-response-rules
elastic/security-solution
elastic/security-threat-hunting
|
4451fa7
to
a3e15ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM 👍 I left a few relatively minor comments.
I run some of these tests locally and got various timeouts. Now I'm pretty convinced that the default timeout of 20 seconds just doesn't work. We need to increase it to a value that would work in 99% of local test runs for every engineer in AET. I can play with it later and open a separate PR. cc @MadameSheema
getPrepackagedRulesCount().then((prepackagedRulesCount) => { | ||
checkPrebuiltRulesCannotBeModified(prepackagedRulesCount); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Let's be consistent with naming and call it prebuilt
everywhere :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd stick to prepackaged
, then. As that's how we call them on the API level, and that's something we cannot change already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, we are keeping the prebuilt
naming. However, there still will be some discrepancy between naming in some parts of our application (e.g., schema and API still use prepackaged
), which will be addressed separately.
getPrepackagedRulesCount().then((prepackagedRulesCount) => { | ||
const expectedNumberCustomRulesToBeExported = 1; | ||
const totalNumberOfRules = expectedNumberCustomRulesToBeExported + prepackagedRulesCount; | ||
|
||
loadPrebuiltDetectionRulesFromHeaderBtn(); | ||
loadPrebuiltDetectionRulesFromHeaderBtn(); | ||
|
||
selectAllRules(); | ||
bulkExportRules(); | ||
selectAllRules(); | ||
bulkExportRules(); | ||
|
||
cy.get(MODAL_CONFIRMATION_BODY).contains( | ||
`${totalNumberOfPrebuiltRules} prebuilt Elastic rules (exporting prebuilt rules is not supported)` | ||
); | ||
cy.get(MODAL_CONFIRMATION_BODY).contains( | ||
`${prepackagedRulesCount} prebuilt Elastic rules (exporting prebuilt rules is not supported)` | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we're getting the count before installing the rules. I guess, it works because getPrepackagedRulesCount
returns the number of prebuilt rules (to be precise, the number of security-rule
asset SOs) available for installation rather than the number of installed prebuilt rules (the number of alert
SOs).
I think this naming can be confusing and I'd rename getPrepackagedRulesCount
to getNumberOfAllAvailablePrebuiltRules
or smth like that.
Also, shouldn't we wait after calling loadPrebuiltDetectionRulesFromHeaderBtn
until the installation is finished? I don't see any explicit check for this like waitForPrebuiltDetectionRulesToBeLoaded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I see how that naming could be confusing. Changed getPrepackagedRulesCount
to getAvailablePrepackagedRulesCount
, hope that helps a bit.
Also, shouldn't we wait after calling loadPrebuiltDetectionRulesFromHeaderBtn until the installation is finished?
Inside that method, we wait until the update button disappears, indicating the completion. So we should be good (until UI changes 🙂).
@@ -79,31 +64,27 @@ describe('Prebuilt rules', () => { | |||
|
|||
cy.get(SELECT_ALL_RULES_ON_PAGE_CHECKBOX).click(); | |||
enableSelectedRules(); | |||
waitForRuleToChangeStatus(); | |||
waitForRuleToUpdate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Offtopic, but... I'm curious how this test called Allows to enable/disable all rules at once
ended up being a part of the Rule monitoring table
set which is in turn a part of the test suite for prebuilt rules. How all that is related?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷♂️ I don't think there's a good reason for that. This code is probably overdue for refactoring.
beforeEach(() => { | ||
deleteAlertsAndRules(); | ||
visitWithoutDateRange(DETECTIONS_RULE_MANAGEMENT_URL); | ||
loadPrebuiltDetectionRules(); | ||
waitForPrebuiltDetectionRulesToBeLoaded(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -184,7 +184,7 @@ describe('Related integrations', () => { | |||
'{"package":"system","version":"1.17.0"}{"package":"aws","integration":"cloudtrail","version":"1.17.0"}{"package":"aws","integration":"cloudfront","version":"1.17.0"}{"package":"aws","integration":"unknown","version":"1.17.0"}'; | |||
|
|||
enableRule(firstRule); | |||
waitForRuleToChangeStatus(); | |||
waitForRuleToUpdate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
getPrepackagedRulesCount().then((prepackagedRulesCount) => { | ||
cy.get(SELECTED_RULES_NUMBER_LABEL).should('contain.text', prepackagedRulesCount); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can see that loadPrebuiltDetectionRules
is used here in the tests for rule selection in the table, and I don't see how that feature is related to prebuilt rules and why we should be loading all of them to test it.
Seems like we'll need to revisit e2e test coverage for the table after we refactor it.
@@ -132,16 +133,19 @@ export const deleteRuleFromDetailsPage = () => { | |||
}; | |||
|
|||
export const duplicateSelectedRules = () => { | |||
cy.log('Duplicate selected rules'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this improvement 👍
Nit: Can we / should we do it like this to support refactorings easier:
cy.log(duplicateSelectedRules.name)
The downside is probably slightly worse readability of Cypress logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll also need to rewrite all helpers to named functions for this to work. But I'd prefer log messages to be more easily readable by humans as the test runner's output is already overloaded with technical information.
export const waitForRuleToUpdate = () => { | ||
cy.log('Wait for rules to update'); | ||
cy.get(RULE_SWITCH_LOADER, { timeout: 300000 }).should('exist'); | ||
cy.get(RULE_SWITCH_LOADER, { timeout: 300000 }).should('not.exist'); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to add a comment for this function and clarify what it exactly does and where/when it can be used. Because the name is kinda abstract. Perhaps some renaming wouldn't hurt.
export const getPrepackagedRulesCount = () => { | ||
cy.log('Get prepackaged rules count'); | ||
return getPrepackagedRulesStatus().then(({ body }) => { | ||
const prebuiltRulesCount = body.rules_installed + body.rules_not_installed; | ||
|
||
return prebuiltRulesCount; | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For visibility: we talked that this function should return the number of existing security-rule
asset saved objects instead of calculating this sum. In order to do this, the api/detection_engine/rules/prepackaged/_status
endpoint will need to return this value in its response.
b7d0cd3
to
1c52554
Compare
@@ -210,7 +212,9 @@ describe('Detection rules, bulk edit', () => { | |||
clickAddTagsMenuItem(); | |||
waitForMixedRulesBulkEditModal(expectedNumberOfCustomRulesToBeEdited); | |||
|
|||
checkPrebuiltRulesCannotBeModified(totalNumberOfPrebuiltRules); | |||
getAvailablePrepackagedRulesCount().then((availablePrepackagedRulesCount) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inside in getAvailablePrepackagedRulesCoun
, there is a checks for:
const prebuiltRulesCount = body.rules_installed + body.rules_not_installed;
but the number of prebuilt rules displayed in UI is the number of installed.
So, correctly would to assert this value against number of installed rules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the number of prebuilt rules displayed in UI is the number of installed.
So, correctly would to assert this value against number of installed rules
On the loadPrebuiltDetectionRulesFromHeaderBtn()
step, all available prebuilt rules are getting installed, so it is okay to check if the UI displays all rules count.
@@ -179,7 +185,9 @@ export const loadPrebuiltDetectionRules = () => { | |||
* load prebuilt rules by clicking button on page header | |||
*/ | |||
export const loadPrebuiltDetectionRulesFromHeaderBtn = () => { | |||
cy.get(LOAD_PREBUILT_RULES_ON_PAGE_HEADER_BTN).click().should('not.exist'); | |||
cy.log('load prebuilt detection rules from header'); | |||
waitTillPrepackagedRulesReadyToInstall(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use one name for prepackaged/prebuilt rules.
In some places it's prebuilt, in other prepackaged. Can be confusing, especially for new developers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's been already discussed: #142311 (comment)
9db70fe
to
c40a789
Compare
…-ref HEAD~1..HEAD --fix'
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @xcrzx |
Summary
This PR prepares Cypress tests to work with prebuilt detection rules installed from a Fleet package as the filesystem distribution method of prebuilt rules will be removed in future releases (see #139926).
Notable changes:
waitTillPrepackagedRulesReadyToInstall()
to ensure they are available because it could take some time to install the Fleet package with rules (it happens automatically on the initial page load).getPrepackagedRulesCount()
to fetch the total number of prebuilt rules via API.