[SecuritySolution] Fix flaky timeline creation tests (role-based issues) #173413

janmonschke · 2023-12-14T17:47:35Z

Summary

The login as t1_analyst was not working sometimes. In order to reduce flakiness, we moved the role-based login to the test itself.

The flaky test runner also showed that in some cases, we're failing to open the timeline in the tests. The click would perform, but somehow the timeline flyout wouldn't be open. Therefore we added some retry mechanisms to the opening logic (729e5ce).

Also:

removes the unnecessary context calls
updates the test titles to use the should convention

Flaky test runner result (200/200)

fixes #173339

- removes the unnecessary `context` calls - updates the test titles to use the `should` convention

janmonschke · 2023-12-14T20:35:28Z

/ci

elasticmachine · 2024-01-08T14:10:36Z

Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations)

janmonschke · 2024-01-08T14:35:15Z

/ci

PhilippeOberti

looks great overall, left a few comments or potential improvements

x-pack/test/security_solution_cypress/cypress/e2e/investigations/timelines/creation.cy.ts

PhilippeOberti · 2024-01-08T20:59:16Z

x-pack/test/security_solution_cypress/cypress/e2e/investigations/timelines/creation.cy.ts

@@ -126,7 +115,8 @@ describe.skip('Timelines', { tags: ['@ess', '@serverless'] }, (): void => {
      .should('have.text', getTimeline().notes);


looking at the addNotesToTimeline implementation something is a bit fishy:

first the goToNotesTab returns a cy.get but nothing is using that so it could be removed

second is at the end of the implementation we navigate to the query tab (goToQueryTab) then back to the notes tab (goToNotesTab). This seems like way too much logic happening in one task...

I think we could extract the getToNotesTab out, so that it's super clear and developers have to intentionally know what they're doing.
Also we should understand why we need to navigate out then back. Is that an issue with the notes tab not refreshing? Is that still necessary?

first the goToNotesTab returns a cy.get but nothing is using that so it could be removed

I'm not sure what you mean. It's not returning the result of cy.get. It uses it to perform further queries (.find('.euiBadge__text').invoke('text')...)

we navigate to the query tab (goToQueryTab) then back to the notes tab (goToNotesTab)

Yeah, that looks fishy. Let me remove that and see if this has an affect on any of the notes tests. This should not be necessary anymore. That code was added 3 years ago, so a lot has happened since then.

Removing the last two gotos doesn't seem to have an impact on the timeline creation tests neither on the notes tests (which I had to unskip to test)

I'm not sure what you mean. It's not returning the result of cy.get. It uses it to perform further queries

I meant within the goToNotesTab implementation, the last line returns like this

return cy.get(NOTES_TAB_BUTTON);

but everywhere we call it we call it like that

goToNotesTab();`

so we don't actually use the constant returned. Shouldn't we remove that last return line?

x-pack/test/security_solution_cypress/cypress/e2e/investigations/timelines/creation.cy.ts

janmonschke · 2024-01-09T16:02:33Z

/ci

PhilippeOberti

thanks for making all the changes, code looks great! I left a small comment but if the build is green feel free to ignore and merge. It's not super important and could be done within another PR later!

kibana-ci · 2024-01-09T17:08:46Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: c4bf8a9

Failed CI Steps

FTR Configs #17

Test Failures

[job] [logs] FTR Configs #17 / security APIs - Session Concurrent Limit Session Concurrent Limit cleanup should properly clean up sessions that exceeded concurrent session limit even for multiple providers

Metrics [docs]

✅ unchanged

History

💛 Build #186512 was flaky e0577d4
💚 Build #186392 succeeded 76ddd64
💔 Build #183727 failed 3b42d57

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

cc @janmonschke

…es) (elastic#173413)

## Summary Unskips the fullscreen timeline tests. Looking at the issue described in #172547, it appears that the timeline was somehow not opening correctly in some cases. This flakiness has been removed in #173413, so it should be possible to unskip these tests. In addition to unskipping the tests, an unnecessary task execution was removed and two force clicks were cleaned up. [Flaky test runner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4834) (200/200) fixes #172547

## Summary Unskips the fullscreen timeline tests. Looking at the issue described in elastic#172547, it appears that the timeline was somehow not opening correctly in some cases. This flakiness has been removed in elastic#173413, so it should be possible to unskip these tests. In addition to unskipping the tests, an unnecessary task execution was removed and two force clicks were cleaned up. [Flaky test runner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4834) (200/200) fixes elastic#172547

janmonschke added 2 commits December 14, 2023 18:42

fix login flakiness by moving the role-based login to the test

04655e2

- removes the unnecessary `context` calls - updates the test titles to use the `should` convention

prepare flaky test runner

e735cd6

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

janmonschke self-assigned this Dec 14, 2023

janmonschke changed the title ~~[SecuritySolution] Fix flaky timeline creation tests (role-based isssues)~~ [SecuritySolution] Fix flaky timeline creation tests (role-based issues) Dec 14, 2023

janmonschke mentioned this pull request Dec 14, 2023

[Investigations] - Investigations Cypress Tracking #173098

Closed

25 tasks

janmonschke added 3 commits December 14, 2023 20:05

enable video capture of failed tests

99d3d20

log pr labels

a0c3e8e

fix video capture

3b42d57

janmonschke and others added 7 commits December 15, 2023 10:08

revert video capture

1fe2103

logout before changing to read-only role

8769a06

make sure the page has loaded

6b2310b

remove logout

8c082f7

adjustment

6566334

Merge branch 'main' into security/fix-flaky-tl-creation-tests-again

3fcb9a9

undo test runner changes

76ddd64

janmonschke marked this pull request as ready for review January 8, 2024 14:10

janmonschke requested a review from a team as a code owner January 8, 2024 14:10

PhilippeOberti reviewed Jan 8, 2024

View reviewed changes

clean up

e0577d4

janmonschke marked this pull request as draft January 9, 2024 08:17

janmonschke added 5 commits January 9, 2024 09:18

prepare flaky test runner

2164a25

fixing flakiness

74aaabf

Make "unsaved" matcher flake resistant

0e97a2f

Retry to open/create the timeline

73b2f9f

make openTimelineUsingToggle retry when the timeline hasn't opened

9d1f16a

janmonschke added 3 commits January 9, 2024 13:30

replace createTimeline with openTimelineUsingToggle

729e5ce

remove unused imports

34af509

undo flaky test runner setup

c4bf8a9

janmonschke marked this pull request as ready for review January 9, 2024 16:02

PhilippeOberti approved these changes Jan 9, 2024

View reviewed changes

PhilippeOberti merged commit a100cbb into elastic:main Jan 9, 2024
38 checks passed

kibanamachine added v8.13.0 backport:skip This commit does not require backporting labels Jan 9, 2024

nreese pushed a commit to nreese/kibana that referenced this pull request Jan 10, 2024

[SecuritySolution] Fix flaky timeline creation tests (role-based issu…

2c843df

…es) (elastic#173413)

delanni pushed a commit to delanni/kibana that referenced this pull request Jan 11, 2024

[SecuritySolution] Fix flaky timeline creation tests (role-based issu…

20227a7

…es) (elastic#173413)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SecuritySolution] Fix flaky timeline creation tests (role-based issues) #173413

[SecuritySolution] Fix flaky timeline creation tests (role-based issues) #173413

janmonschke commented Dec 14, 2023 •

edited

Loading

janmonschke commented Dec 14, 2023

elasticmachine commented Jan 8, 2024

janmonschke commented Jan 8, 2024

PhilippeOberti left a comment •

edited

Loading

PhilippeOberti Jan 8, 2024

janmonschke Jan 9, 2024 •

edited

Loading

janmonschke Jan 9, 2024

PhilippeOberti Jan 9, 2024

janmonschke commented Jan 9, 2024

PhilippeOberti left a comment

kibana-ci commented Jan 9, 2024

		@@ -126,7 +115,8 @@ describe.skip('Timelines', { tags: ['@ess', '@serverless'] }, (): void => {
		.should('have.text', getTimeline().notes);

[SecuritySolution] Fix flaky timeline creation tests (role-based issues) #173413

[SecuritySolution] Fix flaky timeline creation tests (role-based issues) #173413

Conversation

janmonschke commented Dec 14, 2023 • edited Loading

Summary

janmonschke commented Dec 14, 2023

elasticmachine commented Jan 8, 2024

janmonschke commented Jan 8, 2024

PhilippeOberti left a comment • edited Loading

Choose a reason for hiding this comment

PhilippeOberti Jan 8, 2024

Choose a reason for hiding this comment

janmonschke Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

janmonschke Jan 9, 2024

Choose a reason for hiding this comment

PhilippeOberti Jan 9, 2024

Choose a reason for hiding this comment

janmonschke commented Jan 9, 2024

PhilippeOberti left a comment

Choose a reason for hiding this comment

kibana-ci commented Jan 9, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

History

janmonschke commented Dec 14, 2023 •

edited

Loading

PhilippeOberti left a comment •

edited

Loading

janmonschke Jan 9, 2024 •

edited

Loading