From 659c34b777febd4f335b745b87e67fc0c40bd00b Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Tue, 13 Oct 2020 19:34:35 -0500 Subject: [PATCH 1/7] Unskip EQL tests These _should_ be fixed with the latest ES on master, but let's see if CI disagrees. --- .../cypress/integration/alerts_detection_rules_eql.spec.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts index 5502f35d6f0f8..83b9ac1e33e21 100644 --- a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts +++ b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts @@ -87,8 +87,7 @@ const expectedNumberOfRules = 1; const expectedNumberOfAlerts = 7; const expectedNumberOfSequenceAlerts = 1; -// Failing: See https://github.com/elastic/kibana/issues/79522 -describe.skip('Detection rules, EQL', () => { +describe('Detection rules, EQL', () => { beforeEach(() => { esArchiverLoad('timeline'); }); From 4d9ca5444840fcb1c432e377b6aeb109b1c0db4e Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Fri, 16 Oct 2020 19:32:01 -0500 Subject: [PATCH 2/7] Wait until alerts have populated on Rule Details Occasionally our tests hit a scenario where the rule has executed (its status is "succeeded"), but the generated alerts have not populated in the same time frame. In this case the test fails oddly, saying that the "alert count" element is not there when it is. I attempted to improve the error message by using a .should() with a callback, but that lead to even stranger behavior as the .should() would fail once (expected), and then not be able to find the element a second time. :( So we instead focus on fixing the real problem, here: wait until alerts populate (have a non-zero count) before performing the assertion. Because the page will not update automatically, we can't rely on cypress' retryability and must instead assert, click Refresh, and assert again, much like we're doing while waiting for the rule to execute. And like `waitForTheRuleToBeExecuted`, we're using a while loop that has no guarantee of ever exiting :( --- .../integration/alerts_detection_rules_eql.spec.ts | 8 +++----- .../security_solution/cypress/tasks/create_new_rule.ts | 10 ++++++++++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts index 83b9ac1e33e21..803cf428101c0 100644 --- a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts +++ b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts @@ -67,6 +67,7 @@ import { fillDefineEqlRuleAndContinue, fillScheduleRuleAndContinue, selectEqlRuleType, + waitForAlertsToPopulate, waitForTheRuleToBeExecuted, } from '../tasks/create_new_rule'; import { esArchiverLoad, esArchiverUnload } from '../tasks/es_archiver'; @@ -200,12 +201,9 @@ describe('Detection rules, EQL', () => { goToRuleDetails(); refreshPage(); waitForTheRuleToBeExecuted(); + waitForAlertsToPopulate(); - cy.get(NUMBER_OF_ALERTS) - .invoke('text') - .then((numberOfAlertsText) => { - cy.wrap(parseInt(numberOfAlertsText, 10)).should('eql', expectedNumberOfSequenceAlerts); - }); + cy.get(NUMBER_OF_ALERTS).should('have.text', expectedNumberOfSequenceAlerts); cy.get(ALERT_RULE_NAME).first().should('have.text', eqlSequenceRule.name); cy.get(ALERT_RULE_VERSION).first().should('have.text', '1'); cy.get(ALERT_RULE_METHOD).first().should('have.text', 'eql'); diff --git a/x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts b/x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts index fa3c219595c72..c1c1804a955cc 100644 --- a/x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts +++ b/x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts @@ -11,6 +11,7 @@ import { OverrideRule, ThresholdRule, } from '../objects/rule'; +import { NUMBER_OF_ALERTS } from '../screens/alerts'; import { ABOUT_CONTINUE_BTN, ABOUT_EDIT_TAB, @@ -271,6 +272,15 @@ export const waitForTheRuleToBeExecuted = async () => { } }; +export const waitForAlertsToPopulate = async () => { + let alertCount = 0; + while (alertCount <= 0) { + cy.get(REFRESH_BUTTON).click(); + const countText = await cy.get(NUMBER_OF_ALERTS).invoke('text').promisify(); + alertCount = parseInt(countText, 10) || 0; + } +}; + export const selectEqlRuleType = () => { cy.get(EQL_TYPE).click({ force: true }); }; From 9aca92833217f65a4b8069d2cee38dc7140b283b Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Sun, 18 Oct 2020 23:40:16 -0500 Subject: [PATCH 3/7] More robust cypress assertions * Uses should with a text matcher instead of using invoke('text') * Use of not.equal between a string and an element may have been a false positive --- .../security_solution/cypress/tasks/security_header.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/security_solution/cypress/tasks/security_header.ts b/x-pack/plugins/security_solution/cypress/tasks/security_header.ts index 28efc47120d32..a28767b489e46 100644 --- a/x-pack/plugins/security_solution/cypress/tasks/security_header.ts +++ b/x-pack/plugins/security_solution/cypress/tasks/security_header.ts @@ -19,9 +19,9 @@ export const navigateFromHeaderTo = (page: string) => { }; export const refreshPage = () => { - cy.get(REFRESH_BUTTON).click({ force: true }).invoke('text').should('not.equal', 'Updating'); + cy.get(REFRESH_BUTTON).click({ force: true }).should('not.have.text', 'Updating'); }; export const waitForThePageToBeUpdated = () => { - cy.get(REFRESH_BUTTON).should('not.equal', 'Updating'); + cy.get(REFRESH_BUTTON).should('not.have.text', 'Updating'); }; From cbc650b350ed7552d6175c7f1544f8b7f98ee011 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Mon, 19 Oct 2020 12:05:08 -0500 Subject: [PATCH 4/7] Perform cypress loops in a manner guaranteed to exit We have a few tasks that require polling for some background work to be completed. The basic form is: assert the byproduct, or refresh the page and try again. We were previously doing this with a while loop, which was not guaranteed to ever complete, leading to cryptic failures if the process ever hung. Instead, this implements a safer polling mechanism with a definite termination similar to the cypress-wait-until plugin. --- .../cypress/support/commands.js | 33 +++++++++++++++++++ .../cypress/support/index.d.ts | 7 ++++ .../cypress/tasks/create_new_rule.ts | 29 +++++++++------- 3 files changed, 58 insertions(+), 11 deletions(-) diff --git a/x-pack/plugins/security_solution/cypress/support/commands.js b/x-pack/plugins/security_solution/cypress/support/commands.js index 0dbcb1af4642f..dbd60cdd31a5a 100644 --- a/x-pack/plugins/security_solution/cypress/support/commands.js +++ b/x-pack/plugins/security_solution/cypress/support/commands.js @@ -69,3 +69,36 @@ Cypress.Commands.add( }); } ); + +const waitUntil = (subject, fn, options = {}) => { + const { interval = 200, timeout = 5000 } = options; + let attempts = Math.floor(timeout / interval); + + const completeOrRetry = (result) => { + if (result) { + return result; + } + if (attempts < 1) { + throw new Error(`Timed out while retrying, last result was: {${result}}`); + } + cy.wait(interval, { log: false }).then(() => { + attempts--; + // eslint-disable-next-line no-use-before-define + return evaluate(); + }); + }; + + const evaluate = () => { + const result = fn(subject); + + if (result && result.then) { + return result.then(completeOrRetry); + } else { + return completeOrRetry(result); + } + }; + + return evaluate(); +}; + +Cypress.Commands.add('waitUntil', { prevSubject: 'optional' }, waitUntil); diff --git a/x-pack/plugins/security_solution/cypress/support/index.d.ts b/x-pack/plugins/security_solution/cypress/support/index.d.ts index 59180507cbade..0cf3cf614cdb9 100644 --- a/x-pack/plugins/security_solution/cypress/support/index.d.ts +++ b/x-pack/plugins/security_solution/cypress/support/index.d.ts @@ -14,5 +14,12 @@ declare namespace Cypress { searchStrategyName?: string ): Chainable; attachFile(fileName: string, fileType?: string): Chainable; + waitUntil( + fn: (subject: Subject) => boolean | Chainable, + options?: { + interval: number; + timeout: number; + } + ): Chainable; } } diff --git a/x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts b/x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts index c1c1804a955cc..5b2c365dfd8c3 100644 --- a/x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts +++ b/x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts @@ -63,6 +63,7 @@ import { EQL_QUERY_INPUT, } from '../screens/create_new_rule'; import { TIMELINE } from '../screens/timelines'; +import { refreshPage } from './security_header'; export const createAndActivateRule = () => { cy.get(SCHEDULE_CONTINUE_BUTTON).click({ force: true }); @@ -264,21 +265,27 @@ export const selectThresholdRuleType = () => { cy.get(THRESHOLD_TYPE).click({ force: true }); }; -export const waitForTheRuleToBeExecuted = async () => { - let status = ''; - while (status !== 'succeeded') { +export const waitForTheRuleToBeExecuted = () => { + cy.waitUntil(() => { cy.get(REFRESH_BUTTON).click(); - status = await cy.get(RULE_STATUS).invoke('text').promisify(); - } + return cy + .get(RULE_STATUS) + .invoke('text') + .then((ruleStatus) => ruleStatus === 'succeeded'); + }); }; export const waitForAlertsToPopulate = async () => { - let alertCount = 0; - while (alertCount <= 0) { - cy.get(REFRESH_BUTTON).click(); - const countText = await cy.get(NUMBER_OF_ALERTS).invoke('text').promisify(); - alertCount = parseInt(countText, 10) || 0; - } + cy.waitUntil(() => { + refreshPage(); + return cy + .get(NUMBER_OF_ALERTS) + .invoke('text') + .then((countText) => { + const alertCount = parseInt(countText, 10) || 0; + return alertCount > 0; + }); + }); }; export const selectEqlRuleType = () => { From dff464554fcdc923b019f92faf1c055b5a6e4e94 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Mon, 19 Oct 2020 12:19:08 -0500 Subject: [PATCH 5/7] Update other specs that are asserting on alerts * Do not automatically refresh the page * This is only necessary if we're not in the state we need. The `waitFor` helper functions automatically reload whatever needs to be reloaded, so we're delegating this task to them. * Ensure we wait for alerts to be nonzero before our assertion * Otherwise we get some strange behavior around this field's availability; see previous commits --- .../integration/alerts_detection_rules_custom.spec.ts | 10 +++------- .../integration/alerts_detection_rules_eql.spec.ts | 9 ++------- .../alerts_detection_rules_override.spec.ts | 10 +++------- .../alerts_detection_rules_threshold.spec.ts | 10 +++------- 4 files changed, 11 insertions(+), 28 deletions(-) diff --git a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_custom.spec.ts b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_custom.spec.ts index 41665cf6d20a4..d7a963ba4efa7 100644 --- a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_custom.spec.ts +++ b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_custom.spec.ts @@ -91,12 +91,12 @@ import { goToAboutStepTab, goToActionsStepTab, goToScheduleStepTab, + waitForAlertsToPopulate, waitForTheRuleToBeExecuted, } from '../tasks/create_new_rule'; import { saveEditedRule, waitForKibana } from '../tasks/edit_rule'; import { esArchiverLoad, esArchiverUnload } from '../tasks/es_archiver'; import { loginAndWaitForPageWithoutDateRange } from '../tasks/login'; -import { refreshPage } from '../tasks/security_header'; import { DETECTIONS_URL } from '../urls/navigation'; @@ -197,14 +197,10 @@ describe('Custom detection rules creation', () => { ); }); - refreshPage(); waitForTheRuleToBeExecuted(); + waitForAlertsToPopulate(); - cy.get(NUMBER_OF_ALERTS) - .invoke('text') - .then((numberOfAlertsText) => { - cy.wrap(parseInt(numberOfAlertsText, 10)).should('be.above', 0); - }); + cy.get(NUMBER_OF_ALERTS).invoke('text').then(parseFloat).should('be.above', 0); cy.get(ALERT_RULE_NAME).first().should('have.text', newRule.name); cy.get(ALERT_RULE_VERSION).first().should('have.text', '1'); cy.get(ALERT_RULE_METHOD).first().should('have.text', 'query'); diff --git a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts index 803cf428101c0..566720bd6083a 100644 --- a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts +++ b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts @@ -160,14 +160,10 @@ describe('Detection rules, EQL', () => { ); }); - refreshPage(); waitForTheRuleToBeExecuted(); + waitForAlertsToPopulate(); - cy.get(NUMBER_OF_ALERTS) - .invoke('text') - .then((numberOfAlertsText) => { - cy.wrap(parseInt(numberOfAlertsText, 10)).should('eql', expectedNumberOfAlerts); - }); + cy.get(NUMBER_OF_ALERTS).should('have.text', expectedNumberOfAlerts); cy.get(ALERT_RULE_NAME).first().should('have.text', eqlRule.name); cy.get(ALERT_RULE_VERSION).first().should('have.text', '1'); cy.get(ALERT_RULE_METHOD).first().should('have.text', 'eql'); @@ -199,7 +195,6 @@ describe('Detection rules, EQL', () => { filterByCustomRules(); goToRuleDetails(); - refreshPage(); waitForTheRuleToBeExecuted(); waitForAlertsToPopulate(); diff --git a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_override.spec.ts b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_override.spec.ts index edf7305f6916e..e31fe2e9a3911 100644 --- a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_override.spec.ts +++ b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_override.spec.ts @@ -72,11 +72,11 @@ import { fillAboutRuleWithOverrideAndContinue, fillDefineCustomRuleWithImportedQueryAndContinue, fillScheduleRuleAndContinue, + waitForAlertsToPopulate, waitForTheRuleToBeExecuted, } from '../tasks/create_new_rule'; import { esArchiverLoad, esArchiverUnload } from '../tasks/es_archiver'; import { loginAndWaitForPageWithoutDateRange } from '../tasks/login'; -import { refreshPage } from '../tasks/security_header'; import { DETECTIONS_URL } from '../urls/navigation'; @@ -179,14 +179,10 @@ describe('Detection rules, override', () => { ); }); - refreshPage(); waitForTheRuleToBeExecuted(); + waitForAlertsToPopulate(); - cy.get(NUMBER_OF_ALERTS) - .invoke('text') - .then((numberOfAlertsText) => { - cy.wrap(parseInt(numberOfAlertsText, 10)).should('be.above', 0); - }); + cy.get(NUMBER_OF_ALERTS).invoke('text').then(parseFloat).should('be.above', 0); cy.get(ALERT_RULE_NAME).first().should('have.text', 'auditbeat'); cy.get(ALERT_RULE_VERSION).first().should('have.text', '1'); cy.get(ALERT_RULE_METHOD).first().should('have.text', 'query'); diff --git a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_threshold.spec.ts b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_threshold.spec.ts index 5095e856e3f65..f997b9eb3bc51 100644 --- a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_threshold.spec.ts +++ b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_threshold.spec.ts @@ -68,11 +68,11 @@ import { fillDefineThresholdRuleAndContinue, fillScheduleRuleAndContinue, selectThresholdRuleType, + waitForAlertsToPopulate, waitForTheRuleToBeExecuted, } from '../tasks/create_new_rule'; import { esArchiverLoad, esArchiverUnload } from '../tasks/es_archiver'; import { loginAndWaitForPageWithoutDateRange } from '../tasks/login'; -import { refreshPage } from '../tasks/security_header'; import { DETECTIONS_URL } from '../urls/navigation'; @@ -162,14 +162,10 @@ describe('Detection rules, threshold', () => { ); }); - refreshPage(); waitForTheRuleToBeExecuted(); + waitForAlertsToPopulate(); - cy.get(NUMBER_OF_ALERTS) - .invoke('text') - .then((numberOfAlertsText) => { - cy.wrap(parseInt(numberOfAlertsText, 10)).should('be.below', 100); - }); + cy.get(NUMBER_OF_ALERTS).invoke('text').then(parseFloat).should('be.below', 100); cy.get(ALERT_RULE_NAME).first().should('have.text', newThresholdRule.name); cy.get(ALERT_RULE_VERSION).first().should('have.text', '1'); cy.get(ALERT_RULE_METHOD).first().should('have.text', 'threshold'); From 245970d689651d791b448cbfc28ec5a481f7d37e Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Mon, 19 Oct 2020 13:28:40 -0500 Subject: [PATCH 6/7] Remove unused import --- .../cypress/integration/alerts_detection_rules_eql.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts index 566720bd6083a..1b76e74d47bb1 100644 --- a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts +++ b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts @@ -72,7 +72,6 @@ import { } from '../tasks/create_new_rule'; import { esArchiverLoad, esArchiverUnload } from '../tasks/es_archiver'; import { loginAndWaitForPageWithoutDateRange } from '../tasks/login'; -import { refreshPage } from '../tasks/security_header'; import { DETECTIONS_URL } from '../urls/navigation'; From 24748c9c75bb9f8729c421604e3e74b115357072 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Mon, 19 Oct 2020 18:35:57 -0500 Subject: [PATCH 7/7] Fix false positive in Rule Creation specs Threat Match Rules introduced an additional query input, causing our CUSTOM_QUERY_INPUT to be ambiguous. However, instead of failing due to the ambiguity, the behavior of cypress seems to be to pass! While I haven't yet tracked down the cause of these false positives, disambiguating these selectors is the immediate fix. --- .../security_solution/cypress/screens/create_new_rule.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/security_solution/cypress/screens/create_new_rule.ts b/x-pack/plugins/security_solution/cypress/screens/create_new_rule.ts index e1ab5ff30572f..5e7dce6966195 100644 --- a/x-pack/plugins/security_solution/cypress/screens/create_new_rule.ts +++ b/x-pack/plugins/security_solution/cypress/screens/create_new_rule.ts @@ -31,7 +31,11 @@ export const COMBO_BOX_INPUT = '[data-test-subj="comboBoxInput"]'; export const CREATE_AND_ACTIVATE_BTN = '[data-test-subj="create-activate"]'; -export const CUSTOM_QUERY_INPUT = '[data-test-subj="queryInput"]'; +export const CUSTOM_QUERY_INPUT = + '[data-test-subj="detectionEngineStepDefineRuleQueryBar"] [data-test-subj="queryInput"]'; + +export const THREAT_MATCH_QUERY_INPUT = + '[data-test-subj="detectionEngineStepDefineThreatRuleQueryBar"] [data-test-subj="queryInput"]'; export const DEFINE_CONTINUE_BUTTON = '[data-test-subj="define-continue"]';