[Investigations] - Unskip and refactor discover state tests #173308

michaelolo24 · 2023-12-13T17:27:29Z

Dependent on: #173015

fixes Failing test: Security Solution Cypress.x-pack/test/security_solution_cypress/cypress/e2e/investigations/timelines/discover/discover_state·cy·ts - Discover State "before each" hook for "should remember kql query when navigating away and back to discover " "before each" hook for "should remember kql query when navigating away and back to discover " #165663
fixes Failing test: Security Solution Cypress.x-pack/test/security_solution_cypress/cypress/e2e/investigations/timelines/discover/discover_state·cy·ts - Discover State should remember dataView when navigating away and back to discover should remember dataView when navigating away and back to discover #165747

PhilippeOberti

I left a few comments of extra things to potentially cleanup

PhilippeOberti · 2023-12-18T17:38:57Z

..._cypress/cypress/e2e/investigations/timelines/esql/discover_timeline_state_integration.cy.ts

  createNewTimeline,
+  createTimelineOptionsPopoverBottomBar,


this method does not exist in this file

Rebase error, fixing

PhilippeOberti · 2023-12-18T17:40:42Z

..._cypress/cypress/e2e/investigations/timelines/esql/discover_timeline_state_integration.cy.ts

+      login();
+      visitWithTimeRange(ALERTS_URL);
+      createTimelineOptionsPopoverBottomBar();
+      goToEsqlTab();


diving into this function, I think we should remove the should('be.visible') here (Cypress already checks for visibility)

also I'm wondering if we could remove the about the delay 500? We're not using that delay in the expandable flyout Cypress tests and I don't remember seeing a test fail because of that

Will remove should('be.visible'), and I figure the delay was put there because of how long it can take for the elements in the timeline tabs to load, but if it is necessary, there are definitely better ways of doing it besides an explicit delay time. Will look into it

PhilippeOberti · 2023-12-18T17:43:13Z

..._cypress/cypress/e2e/investigations/timelines/esql/discover_timeline_state_integration.cy.ts

+    describe('save/restore', () => {
+      beforeEach(() => {
+        handleIntercepts();
+      });
      it('should be able create an empty timeline with default discover state', () => {
        addNameToTimelineAndSave('Timerange timeline');
        createNewTimeline();


diving into this function, we should be able to remove the should('be.visible') here (Cypress already checks for visibility)

same thing like 123, looking into the addDiscoverEsqlQuery function, should('be.visible') from here

also, line 136, looking into the openTimelineFromSettings function, I think we should be able to delete this line entirely

let's remove it from here as well, and also the force: true?

Just gonna remove all of them in the timeline related files

PhilippeOberti · 2023-12-18T17:50:17Z

x-pack/test/security_solution_cypress/cypress/tasks/timeline.ts

+export const confirmLeaveUnsavedTimeline = () => {
+  cy.get(DELETION_CONFIRMATION).click();
+};
+
+export const deleteTimeline = () => {
+  cy.get(TIMELINE_COLLAPSED_ITEMS_BTN).click();
+  cy.get(DELETE_TIMELINE_BTN).click();
+  cy.get(DELETION_CONFIRMATION).click();
+};
+


I'm getting errors where these constants are not defined

Rebase error, will address, thanks!

PhilippeOberti · 2023-12-18T17:51:48Z

..._cypress/cypress/e2e/investigations/timelines/esql/discover_timeline_state_integration.cy.ts

@@ -212,6 +203,7 @@ describe.skip(
        cy.wait(`@${TIMELINE_REQ_WITH_SAVED_SEARCH}`);
        openKibanaNavigation();
        navigateFromKibanaCollapsibleTo(STACK_MANAGEMENT_PAGE);
+        confirmLeaveUnsavedTimeline();
        cy.get(LOADING_INDICATOR).should('not.exist');
        goToSavedObjectSettings();


there is a chance we could remove line 11 and 12 of this goToSavedObjectSettings no?

Yep, will do

PhilippeOberti · 2023-12-18T17:52:31Z

..._cypress/cypress/e2e/investigations/timelines/esql/discover_timeline_state_integration.cy.ts

@@ -212,6 +203,7 @@ describe.skip(
        cy.wait(`@${TIMELINE_REQ_WITH_SAVED_SEARCH}`);
        openKibanaNavigation();
        navigateFromKibanaCollapsibleTo(STACK_MANAGEMENT_PAGE);
+        confirmLeaveUnsavedTimeline();
        cy.get(LOADING_INDICATOR).should('not.exist');
        goToSavedObjectSettings();
        cy.get(LOADING_INDICATOR).should('not.exist');


should we extract the line below into a task function? (cy.get(SAVED_OBJECTS_TAGS_FILTER).trigger('click');)

PhilippeOberti · 2023-12-18T17:56:21Z

..._cypress/cypress/e2e/investigations/timelines/esql/discover_timeline_state_integration.cy.ts

what do you think about removing the nested describes (line 105 describe('save/restore', () => { and line 194 describe('saved search', () => {)?
With clear text title I don't think the describe are necessary. They both have the same beforeEach code so that should work. It would make the file a bit easier to read.

Also I wonder if we could move the const handleIntercepts method above the top describe.

Yea, will move the handle intercepts up, but I lean leaving the nested describe blocks as while all the beforeEach logic is the same, the context of what each block is testing is different, but I'll rename them to make them clearer

I'm not a fan of using describe just to separate tests like this but I'm not totally against it either 😄

If we keep these nested describe, let's update the test titles to not contains the same string then? We probably don't need the repetition

Yep, I had updated the titles in the last commit :)

- The test title only mentions to check for title changes - Description changes seem to be buggy in general and are not important at this point.

… call is in progress

janmonschke · 2024-01-15T16:10:52Z

@kqualters-elastic Could you have a look at e770e35? I noticed that with the original fix, it would still sometimes create two saved searches.

kibana-ci · 2024-01-15T17:35:30Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: f9fe147

Failed CI Steps

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	11.4MB	11.4MB	+6.0B

History

💔 Build #187386 failed 5c762c3
💔 Build #187210 failed 1b8084c
💔 Build #184072 failed aee88e1
💔 Build #184065 failed bd67a22f39d490d9473b0a6122995bc5f39ce6ff

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

lgestc

just one nit about magic value used

lgestc · 2024-01-16T09:48:19Z

..._solution/public/common/components/discover_in_timeline/use_discover_in_timeline_actions.tsx

@@ -205,6 +205,10 @@ export const useDiscoverInTimelineActions = (
      } else {
        // If no saved search exists. Create a new saved search instance and associate it with the timeline.
        try {
+          // Make sure we're not creating a saved search while a previous creation call is in progress
+          if (status !== 'idle') {


isnt there some kind of enum we could use here?

@lgestc status comes from @tanstack/query-core and is union of string literals:

export type MutationStatus = 'idle' | 'loading' | 'success' | 'error'

So the type is checked, just not as an enum :)

kibanamachine · 2024-01-16T12:22:35Z

💔 All backports failed

Status	Branch	Result
❌	8.12	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 173308

Questions ?

Please refer to the Backport tool documentation

janmonschke · 2024-01-16T13:21:07Z

💚 All backports created successfully

Status	Branch	Result
✅	8.12

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…173308) Dependent on: elastic#173015 - fixes elastic#165663 - fixes elastic#165747 [Flaky Tests (100/100)](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4870) --------- Co-authored-by: Jan Monschke <[email protected]> (cherry picked from commit 1172c0e) # Conflicts: # x-pack/test/security_solution_cypress/cypress/tasks/common.ts

…173308) (#174914) # Backport This will backport the following commits from `main` to `8.12`: - [[Investigations] - Unskip and refactor discover state tests (#173308)](#173308)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  --------- Co-authored-by: Michael Olorunnisola <[email protected]>

michaelolo24 added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team labels Dec 13, 2023

michaelolo24 mentioned this pull request Dec 13, 2023

[Investigations] - Investigations Cypress Tracking #173098

Closed

25 tasks

michaelolo24 force-pushed the unskip-discover-state-tests branch from 512e901 to d258f4f Compare December 18, 2023 17:24

michaelolo24 marked this pull request as ready for review December 18, 2023 17:25

michaelolo24 requested a review from a team as a code owner December 18, 2023 17:25

PhilippeOberti reviewed Dec 18, 2023

View reviewed changes

michaelolo24 added 3 commits December 18, 2023 14:22

fix discover_state file and unskip

326536f

reintroduce task

7ab4b4b

fix rebase errors and pr feedback cleanup

aee88e1

michaelolo24 force-pushed the unskip-discover-state-tests branch from bd67a22 to aee88e1 Compare December 18, 2023 19:22

michaelolo24 mentioned this pull request Dec 18, 2023

[Investigations] Unskip dataproviders test #173183

Merged

janmonschke added 5 commits January 12, 2024 14:24

Merge branch 'main' into pr/173308

1b8084c

Merge branch 'main' into pr/173308

5c762c3

Remove description test and check for titles

232fae2

- The test title only mentions to check for title changes - Description changes seem to be buggy in general and are not important at this point.

Make sure we're not creating a saved search while a previous creation…

e770e35

… call is in progress

the saved search update sometimes takes longer, so let's wait

f9fe147

michaelolo24 requested a review from a team as a code owner January 15, 2024 15:08

janmonschke added v8.12.1 v8.1.3 v8.13.0 and removed v8.1.3 labels Jan 15, 2024

lgestc reviewed Jan 16, 2024

View reviewed changes

lgestc approved these changes Jan 16, 2024

View reviewed changes

janmonschke self-assigned this Jan 16, 2024

janmonschke merged commit 1172c0e into elastic:main Jan 16, 2024
43 of 44 checks passed

janmonschke mentioned this pull request Jan 16, 2024

[8.12] [Investigations] - Unskip and refactor discover state tests (#173308) #174914

Merged

kibanamachine added the v8.12.0 label Jan 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Investigations] - Unskip and refactor discover state tests #173308

[Investigations] - Unskip and refactor discover state tests #173308

michaelolo24 commented Dec 13, 2023 •

edited by kibanamachine

Loading

PhilippeOberti left a comment •

edited

Loading

PhilippeOberti Dec 18, 2023

michaelolo24 Dec 18, 2023

PhilippeOberti Dec 18, 2023

PhilippeOberti Dec 18, 2023

michaelolo24 Dec 18, 2023

PhilippeOberti Dec 18, 2023

PhilippeOberti Dec 18, 2023

PhilippeOberti Dec 18, 2023

PhilippeOberti Dec 18, 2023

michaelolo24 Dec 18, 2023

PhilippeOberti Dec 18, 2023

michaelolo24 Dec 18, 2023

PhilippeOberti Dec 18, 2023

michaelolo24 Dec 18, 2023

PhilippeOberti Dec 18, 2023

PhilippeOberti Dec 18, 2023

michaelolo24 Dec 18, 2023

PhilippeOberti Dec 18, 2023

michaelolo24 Dec 20, 2023

janmonschke commented Jan 15, 2024

kibana-ci commented Jan 15, 2024 •

edited

Loading

lgestc left a comment

lgestc Jan 16, 2024

janmonschke Jan 16, 2024

kibanamachine commented Jan 16, 2024

janmonschke commented Jan 16, 2024

[Investigations] - Unskip and refactor discover state tests #173308

[Investigations] - Unskip and refactor discover state tests #173308

Conversation

michaelolo24 commented Dec 13, 2023 • edited by kibanamachine Loading

PhilippeOberti left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janmonschke commented Jan 15, 2024

kibana-ci commented Jan 15, 2024 • edited Loading

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

History

lgestc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Jan 16, 2024

💔 All backports failed

Manual backport

Questions ?

janmonschke commented Jan 16, 2024

💚 All backports created successfully

Questions ?

michaelolo24 commented Dec 13, 2023 •

edited by kibanamachine

Loading

PhilippeOberti left a comment •

edited

Loading

kibana-ci commented Jan 15, 2024 •

edited

Loading