[Security Solution] Integration tests for `query` diff algorithms by dplumlee · Pull Request #192655 · elastic/kibana

dplumlee · 2024-09-12T04:04:01Z

Summary

Switches kql_query, eql_query, and esql_query fields to use the implemented diff algorithms assigned to them in #190179

Adds integration tests in accordance to #192529 for the upgrade/_review API endpoint for the query field diff algorithms.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
Flaky Test Runner was used on any tests changed

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2024-09-12T04:04:03Z

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

elasticmachine · 2024-09-12T04:04:03Z

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

elasticmachine · 2024-09-12T04:04:03Z

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

kibanamachine · 2024-09-12T07:00:05Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6921

[✅] x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/configs/ess.config.ts: 100/100 tests passed.
[✅] x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/configs/serverless.config.ts: 100/100 tests passed.

see run history

nikitaindik

Thanks @dplumlee! I have reviewed and left a few comments.

...les/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.eql_query_fields.ts

nikitaindik · 2024-09-12T11:08:44Z

...les/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.eql_query_fields.ts

+            has_base_version: true,
+          });
+
+          expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(2);


Please help me understand why is it 2 here? Which other field was updated except eql_query? Is it version? If yes, then why is num_fields_with_updates === 1 for scenario ABA?

Yeah, it's version. It will always be present given the nature of the way the tests are written. I can put this in a comment.

For ABA, there's technically no update for the eql_query field, only version

Got it. Thanks for the explanation.

nikitaindik · 2024-09-12T11:24:20Z

...les/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.eql_query_fields.ts

+          await installPrebuiltRules(es, supertest);
+
+          // Customize an eql_query field on the installed rule
+          await updateRule(supertest, {


Why is updateRule used here, but patchRule is used for ABB below? Is there a difference?

No, this is copied over from the kql_query file where the difference was needed, I can switch them to be the same

nit: I think it's better to use patchRule because it communicates the intent more clearly. Like "I want to update these specific fields without touching the other ones".

I'd probably agree with you for the ones that don't need to unset fields, but for whatever reason, certain fields like saved_id and threat_query don't correctly update with this PATCH util. I didn't look too much into it as I thought keeping the tests consistent and readable was a good reason to just use the updateRule util, possible I was just using it incorrectly. But it works with other fields so not sure, I'll look more into it soon and open a ticket to investigate if need be

...les/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.eql_query_fields.ts

...les/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.kql_query_fields.ts

...es/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.esql_query_fields.ts

nikitaindik · 2024-09-12T14:04:42Z

...les/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.kql_query_fields.ts

Do you think we should also a file for threat_query? It's mostly like kql_query except it can only have inline queries.

Or maybe we could split this kql_query test file into two files - one for inline, another for saved queries. And then mention in the comment that the inline query one applies to both kql_query and threat_query. What's your opinion?

I was thinking a bit more about this last night, I think I'm going to add a few of these cases into the ABC category. I think it'd also be worth covering new_terms and threshold rule types as they have the kql_query field as well.

As the diff algorithm itself is the same between kql_query and threat_query, I don't think we need an entire separate file - I think just having the extra test cases for coverage exist in the ABC scenario as we've done for the other diff algorithms will suffice.

…m-integration-tests

nikitaindik

Thanks for the updates and explanations @dplumlee! PR LGTM now. Left one nit comment.

…m-integration-tests

jpdjere

LGTM ✅

dplumlee · 2024-09-16T22:20:25Z

@elasticmachine merge upstream

kibana-ci · 2024-09-16T23:58:56Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 3c3d396

Failed CI Steps

Metrics [docs]

✅ unchanged

History

💔 Build #234530 failed fa15b79
💚 Build #234167 succeeded b01b5ba
💔 Build #233652 failed d6a8fd3

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

cc @dplumlee

…astic#192655) ## Summary Completes elastic#187658 Switches `kql_query`, `eql_query`, and `esql_query` fields to use the implemented diff algorithms assigned to them in elastic#190179 Adds integration tests in accordance to elastic#192529 for the `upgrade/_review` API endpoint for the `query` field diff algorithms. ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed ### For maintainers - [ ] 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) (cherry picked from commit ceb1b1a)

kibanamachine · 2024-09-17T00:03:38Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…f algorithms (#192655) (#193108) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution] Integration tests for `query` diff algorithms (#192655)](#192655)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Davis Plumlee <56367316+dplumlee@users.noreply.github.com>

adds integration tests

d6a8fd3

dplumlee self-assigned this Sep 12, 2024

dplumlee requested a review from a team as a code owner September 12, 2024 04:04

dplumlee requested a review from nikitaindik September 12, 2024 04:04

dplumlee requested a review from jpdjere September 12, 2024 04:04

dplumlee mentioned this pull request Sep 12, 2024

[Security Solution] Implement query fields diff algorithms #187658

Closed

8 tasks

banderror requested a review from pborgonovi September 12, 2024 11:41

nikitaindik reviewed Sep 12, 2024

View reviewed changes

dplumlee assigned dplumlee and unassigned dplumlee Sep 12, 2024

dplumlee added 2 commits September 12, 2024 14:22

Merge remote-tracking branch 'upstream/main' into query-diff-algorith…

21a8e9d

…m-integration-tests

addresses comments and adds more tests

b01b5ba

dplumlee requested a review from nikitaindik September 13, 2024 19:18

nikitaindik approved these changes Sep 16, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into query-diff-algorith…

fa15b79

…m-integration-tests

jpdjere approved these changes Sep 16, 2024

View reviewed changes

dplumlee enabled auto-merge (squash) September 16, 2024 17:45

Merge branch 'main' into query-diff-algorithm-integration-tests

3c3d396

dplumlee merged commit ceb1b1a into elastic:main Sep 16, 2024

kibanamachine added the v9.0.0 label Sep 16, 2024

kibanamachine mentioned this pull request Sep 17, 2024

[8.x] [Security Solution] Integration tests for `query` diff algorithms (#192655) #193108

Merged

dplumlee deleted the query-diff-algorithm-integration-tests branch September 17, 2024 00:06

Conversation

dplumlee commented Sep 12, 2024 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Sep 12, 2024

Uh oh!

elasticmachine commented Sep 12, 2024

Uh oh!

elasticmachine commented Sep 12, 2024

Uh oh!

kibanamachine commented Sep 12, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6921

Uh oh!

nikitaindik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dplumlee Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikitaindik left a comment

Choose a reason for hiding this comment

Uh oh!

jpdjere left a comment

Choose a reason for hiding this comment

Uh oh!

dplumlee commented Sep 16, 2024

Uh oh!

kibana-ci commented Sep 16, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

History

Uh oh!

kibanamachine commented Sep 17, 2024

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dplumlee commented Sep 12, 2024 •

edited by kibanamachine

Loading

dplumlee Sep 13, 2024 •

edited

Loading