-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix flaky tests #175965
Fix flaky tests #175965
Conversation
/ci |
/ci |
/ci |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detection-engine (Team:Detection Engine) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
But does 25 tests enough to reproduce flakiness?
@nkhristinin Thank for the review! I re-run flaky test runner with the max time possible which is 97. Updated links in description. |
@@ -54,6 +53,7 @@ describe.skip('Alert user assignment - ESS & Serverless', { tags: ['@ess', '@ser | |||
login(ROLES.soc_manager); | |||
login(ROLES.detections_admin); | |||
login(ROLES.platform_engineer); | |||
login(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this login
method needed? Theoretically I believe it should not, in all the tests where the login()
has been added to the before hook, we are calling on the beforeEach, loadPageAs()
. As part of the implementation, that method, a login is performed so having that one sounds redundant. If it is needed, it would be nice to understand why to make sure we are not masking a possible issue :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that's a good point! We do not really need login()
here.
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @e40pud |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!! Thanks for bring these tests back to life :)
## Summary These changes fix flaky alert assignments cypress tests: - elastic#173429 - elastic#172520 - elastic#172557 @MadameSheema mentioned that there were a lot of changes around login functionality recently which could have fixed original issues. We agreed that we will run a flaky test runner and if everything works fine we would un-skip tests and monitor them after the merge. ### Checklist Delete any items that are not applicable to this PR. - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ESS 97 times](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5064) - [Serverless 97 times](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5052)
## Summary These changes fix flaky alert assignments cypress tests: - elastic#173429 - elastic#172520 - elastic#172557 @MadameSheema mentioned that there were a lot of changes around login functionality recently which could have fixed original issues. We agreed that we will run a flaky test runner and if everything works fine we would un-skip tests and monitor them after the merge. ### Checklist Delete any items that are not applicable to this PR. - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ESS 97 times](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5064) - [Serverless 97 times](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5052)
## Summary These changes fix flaky alert assignments cypress tests: - elastic#173429 - elastic#172520 - elastic#172557 @MadameSheema mentioned that there were a lot of changes around login functionality recently which could have fixed original issues. We agreed that we will run a flaky test runner and if everything works fine we would un-skip tests and monitor them after the merge. ### Checklist Delete any items that are not applicable to this PR. - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ESS 97 times](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5064) - [Serverless 97 times](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5052)
Summary
These changes fix flaky alert assignments cypress tests:
@MadameSheema mentioned that there were a lot of changes around login functionality recently which could have fixed original issues. We agreed that we will run a flaky test runner and if everything works fine we would un-skip tests and monitor them after the merge.
Checklist
Delete any items that are not applicable to this PR.