Skip to content

[Security Solution] Simple diff algorithm test plan#184484

Merged
banderror merged 10 commits intoelastic:mainfrom
dplumlee:single-line-field-diff-test-plan
Jun 10, 2024
Merged

[Security Solution] Simple diff algorithm test plan#184484
banderror merged 10 commits intoelastic:mainfrom
dplumlee:single-line-field-diff-test-plan

Conversation

@dplumlee
Copy link
Copy Markdown
Contributor

@dplumlee dplumlee commented May 30, 2024

Summary

Adds test plan in accordance to #180158

Adds test cases for simple diff algorithm to prebuilt rules' Installation and upgrade test plan

@dplumlee dplumlee added release_note:skip Skip the PR/issue when compiling release notes test-plan Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area v8.15.0 labels May 30, 2024
@dplumlee dplumlee self-assigned this May 30, 2024
@dplumlee dplumlee marked this pull request as ready for review May 30, 2024 16:14
@dplumlee dplumlee requested a review from a team as a code owner May 30, 2024 16:14
@dplumlee dplumlee requested a review from maximpn May 30, 2024 16:14
@elasticmachine
Copy link
Copy Markdown
Contributor

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

@elasticmachine
Copy link
Copy Markdown
Contributor

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

@elasticmachine
Copy link
Copy Markdown
Contributor

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

@dplumlee
Copy link
Copy Markdown
Contributor Author

/ci

@kibana-ci
Copy link
Copy Markdown

💔 Build Failed

Failed CI Steps

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

cc @dplumlee

@dplumlee
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@nikitaindik nikitaindik requested review from nikitaindik and removed request for maximpn May 31, 2024 11:58
@nikitaindik
Copy link
Copy Markdown
Contributor

nikitaindik commented May 31, 2024

Do we also want to add "base version missing" scenarios here ("-AA", "-AB")? As I understand, it might be the case if base version was removed from the Fleet package.

UPD: Just noticed it's included as CASEs in the last two scenarios.

Copy link
Copy Markdown
Contributor

@nikitaindik nikitaindik left a comment

Choose a reason for hiding this comment

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

Hey @dplumlee! I left a few comments. Please take a look when you get a chance.

@dplumlee dplumlee force-pushed the single-line-field-diff-test-plan branch from 12c3e16 to 87f239a Compare May 31, 2024 15:22
Copy link
Copy Markdown
Contributor

@nikitaindik nikitaindik left a comment

Choose a reason for hiding this comment

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

Thanks for answering my questions @dplumlee! Approving.

@dplumlee
Copy link
Copy Markdown
Contributor Author

dplumlee commented Jun 3, 2024

@elasticmachine merge upstream

Copy link
Copy Markdown
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

@dplumlee Overall, LGTM 👍

  • The set of scenarios looks comprehensive. I'd only extract one case into its own scenario.
  • I think we're missing some assertions. I think these scenarios should cover more behavior in the app.
  • I'd reword a few things.

I left some comments applied to a single scenario, but they actually apply to all of them. Let me know what you think.

Comment on lines +894 to +900
And <field_name> field should not be returned from the `upgrade/_review` API endpoint
And <field_name> field should not be shown in the upgrade preview UI

Examples:
| field_name | base_version | current_version | target_version |
| name | "A" | "B" | "B" |
| risk_score | 1 | 2 | 2 |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: Just want to point out that this case is similar to the one described in #180154, and we might want to revisit it later. But for now let's keep it as is.

@banderror
Copy link
Copy Markdown
Contributor

@elasticmachine merge upstream

@banderror banderror enabled auto-merge (squash) June 10, 2024 12:58
@banderror banderror merged commit d014908 into elastic:main Jun 10, 2024
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Jun 10, 2024
@dplumlee dplumlee deleted the single-line-field-diff-test-plan branch June 10, 2024 13:47
dplumlee added a commit that referenced this pull request Jun 13, 2024
)

## Summary

Completes related tickets:
#180160 and
#180158

Switches fields to use the diff algorithms assigned to them in the
related tickets

Adds integration tests in accordance to
#184484 for the `upgrade/_review`
API endpoint for the simple diff algorithm.

Also changes logic in the `upgrade/_review` API endpoint to return user
customized fields in the diffs even if there was not an update for that
field. This new logic is described in
#180154. We filter out the
fields that fall under this new logic so that they are only returned
from the API but not displayed in the per-field rule diff flyout as the
current UI cannot support them. They are utilized in testing logic and
will be implemented in the UI at a later date

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

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area release_note:skip Skip the PR/issue when compiling release notes 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. test-plan v8.15.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants