Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Open
4 of 9 tasks
Tracked by #153633
, #153664
...
banderror opened this issue Jul 8, 2023 · 13 comments
Open
4 of 9 tasks
Tracked by #153633
, #153664
...

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

banderror opened this issue Jul 8, 2023 · 13 comments
Labels
Meta Team:Detection Engine Security Solution Detection Engine Area Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting Security Solution Threat Hunting Team technical debt Improvement of the software architecture and operational architecture test_ui_functional test test-api-integration

Comments

@banderror
Copy link
Contributor

banderror commented Jul 8, 2023 •
edited

Epic: #153633

Summary

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

Sub-tasks

Tracking flaky tests
Beta Give feedback

Cypress APIs
Beta Give feedback
  1. [Security Solution] Add a linter rule to discourage forced Cypress actions #158713
    Team: SecuritySolution Team:Detection Rule Management Team:Detections and Resp es-lint technical debt test
    maximpn

Detection rules
Beta Give feedback
  1. [Security Solution] Cypress: create rules disabled by default #147377
    Team: SecuritySolution Team:Detection Rule Management Team:Detections and Resp performance technical debt test test_ui_functional
  2. [Security Solution] Cypress: set a large default rule interval #153689
    Team: SecuritySolution Team:Detection Rule Management Team:Detections and Resp performance technical debt test test_ui_functional
  3. [Security Solution] Make Cypress tests for rules driven by code instead of data #153692
    Team: SecuritySolution Team:Detection Rule Management Team:Detections and Resp performance refactoring technical debt test test_ui_functional
  4. [Security Solution] Replace goToRuleDetails with goToTheRuleDetailsOf in e2e tests #164513
    8.11 candidate Team: SecuritySolution Team:Detection Rule Management Team:Detections and Resp technical debt test
    maximpn
@banderror banderror added test test_ui_functional technical debt Improvement of the software architecture and operational architecture test-api-integration Team:Detections and Resp Security Detection Response Team Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. 8.10 candidate labels Jul 8, 2023
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@banderror banderror mentioned this issue Jul 8, 2023
Open
@banderror banderror added the Meta label Jul 8, 2023
@banderror banderror mentioned this issue Jul 8, 2023
Closed
12 tasks
@banderror banderror added Team:Detection Rule Management Security Detection Rule Management Team Team:Detection Engine Security Solution Detection Engine Area labels Jul 8, 2023
@VidhiRambhia
Copy link
Contributor

VidhiRambhia commented Nov 9, 2023 •
edited

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

@banderror
Copy link
Contributor Author

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
Copy link
Contributor

Hi @banderror,

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

@VidhiRambhia VidhiRambhia mentioned this issue Nov 23, 2023
Merged
2 tasks
@VidhiRambhia
Copy link
Contributor

VidhiRambhia commented Nov 23, 2023 •
edited

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 - #171813

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

For the third issue, I have raised a draft PR - #171814

Please let me know your thoughts on these!

banderror pushed a commit that referenced this issue Nov 30, 2023
@VidhiRambhia
Updating PR template to add a checklist item (#171813)
508e9da 
**Related to:** #161505

## Summary

This PR adds a checklist item in the pull request template for this
repository. The added checklist item is to check if the [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on tests changed in the pull request.

### Checklist

- [X] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)

### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
@banderror
Copy link
Contributor Author

@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.

@VidhiRambhia
Copy link
Contributor

@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
Copy link
Member

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
Copy link
Contributor Author

@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 VidhiRambhia mentioned this issue Dec 1, 2023
Draft
10 tasks
@VidhiRambhia
Copy link
Contributor

VidhiRambhia commented Dec 1, 2023 •
edited

@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'. - #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
Copy link
Contributor Author

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.

@banderror banderror mentioned this issue Dec 12, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Meta Team:Detection Engine Security Solution Detection Engine Area Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting Security Solution Threat Hunting Team technical debt Improvement of the software architecture and operational architecture test_ui_functional test test-api-integration
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@maximpn @banderror @elasticmachine @MadameSheema @VidhiRambhia