elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.63k stars 8.22k forks source link

[Security Solution] Fix flaky tests: common tasks #161505

Open banderror opened 1 year ago

banderror commented 1 year ago

Epic: https://github.com/elastic/kibana/issues/153633

Summary

Fix issues causing flakiness in Cypress or API integration tests, common to many tests for Security Solution.

Sub-tasks

### Tracking flaky tests
- [ ] Split Cypress tests into multiple Cypress configs for each area to be able to run them separately with the [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
- [x] Update PR template - add a checklist item to use the [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) on the tests changed in the PR ([PR](https://github.com/elastic/kibana/pull/171813))
- [x] Ask teams to start using the [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) on most PRs
- [x] Start tracking the `#kibana-unsupported-ftrs-alerts` channel - this one shows flaky tests on the `main` branch
### Cypress APIs
- [ ] https://github.com/elastic/kibana/issues/158713
### Detection rules
- [ ] https://github.com/elastic/kibana/issues/147377
- [ ] https://github.com/elastic/kibana/issues/153689
- [ ] https://github.com/elastic/kibana/issues/153692
- [ ] https://github.com/elastic/kibana/issues/164513
elasticmachine commented 1 year ago

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

elasticmachine commented 1 year ago

Pinging @elastic/security-solution (Team: SecuritySolution)

elasticmachine commented 1 year ago

Pinging @elastic/security-detections-response (Team:Detections and Resp)

VidhiRambhia commented 1 year ago

Hi @banderror, I am interested in this issue. Can I pick it up?

banderror commented 12 months ago

Hey @VidhiRambhia 👋 , great to see people from the community willing to make code contributions to Kibana! Community contributions are always welcome at Elastic.

Since this specific ticket requires a good understanding of the tests owned by @elastic/security-detection-rule-management and @elastic/security-detection-engine teams, I'd not personally recommend working on just about any task tracked in the description. Maybe you could pick one of these tasks because they are less involved:

VidhiRambhia commented 12 months ago

Hi @banderror,

That makes sense. I'll take a look at these tasks and keep you posted! Thank you!

VidhiRambhia commented 11 months ago

Hi @banderror, I have addressed the 'Update PR template - add a checklist item to use the Flaky Test Runner on the tests changed in the PR' subtask item in this PR - https://github.com/elastic/kibana/pull/171813

For the second issue - https://github.com/elastic/kibana/issues/158713, it seems like the linter rule has been set to 'warn'. Should it be changed to 'error'? https://github.com/VidhiRambhia/kibana/blob/f9c9722c6f568d5e730f0b2826382ca0f5508e1e/x-pack/test/security_solution_cypress/cypress/.eslintrc.json

For the third issue, I have raised a draft PR - https://github.com/elastic/kibana/pull/171814

Please let me know your thoughts on these!

banderror commented 11 months ago

@VidhiRambhia Thank you so much for working on these!

For the second issue - https://github.com/elastic/kibana/issues/158713, it seems like the linter rule has been set to 'warn'. Should it be changed to 'error'? https://github.com/VidhiRambhia/kibana/blob/f9c9722c6f568d5e730f0b2826382ca0f5508e1e/x-pack/test/security_solution_cypress/cypress/.eslintrc.json

Yeah, I think we can start with making it an "error" and seeing how many linting errors we'd get. If 0 or just a few, we could try to fix them and merge the change.

VidhiRambhia commented 11 months ago

@VidhiRambhia Thank you so much for working on these!

For the second issue - #158713, it seems like the linter rule has been set to 'warn'. Should it be changed to 'error'? https://github.com/VidhiRambhia/kibana/blob/f9c9722c6f568d5e730f0b2826382ca0f5508e1e/x-pack/test/security_solution_cypress/cypress/.eslintrc.json

Yeah, I think we can start with making it an "error" and seeing how many linting errors we'd get. If 0 or just a few, we could try to fix them and merge the change.

That makes sense, I'll try this!

MadameSheema commented 11 months ago

Yeah, I think we can start with making it an "error" and seeing how many linting errors we'd get. If 0 or just a few, we could try to fix them and merge the change.

@banderror we should not do that until we have get rid off the force true from all the code and we propose an alternative :)

banderror commented 11 months ago

@MadameSheema Alternatives will be case-by-case and will likely require fixing the app's react components, I presume. At this point, I'd like to see how many force: true instances we have at all.

VidhiRambhia commented 11 months ago

@VidhiRambhia Thank you so much for working on these!

For the second issue - #158713, it seems like the linter rule has been set to 'warn'. Should it be changed to 'error'? https://github.com/VidhiRambhia/kibana/blob/f9c9722c6f568d5e730f0b2826382ca0f5508e1e/x-pack/test/security_solution_cypress/cypress/.eslintrc.json

Yeah, I think we can start with making it an "error" and seeing how many linting errors we'd get. If 0 or just a few, we could try to fix them and merge the change.

Hi @banderror,

I changed the linter rule for cypress/no-force to 'error'. - https://github.com/elastic/kibana/pull/172396 After running npx eslint in the security_solution_cypress folder, I found 181 errors related to the use of cypress { force: true }. Here is the error log log.txt

banderror commented 11 months ago

Thanks @VidhiRambhia, this is useful information. I think 181 linting errors might be too many for trying to fix all of them in one go and making this linter rule generate errors instead of warnings. Let's keep it as is for now.