Skip to content

Commit

Permalink
[Security Solution] Fix value lists tests flakiness (elastic#164253)
Browse files Browse the repository at this point in the history 
**Fixes:** elastic#164056

## Summary

This PR fixes [value_lists.cy.ts](https://github.com/elastic/kibana/blob/main/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/value_lists/value_lists.cy.ts) tests flakiness.

## The flakiness reason

Value list items are processed in a bulk via bulk creation and `refresh=wait_for` is [used](https://github.com/elastic/kibana/blob/main/x-pack/plugins/lists/server/services/items/create_list_items_bulk.ts#L87). The problem it returns sometimes earlier than data is available. [Bulk API docs](https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-bulk.html#bulk-refresh) say the following

> Only the shards that receive the bulk request will be affected by refresh. Imagine a _bulk?refresh=wait_for request with three documents in it that happen to be routed to different shards in an index with five shards. The request will only wait for those three shards to refresh. The other two shards that make up the index do not participate in the _bulk request at all.

While (it seems) only one shard is used in tests but it still cause issues (approx. 1 test per 50 fails) so adding explicit index refresh helps to get rid of flakiness.

## Flaky test runner

[value_lists.cy.ts (150 runs)](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2924) 🟢

(cherry picked from commit d34c845)
  • Loading branch information
@maximpn
maximpn committed Aug 21, 2023
1 parent 1a1bdb7 commit 443ce4f
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 77 deletions.
126 changes: 77 additions & 49 deletions ...st/security_solution_cypress/cypress/e2e/detection_response/value_lists/value_lists.cy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,33 +17,35 @@ import {
selectValueListsFile,
uploadValueList,
selectValueListType,
deleteAllValueListsFromUI,
closeValueListsModal,
importValueList,
deleteValueListsFile,
exportValueList,
waitForListsIndex,
deleteValueLists,
} from '../../../tasks/lists';
import {
VALUE_LISTS_TABLE,
VALUE_LISTS_ROW,
VALUE_LISTS_MODAL_ACTIVATOR,
} from '../../../screens/lists';
import { refreshIndex } from '../../../tasks/api_calls/elasticsearch';

const TEXT_LIST_FILE_NAME = 'value_list.txt';
const IPS_LIST_FILE_NAME = 'ip_list.txt';
const CIDRS_LIST_FILE_NAME = 'cidr_list.txt';

describe('value lists', () => {
describe('management modal', { tags: [tag.ESS, tag.SERVERLESS] }, () => {
beforeEach(() => {
login();
deleteValueLists([TEXT_LIST_FILE_NAME, IPS_LIST_FILE_NAME, CIDRS_LIST_FILE_NAME]);
createListsIndex();
visitWithoutDateRange(DETECTIONS_RULE_MANAGEMENT_URL);
waitForListsIndex();
waitForValueListsModalToBeLoaded();
});

afterEach(() => {
deleteAllValueListsFromUI();
});

it('can open and close the modal', () => {
openValueListsModal();
closeValueListsModal();
Expand All @@ -55,121 +57,122 @@ describe('value lists', () => {
});

it('creates a "keyword" list from an uploaded file', () => {
const listName = 'value_list.txt';
selectValueListType('keyword');
selectValueListsFile(listName);
selectValueListsFile(TEXT_LIST_FILE_NAME);
uploadValueList();

cy.get(VALUE_LISTS_TABLE)
.find(VALUE_LISTS_ROW)
.should(($row) => {
expect($row.text()).to.contain(listName);
expect($row.text()).to.contain(TEXT_LIST_FILE_NAME);
expect($row.text()).to.contain('Keywords');
});
});

it('creates a "text" list from an uploaded file', () => {
const listName = 'value_list.txt';
selectValueListType('text');
selectValueListsFile(listName);
selectValueListsFile(TEXT_LIST_FILE_NAME);
uploadValueList();

cy.get(VALUE_LISTS_TABLE)
.find(VALUE_LISTS_ROW)
.should(($row) => {
expect($row.text()).to.contain(listName);
expect($row.text()).to.contain(TEXT_LIST_FILE_NAME);
expect($row.text()).to.contain('Text');
});
});

it('creates a "ip" list from an uploaded file', () => {
const listName = 'ip_list.txt';
selectValueListType('ip');
selectValueListsFile(listName);
selectValueListsFile(IPS_LIST_FILE_NAME);
uploadValueList();

cy.get(VALUE_LISTS_TABLE)
.find(VALUE_LISTS_ROW)
.should(($row) => {
expect($row.text()).to.contain(listName);
expect($row.text()).to.contain(IPS_LIST_FILE_NAME);
expect($row.text()).to.contain('IP addresses');
});
});

it('creates a "ip_range" list from an uploaded file', () => {
const listName = 'cidr_list.txt';
selectValueListType('ip_range');
selectValueListsFile(listName);
selectValueListsFile(CIDRS_LIST_FILE_NAME);
uploadValueList();

cy.get(VALUE_LISTS_TABLE)
.find(VALUE_LISTS_ROW)
.should(($row) => {
expect($row.text()).to.contain(listName);
expect($row.text()).to.contain(CIDRS_LIST_FILE_NAME);
expect($row.text()).to.contain('IP ranges');
});
});
});

describe('delete list types', () => {
it('deletes a "keyword" list from an uploaded file', () => {
const listName = 'value_list.txt';
importValueList(listName, 'keyword');
importValueList(TEXT_LIST_FILE_NAME, 'keyword');
openValueListsModal();
deleteValueListsFile(listName);
deleteValueListsFile(TEXT_LIST_FILE_NAME);
cy.get(VALUE_LISTS_TABLE)
.find(VALUE_LISTS_ROW)
.should(($row) => {
expect($row.text()).not.to.contain(listName);
expect($row.text()).not.to.contain(TEXT_LIST_FILE_NAME);
});
});

it('deletes a "text" list from an uploaded file', () => {
const listName = 'value_list.txt';
importValueList(listName, 'text');
importValueList(TEXT_LIST_FILE_NAME, 'text');
openValueListsModal();
deleteValueListsFile(listName);
deleteValueListsFile(TEXT_LIST_FILE_NAME);
cy.get(VALUE_LISTS_TABLE)
.find(VALUE_LISTS_ROW)
.should(($row) => {
expect($row.text()).not.to.contain(listName);
expect($row.text()).not.to.contain(TEXT_LIST_FILE_NAME);
});
});

it('deletes a "ip" from an uploaded file', () => {
const listName = 'ip_list.txt';
importValueList(listName, 'ip');
importValueList(IPS_LIST_FILE_NAME, 'ip');
openValueListsModal();
deleteValueListsFile(listName);
deleteValueListsFile(IPS_LIST_FILE_NAME);
cy.get(VALUE_LISTS_TABLE)
.find(VALUE_LISTS_ROW)
.should(($row) => {
expect($row.text()).not.to.contain(listName);
expect($row.text()).not.to.contain(IPS_LIST_FILE_NAME);
});
});

it('deletes a "ip_range" from an uploaded file', () => {
const listName = 'cidr_list.txt';
importValueList(listName, 'ip_range', ['192.168.100.0']);
importValueList(CIDRS_LIST_FILE_NAME, 'ip_range', ['192.168.100.0']);
openValueListsModal();
deleteValueListsFile(listName);
deleteValueListsFile(CIDRS_LIST_FILE_NAME);
cy.get(VALUE_LISTS_TABLE)
.find(VALUE_LISTS_ROW)
.should(($row) => {
expect($row.text()).not.to.contain(listName);
expect($row.text()).not.to.contain(CIDRS_LIST_FILE_NAME);
});
});
});

describe('export list types', () => {
it('exports a "keyword" list from an uploaded file', () => {
const listName = 'value_list.txt';
cy.intercept('POST', `/api/lists/items/_export?list_id=${listName}`).as('exportList');
importValueList('value_list.txt', 'keyword');
cy.intercept('POST', `/api/lists/items/_export?list_id=${TEXT_LIST_FILE_NAME}`).as(
'exportList'
);
importValueList(TEXT_LIST_FILE_NAME, 'keyword');

// Importing value lists includes bulk creation of list items with refresh=wait_for
// While it should wait for data update and return after that it's not always a case with bulk operations.
// Sometimes list items are empty making this test flaky.
// To fix it refresh used list items index (for the default space)
refreshIndex('.items-default');

openValueListsModal();
exportValueList();

cy.wait('@exportList').then(({ response }) => {
cy.fixture(listName).then((list: string) => {
cy.fixture(TEXT_LIST_FILE_NAME).then((list: string) => {
const [lineOne, lineTwo] = list.split('\n');
expect(response?.body).to.contain(lineOne);
expect(response?.body).to.contain(lineTwo);
Expand All @@ -178,13 +181,22 @@ describe('value lists', () => {
});

it('exports a "text" list from an uploaded file', () => {
const listName = 'value_list.txt';
cy.intercept('POST', `/api/lists/items/_export?list_id=${listName}`).as('exportList');
importValueList(listName, 'text');
cy.intercept('POST', `/api/lists/items/_export?list_id=${TEXT_LIST_FILE_NAME}`).as(
'exportList'
);
importValueList(TEXT_LIST_FILE_NAME, 'text');

// Importing value lists includes bulk creation of list items with refresh=wait_for
// While it should wait for data update and return after that it's not always a case with bulk operations.
// Sometimes list items are empty making this test flaky.
// To fix it refresh used list items index (for the default space)
refreshIndex('.items-default');

openValueListsModal();
exportValueList();

cy.wait('@exportList').then(({ response }) => {
cy.fixture(listName).then((list: string) => {
cy.fixture(TEXT_LIST_FILE_NAME).then((list: string) => {
const [lineOne, lineTwo] = list.split('\n');
expect(response?.body).to.contain(lineOne);
expect(response?.body).to.contain(lineTwo);
Expand All @@ -193,13 +205,21 @@ describe('value lists', () => {
});

it('exports a "ip" list from an uploaded file', () => {
const listName = 'ip_list.txt';
cy.intercept('POST', `/api/lists/items/_export?list_id=${listName}`).as('exportList');
importValueList(listName, 'ip');
cy.intercept('POST', `/api/lists/items/_export?list_id=${IPS_LIST_FILE_NAME}`).as(
'exportList'
);
importValueList(IPS_LIST_FILE_NAME, 'ip');

// Importing value lists includes bulk creation of list items with refresh=wait_for
// While it should wait for data update and return after that it's not always a case with bulk operations.
// Sometimes list items are empty making this test flaky.
// To fix it refresh used list items index (for the default space)
refreshIndex('.items-default');

openValueListsModal();
exportValueList();
cy.wait('@exportList').then(({ response }) => {
cy.fixture(listName).then((list: string) => {
cy.fixture(IPS_LIST_FILE_NAME).then((list: string) => {
const [lineOne, lineTwo] = list.split('\n');
expect(response?.body).to.contain(lineOne);
expect(response?.body).to.contain(lineTwo);
Expand All @@ -208,13 +228,21 @@ describe('value lists', () => {
});

it('exports a "ip_range" list from an uploaded file', () => {
const listName = 'cidr_list.txt';
cy.intercept('POST', `/api/lists/items/_export?list_id=${listName}`).as('exportList');
importValueList(listName, 'ip_range', ['192.168.100.0']);
cy.intercept('POST', `/api/lists/items/_export?list_id=${CIDRS_LIST_FILE_NAME}`).as(
'exportList'
);
importValueList(CIDRS_LIST_FILE_NAME, 'ip_range', ['192.168.100.0']);

// Importing value lists includes bulk creation of list items with refresh=wait_for
// While it should wait for data update and return after that it's not always a case with bulk operations.
// Sometimes list items are empty making this test flaky.
// To fix it refresh used list items index (for the default space)
refreshIndex('.items-default');

openValueListsModal();
exportValueList();
cy.wait('@exportList').then(({ response }) => {
cy.fixture(listName).then((list: string) => {
cy.fixture(CIDRS_LIST_FILE_NAME).then((list: string) => {
const [lineOne] = list.split('\n');
expect(response?.body).to.contain(lineOne);
});
Expand Down
35 changes: 7 additions & 28 deletions x-pack/test/security_solution_cypress/cypress/tasks/lists.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
VALUE_LIST_CLOSE_BUTTON,
VALUE_LIST_DELETE_BUTTON,
VALUE_LIST_EXPORT_BUTTON,
VALUE_LIST_FILES,
VALUE_LIST_FILE_PICKER,
VALUE_LIST_FILE_UPLOAD_BUTTON,
VALUE_LIST_TYPE_SELECTOR,
Expand Down Expand Up @@ -73,9 +72,14 @@ export const exportValueList = (): Cypress.Chainable<JQuery<HTMLElement>> => {

/**
* Given an array of value lists this will delete them all using Cypress Request and the lists REST API
*
* If a list doesn't exist it ignores the error.
*
* Ref: https://www.elastic.co/guide/en/security/current/lists-api-delete-container.html
*/
const deleteValueLists = (lists: string[]): Array<Cypress.Chainable<Cypress.Response<unknown>>> => {
export const deleteValueLists = (
lists: string[]
): Array<Cypress.Chainable<Cypress.Response<unknown>>> => {
return lists.map((list) => deleteValueList(list));
};

Expand All @@ -88,6 +92,7 @@ const deleteValueList = (list: string): Cypress.Chainable<Cypress.Response<unkno
method: 'DELETE',
url: `api/lists?id=${list}`,
headers: { 'kbn-xsrf': 'delete-lists', 'x-elastic-internal-origin': 'security-solution' },
failOnStatusCode: false,
});
};

Expand Down Expand Up @@ -149,29 +154,3 @@ export const importValueList = (
) => {
return cy.fixture<string>(file).then((data) => uploadListItemData(file, type, data));
};

/**
* If you are on the value lists from the UI, this will loop over all the HTML elements
* that have action-delete-value-list-${list_name} and delete all of those value lists
* using Cypress Request and the lists REST API.
* If the UI does not contain any value based lists this will not fail. If the UI does
* contain value based lists but the backend does not return a success on DELETE then this
* will cause errors.
* Ref: https://www.elastic.co/guide/en/security/current/lists-api-delete-container.html
*/
export const deleteAllValueListsFromUI = (): Array<
Cypress.Chainable<Cypress.Response<unknown>>
> => {
const lists = Cypress.$(VALUE_LIST_FILES)
.toArray()
.reduce<string[]>((accum, $el) => {
const attribute = $el.getAttribute('data-test-subj');
if (attribute != null) {
const list = attribute.substr('data-test-subj-value-list'.length);
return [...accum, list];
} else {
return accum;
}
}, []);
return deleteValueLists(lists);
};

0 comments on commit 443ce4f

Please sign in to comment.