From 443ce4fda4fe7987688d3cf55883c1b3f9ea15c2 Mon Sep 17 00:00:00 2001 From: Maxim Palenov Date: Mon, 21 Aug 2023 16:31:21 +0200 Subject: [PATCH] [Security Solution] Fix value lists tests flakiness (#164253) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Fixes:** https://github.com/elastic/kibana/issues/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 d34c845955e2cb1cd6bae630ac3d0d551544f916) --- .../value_lists/value_lists.cy.ts | 126 +++++++++++------- .../cypress/tasks/lists.ts | 35 +---- 2 files changed, 84 insertions(+), 77 deletions(-) diff --git a/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/value_lists/value_lists.cy.ts b/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/value_lists/value_lists.cy.ts index 60b0b02d79726..697ccadbdbc7f 100644 --- a/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/value_lists/value_lists.cy.ts +++ b/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/value_lists/value_lists.cy.ts @@ -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(); @@ -55,57 +57,53 @@ 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'); }); }); @@ -113,63 +111,68 @@ describe('value lists', () => { 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); @@ -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); @@ -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); @@ -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); }); diff --git a/x-pack/test/security_solution_cypress/cypress/tasks/lists.ts b/x-pack/test/security_solution_cypress/cypress/tasks/lists.ts index d160f4c2deb42..e9dd3d882b429 100644 --- a/x-pack/test/security_solution_cypress/cypress/tasks/lists.ts +++ b/x-pack/test/security_solution_cypress/cypress/tasks/lists.ts @@ -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, @@ -73,9 +72,14 @@ export const exportValueList = (): Cypress.Chainable> => { /** * 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>> => { +export const deleteValueLists = ( + lists: string[] +): Array>> => { return lists.map((list) => deleteValueList(list)); }; @@ -88,6 +92,7 @@ const deleteValueList = (list: string): Cypress.Chainable { return cy.fixture(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> -> => { - const lists = Cypress.$(VALUE_LIST_FILES) - .toArray() - .reduce((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); -};