Skip to content

Commit

Permalink
[Security Solution] Get rid of goToRuleDetails() in favor of `goToR…
Browse files Browse the repository at this point in the history
…uleDetailsOf()` (elastic#165011)

**Addresses:** elastic#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

  ```ts
  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)](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3017) (there is no indication of negative impact)
  • Loading branch information
maximpn authored Sep 7, 2023
1 parent 3151aad commit 79c3fae
Show file tree
Hide file tree
Showing 39 changed files with 694 additions and 660 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,13 @@ import { useBoolState } from '../../../../common/hooks/use_bool_state';
import { useLegacyUrlRedirect } from './use_redirect_legacy_url';
import { RuleDetailTabs, useRuleDetailsTabs } from './use_rule_details_tabs';

const RULE_EXCEPTION_LIST_TYPES = [
ExceptionListTypeEnum.DETECTION,
ExceptionListTypeEnum.RULE_DEFAULT,
];

const RULE_ENDPOINT_EXCEPTION_LIST_TYPE = [ExceptionListTypeEnum.ENDPOINT];

/**
* Need a 100% height here to account for the graph/analyze tool, which sets no explicit height parameters, but fills the available space.
*/
Expand Down Expand Up @@ -603,6 +610,7 @@ const RuleDetailsPageComponent: React.FC<DetectionEngineComponentProps> = ({
<RuleSwitch
id={rule?.id ?? '-1'}
isDisabled={
!rule ||
!isExistingRule ||
!canEditRuleWithActions(rule, hasActionsPrivileges) ||
!hasUserCRUDPermission(canUserCRUD) ||
Expand Down Expand Up @@ -760,10 +768,7 @@ const RuleDetailsPageComponent: React.FC<DetectionEngineComponentProps> = ({
<Route path={`/rules/id/:detailName/:tabName(${RuleDetailTabs.exceptions})`}>
<ExceptionsViewer
rule={rule}
listTypes={[
ExceptionListTypeEnum.DETECTION,
ExceptionListTypeEnum.RULE_DEFAULT,
]}
listTypes={RULE_EXCEPTION_LIST_TYPES}
onRuleChange={refreshRule}
isViewReadOnly={!isExistingRule}
data-test-subj="exceptionTab"
Expand All @@ -774,7 +779,7 @@ const RuleDetailsPageComponent: React.FC<DetectionEngineComponentProps> = ({
>
<ExceptionsViewer
rule={rule}
listTypes={[ExceptionListTypeEnum.ENDPOINT]}
listTypes={RULE_ENDPOINT_EXCEPTION_LIST_TYPE}
onRuleChange={refreshRule}
isViewReadOnly={!isExistingRule}
data-test-subj="endpointExceptionsTab"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@

import { ROLES } from '@kbn/security-solution-plugin/common/test';

import { DETECTIONS_RULE_MANAGEMENT_URL, ALERTS_URL } from '../../urls/navigation';
import { DETECTIONS_RULE_MANAGEMENT_URL, ALERTS_URL, ruleDetailsUrl } from '../../urls/navigation';
import { getNewRule } from '../../objects/rule';
import { PAGE_TITLE } from '../../screens/common/page';

import { login, visitWithoutDateRange, waitForPageWithoutDateRange } from '../../tasks/login';
import { goToRuleDetails } from '../../tasks/alerts_detection_rules';
import { createRule, deleteCustomRule } from '../../tasks/api_calls/rules';
import {
getCallOut,
Expand Down Expand Up @@ -80,10 +79,9 @@ describe(

context('On Rule Details page', () => {
beforeEach(() => {
createRule(getNewRule({ rule_id: 'rule_testing' }));
loadPageAsPlatformEngineerUser(DETECTIONS_RULE_MANAGEMENT_URL);
waitForPageTitleToBeShown();
goToRuleDetails();
createRule(getNewRule({ rule_id: 'rule_testing' })).then((rule) =>
loadPageAsPlatformEngineerUser(ruleDetailsUrl(rule.body.id))
);
});

afterEach(() => {
Expand Down Expand Up @@ -130,10 +128,9 @@ describe(

context('On Rule Details page', () => {
beforeEach(() => {
createRule(getNewRule({ rule_id: 'rule_testing' }));
loadPageAsPlatformEngineerUser(DETECTIONS_RULE_MANAGEMENT_URL);
waitForPageTitleToBeShown();
goToRuleDetails();
createRule(getNewRule({ rule_id: 'rule_testing' })).then((rule) =>
loadPageAsPlatformEngineerUser(ruleDetailsUrl(rule.body.id))
);
});

afterEach(() => {
Expand Down Expand Up @@ -180,10 +177,9 @@ describe(

context('On Rule Details page', () => {
beforeEach(() => {
createRule(getNewRule({ rule_id: 'rule_testing' }));
loadPageAsPlatformEngineerUser(DETECTIONS_RULE_MANAGEMENT_URL);
waitForPageTitleToBeShown();
goToRuleDetails();
createRule(getNewRule({ rule_id: 'rule_testing' })).then((rule) =>
loadPageAsPlatformEngineerUser(ruleDetailsUrl(rule.body.id))
);
});

afterEach(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,11 @@ import {
THREAT_DETAILS_ACCORDION,
} from '../../screens/alerts_details';
import { TIMELINE_FIELD } from '../../screens/rule_details';
import { goToRuleDetails } from '../../tasks/alerts_detection_rules';
import { expandFirstAlert, setEnrichmentDates, viewThreatIntelTab } from '../../tasks/alerts';
import { createRule } from '../../tasks/api_calls/rules';
import { openJsonView, openThreatIndicatorDetails } from '../../tasks/alerts_details';

import { DETECTIONS_RULE_MANAGEMENT_URL } from '../../urls/navigation';
import { ruleDetailsUrl } from '../../urls/navigation';
import { addsFieldsToTimeline } from '../../tasks/rule_details';

describe('CTI Enrichment', { tags: ['@ess', '@serverless', '@brokenInServerless'] }, () => {
Expand All @@ -35,7 +34,7 @@ describe('CTI Enrichment', { tags: ['@ess', '@serverless', '@brokenInServerless'
cy.task('esArchiverLoad', { archiveName: 'threat_indicator' });
cy.task('esArchiverLoad', { archiveName: 'suspicious_source_event' });
login();
createRule({ ...getNewThreatIndicatorRule(), rule_id: 'rule_testing', enabled: true });

disableExpandableFlyout();
});

Expand All @@ -46,8 +45,9 @@ describe('CTI Enrichment', { tags: ['@ess', '@serverless', '@brokenInServerless'

beforeEach(() => {
login();
visitWithoutDateRange(DETECTIONS_RULE_MANAGEMENT_URL);
goToRuleDetails();
createRule({ ...getNewThreatIndicatorRule(), rule_id: 'rule_testing', enabled: true }).then(
(rule) => visitWithoutDateRange(ruleDetailsUrl(rule.body.id))
);
});

// Skipped: https://github.com/elastic/kibana/issues/162818
Expand Down Expand Up @@ -159,12 +159,6 @@ describe('CTI Enrichment', { tags: ['@ess', '@serverless', '@brokenInServerless'
cy.task('esArchiverLoad', { archiveName: 'threat_indicator2' });
});

beforeEach(() => {
login();
visitWithoutDateRange(DETECTIONS_RULE_MANAGEMENT_URL);
goToRuleDetails();
});

after(() => {
cy.task('esArchiverUnload', 'threat_indicator2');
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@

import { ROLES } from '@kbn/security-solution-plugin/common/test';

import { DETECTIONS_RULE_MANAGEMENT_URL, ALERTS_URL } from '../../urls/navigation';
import { DETECTIONS_RULE_MANAGEMENT_URL, ALERTS_URL, ruleDetailsUrl } from '../../urls/navigation';
import { getNewRule } from '../../objects/rule';
import { PAGE_TITLE } from '../../screens/common/page';

import { login, visitWithoutDateRange, waitForPageWithoutDateRange } from '../../tasks/login';
import { goToRuleDetails } from '../../tasks/alerts_detection_rules';
import { createRule, deleteCustomRule } from '../../tasks/api_calls/rules';
import {
getCallOut,
Expand Down Expand Up @@ -75,10 +74,9 @@ describe('Detections > Callouts', { tags: '@ess' }, () => {

context('On Rule Details page', () => {
beforeEach(() => {
createRule(getNewRule());
loadPageAsReadOnlyUser(DETECTIONS_RULE_MANAGEMENT_URL);
waitForPageTitleToBeShown();
goToRuleDetails();
createRule(getNewRule()).then((rule) =>
loadPageAsReadOnlyUser(ruleDetailsUrl(rule.body.id))
);
});

afterEach(() => {
Expand Down Expand Up @@ -126,10 +124,9 @@ describe('Detections > Callouts', { tags: '@ess' }, () => {

context('On Rule Details page', () => {
beforeEach(() => {
createRule(getNewRule());
loadPageAsPlatformEngineer(DETECTIONS_RULE_MANAGEMENT_URL);
waitForPageTitleToBeShown();
goToRuleDetails();
createRule(getNewRule()).then((rule) =>
loadPageAsPlatformEngineer(ruleDetailsUrl(rule.body.id))
);
});

afterEach(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import { getIndexConnector } from '../../../objects/connector';
import { getSimpleCustomQueryRule } from '../../../objects/rule';

import { goToRuleDetails } from '../../../tasks/alerts_detection_rules';
import { goToRuleDetailsOf } from '../../../tasks/alerts_detection_rules';
import { deleteIndex, waitForNewDocumentToBeIndexed } from '../../../tasks/api_calls/elasticsearch';
import {
cleanKibana,
Expand Down Expand Up @@ -51,14 +51,15 @@ describe(
const initialNumberOfDocuments = 0;
const expectedJson = JSON.parse(actions.connectors[0].document);

it('Indexes a new document after the index action is triggered ', function () {
it('Indexes a new document after the index action is triggered', function () {
visit(RULE_CREATION);
fillDefineCustomRuleAndContinue(rule);
fillAboutRuleAndContinue(rule);
fillScheduleRuleAndContinue(rule);
fillRuleAction(actions);
createAndEnableRule();
goToRuleDetails();

goToRuleDetailsOf(rule.name);

/* When the rule is executed, the action is triggered. We wait for the new document to be indexed */
waitForNewDocumentToBeIndexed(index, initialNumberOfDocuments);
Expand Down
Loading

0 comments on commit 79c3fae

Please sign in to comment.