[Security Solution] [Detection Engine] Fix exception comment flakiness #162807
Conversation
WafaaNasr
added
WIP
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @WafaaNasr |
| namespace_type: NamespaceType; | ||
| }; | ||
| } | ||
| describe('Add, copy comments in different exceptions type and validate sharing them between users', () => { |
There was a problem hiding this comment.
I would advocate for leaving at least one UI test for comments. Not necessary it should be complicated test with 2 different users. Just one user, creating one simple comment.
I think that would be beneficial to keep as it will give us confidence that UI is not broken
There was a problem hiding this comment.
Resort to only one user, should fix this issue? Is that correct? From observations
Navigating to Exceptions from the Rules table after logging in with the second user
That leaves us with:
Asserting that the exception table has two exceptions because the created exception doesn't appear immediately in the
table after the addition, as well asserting that comment is created successfully
Maybe page reload/retry might help with it?
There was a problem hiding this comment.
Thanks, Vitalii for the review :)
I would advocate for leaving at least one UI test for comments
We have UI coverage for the comments panel in Cypress in other tests one of them x-pack/plugins/security_solution/cypress/e2e/exceptions/alerts_table_flow/rule_exceptions/auto_populate_with_alert_data.cy.ts
Unfortunately, the test sometimes is quite flaky in the first steps because the Exception flyout keeps re-rendering which was the reason behind skipping the Exception Flyout validation suite as well.
There was a problem hiding this comment.
However, I agree with you that it is not clear in the issue that the issue is not related to the logic.
I will update it with the issue related to multiple rendering
There was a problem hiding this comment.
pack/plugins/security_solution/cypress/e2e/exceptions/alerts_table_flow/rule_exceptions/auto_populate_with_alert_data.cy.ts
is there tested functionality related to comments? I don't see any calls for addExceptionComment. It looks like we don't have any tests that cover this part of UI anymore after merge
There was a problem hiding this comment.
The addExceptionComment util I added it to open the comment panel and add a comment, and it is indeed unused now (so it should be removed) in the auto_populate_with_alert_data.cy.ts when the user opens the exception, the comment panel will be opened, and pre-filled that's why the test is not flaky because Cypress, in that case, will assert only that the comment panel is opened, the correct comments count is populated along with the text without clicking on the panel and going through the element got lost when the flyout gets rendered multiple times
| @@ -6,15 +6,16 @@ | |||
| */ | |||
|
|
|||
| import { ToolingLog } from '@kbn/tooling-log'; | |||
| import expect from '@kbn/expect'; | |||
| import expect from 'expect'; | |||
There was a problem hiding this comment.
Are we moving away from our local fork of expect?
There was a problem hiding this comment.
'm not certain if it has been recently recommended as a best practice, but I noticed that the new tests are using import expect from 'expect;' instead of @kbn/expect. Additionally, I had to utilize expect.arrayContaining and expect.objectContaining to perform assertions on specific data parts rather than the entire data, resulting in reduced test flakiness.🤞
kibanamachine
added
v8.10.0
backport:skip
elastic#162807) ## Summary - Addresses elastic#162565 - Converting the `Comments` related Cypress test to a FTR test according to these [observations](elastic#162565 (comment)) --------- Co-authored-by: kibanamachine <[email protected]>
WafaaNasr
added
v8.9.2
and removed
backport:skip
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…akiness (#162807) (#164621) # Backport This will backport the following commits from `main` to `8.9`: - [[Security Solution] [Detection Engine] Fix exception comment flakiness (#162807)](#162807) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Wafaa Nasr","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-08-01T13:58:40Z","message":"[Security Solution] [Detection Engine] Fix exception comment flakiness (#162807)\n\n## Summary\r\n\r\n- Addresses #162565 \r\n- Converting the `Comments` related Cypress test to a FTR test according\r\nto these\r\n[observations](https://github.com/elastic/kibana/issues/162565#issuecomment-1653410937)\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"a2275dd346b027db0224e7afa5df58158eb9b641","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["WIP","release_note:skip","test-failure-flaky","Team:Detection Engine","v8.10.0","v8.9.2"],"number":162807,"url":"https://github.com/elastic/kibana/pull/162807","mergeCommit":{"message":"[Security Solution] [Detection Engine] Fix exception comment flakiness (#162807)\n\n## Summary\r\n\r\n- Addresses #162565 \r\n- Converting the `Comments` related Cypress test to a FTR test according\r\nto these\r\n[observations](https://github.com/elastic/kibana/issues/162565#issuecomment-1653410937)\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"a2275dd346b027db0224e7afa5df58158eb9b641"}},"sourceBranch":"main","suggestedTargetBranches":["8.9"],"targetPullRequestStates":[{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/162807","number":162807,"mergeCommit":{"message":"[Security Solution] [Detection Engine] Fix exception comment flakiness (#162807)\n\n## Summary\r\n\r\n- Addresses #162565 \r\n- Converting the `Comments` related Cypress test to a FTR test according\r\nto these\r\n[observations](https://github.com/elastic/kibana/issues/162565#issuecomment-1653410937)\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"a2275dd346b027db0224e7afa5df58158eb9b641"}},{"branch":"8.9","label":"v8.9.2","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT-->
Summary
Commentsrelated Cypress test to a FTR test according to these observations