Skip to content

Commit

Permalink
[Security Solution][Detections] Fix EQL cypress tests (#80440) (#81204)
Browse files Browse the repository at this point in the history
* Unskip EQL tests

These _should_ be fixed with the latest ES on master, but let's see if
CI disagrees.

* 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 :(

* 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

* 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.

* 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

* Remove unused import

* 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.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
  • Loading branch information
rylnd and kibanamachine authored Oct 20, 2020
1 parent 84592a5 commit 8615757
Show file tree
Hide file tree
Showing 9 changed files with 84 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,11 @@ import {
fillDefineEqlRuleAndContinue,
fillScheduleRuleAndContinue,
selectEqlRuleType,
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';

Expand All @@ -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');
});
Expand Down Expand Up @@ -160,14 +159,10 @@ describe.skip('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');
Expand Down Expand Up @@ -199,14 +194,10 @@ describe.skip('Detection rules, EQL', () => {

filterByCustomRules();
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');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"]';

Expand Down
33 changes: 33 additions & 0 deletions x-pack/plugins/security_solution/cypress/support/commands.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
7 changes: 7 additions & 0 deletions x-pack/plugins/security_solution/cypress/support/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,12 @@ declare namespace Cypress {
searchStrategyName?: string
): Chainable<Subject>;
attachFile(fileName: string, fileType?: string): Chainable<JQuery>;
waitUntil(
fn: (subject: Subject) => boolean | Chainable<boolean>,
options?: {
interval: number;
timeout: number;
}
): Chainable<Subject>;
}
}
27 changes: 22 additions & 5 deletions x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
OverrideRule,
ThresholdRule,
} from '../objects/rule';
import { NUMBER_OF_ALERTS } from '../screens/alerts';
import {
ABOUT_CONTINUE_BTN,
ABOUT_EDIT_TAB,
Expand Down Expand Up @@ -62,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 });
Expand Down Expand Up @@ -264,12 +266,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 () => {
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 = () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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');
};

0 comments on commit 8615757

Please sign in to comment.