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.

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

[Security Solutions] Make Cypress tests work independent of on-disk rules vs cloud rules #105857

Conversation

FrankHassanabad
Copy link
Contributor

@FrankHassanabad FrankHassanabad commented Jul 15, 2021

Summary

Whenever we forget to mirror a rule on disk on-disk compared to a fleet server being updated, the pre-packaged rules will activate and prefer to grab the rules from the online service which can and will cause issues with our tests. This PR decouples that logic so that the total count of rules comes from the pre-packaged rule endpoint instead of relying on-disk rules.

  • Increases our timeouts around waiting for pre-packaged rules to happen since we saw timeouts when the online rules were taking over.
  • Uses a >= check instead of strict equals and splits out the regex of numbers from their text in the tests.
  • Changes the prebuild_rules.spec.ts

One important CON to mention is that you're losing a bit of test safety that all the rules were installed correctly since we use a >= even when checking strings around deleting rules.

How to manual testing this situation:
Run the cypress tests and delete one of the rules from

x-pack/plugins/security_solution/server/lib/detection_engine/rules/prepackaged_rules/index.ts

And you should see Cypress still passing tests.

Checklist

@FrankHassanabad FrankHassanabad requested a review from a team as a code owner July 15, 2021 21:55
@FrankHassanabad FrankHassanabad self-assigned this Jul 15, 2021
@FrankHassanabad FrankHassanabad added v8.0.0 v7.15.0 release_note:skip Skip the PR/issue when compiling release notes labels Jul 15, 2021
Copy link
Contributor

@rw-access rw-access left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @FrankHassanabad!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @FrankHassanabad

Copy link
Member

@MadameSheema MadameSheema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this since I it is not a good idea to have our tests blocking things but I would deeply appreciate @FrankHassanabad if we can have a quick call to have a better understanding why that missalignment exist. Thanks for taking care of this :)

@FrankHassanabad
Copy link
Contributor Author

Putting this in draft mode in favor of potentially one of the other options from:
#105980

@FrankHassanabad
Copy link
Contributor Author

Closing this since we went with a different option here:
#106189

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants