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] Per-field diffs test coverage #177399

Merged
merged 5 commits into from
Feb 22, 2024
Merged

[Security Solution] Per-field diffs test coverage #177399

merged 5 commits into from
Feb 22, 2024

Conversation

dplumlee
Copy link
Contributor

@dplumlee dplumlee commented Feb 21, 2024 •
edited by kibanamachine
Loading

Summary

Addresses test coverage acceptance criteria for #166489

Adds test coverage in accordance to the recently merged test plan

Flaky test runner

@dplumlee dplumlee added release_note:skip Skip the PR/issue when compiling release notes test-coverage issues & PRs for improving code test coverage 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.13.0 v8.14.0 labels Feb 21, 2024
@dplumlee dplumlee self-assigned this Feb 21, 2024
@dplumlee dplumlee requested a review from a team as a code owner February 21, 2024 06:22
@dplumlee dplumlee requested a review from maximpn February 21, 2024 06:22
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@banderror banderror mentioned this pull request Feb 21, 2024
Closed
17 tasks
maximpn
maximpn reviewed Feb 22, 2024
View reviewed changes
Copy link
Contributor

@maximpn maximpn left a comment

Choose a reason for hiding this comment

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

@dplumlee thank you for covering per-field diffs by tests 🙏

Changes look good overall I left a few questions and comments to improve readability.

...s/cypress/e2e/detection_response/rule_management/prebuilt_rules/prebuilt_rules_preview.cy.ts
cy.get(PER_FIELD_DIFF_WRAPPER).last().contains('Outdated rule 1').should('be.visible');
cy.get(PER_FIELD_DIFF_WRAPPER).last().contains('Updated rule 1').should('be.visible');

/* Select another rule without closing the preview for the current rule */
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Starting from this point it looks like a new scenario. It shouldn't be a significant overhead to move lines below to a separate scenario but will help us catch problems related to rules switching more efficiently. On top of that the test can varify Outdated rule 2 content as well to make sure not only title changed.

.../public/detection_engine/rule_management/components/rule_details/per_field_rule_diff_tab.tsx Outdated Show resolved Hide resolved
...ic/detection_engine/rule_management/components/rule_details/per_field_rule_diff_tab.test.tsx Show resolved Hide resolved
...ic/detection_engine/rule_management/components/rule_details/per_field_rule_diff_tab.test.tsx Outdated Show resolved Hide resolved
...lution/docs/testing/test_plans/detection_response/prebuilt_rules/installation_and_upgrade.md
@@ -978,14 +978,13 @@ Then the preview should display the changes for the newly selected rule

#### **Scenario: User can see changes when updated rule is a different rule type**

**Automation**: 1 UI integration test
**Automation**: 1 e2e test

```Gherkin
Given a prebuilt rule is installed in Kibana
And this rule has an update available that changes the rule type
When user opens the upgrade preview
Then the rule type changes should be displayed in grouped field diffs with corresponding query fields
Copy link
Contributor

Choose a reason for hiding this comment

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

Did I get it right the tooltip isn't implement yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we are pushing it back since we're not sure how best to implement it yet. I figured the best move would be to take it off here for now, but maybe we leave it in and just make a note to rewrite the test whenever it becomes available? What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion a note in the test plan should convey the intention much better.

maximpn
maximpn approved these changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@maximpn maximpn left a comment

Choose a reason for hiding this comment

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

@dplumlee thank you for addressing my comments 👍

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Investigations - Security Solution Cypress Tests #6 / Basic esql search and filter operations "before each" hook for "should remove the query when the back button is pressed after adding a query" "before each" hook for "should remove the query when the back button is pressed after adding a query"

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 11.6MB 11.6MB +457.0B

History

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

cc @dplumlee

@dplumlee dplumlee merged commit 3c34b53 into elastic:main Feb 22, 2024
35 checks passed
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 22, 2024
@dplumlee
[Security Solution] Per-field diffs test coverage (elastic#177399)
1a75c37 
## Summary

Addresses test coverage acceptance criteria for
elastic#166489

Adds test coverage in accordance to the recently merged [test
plan](elastic#176474)

[Flaky test
runner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5279)

(cherry picked from commit 3c34b53)
@kibanamachine kibanamachine mentioned this pull request Feb 22, 2024
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.13

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

Questions ?

Please refer to the Backport tool documentation

@dplumlee dplumlee deleted the per-field-diff-test-coverage branch February 22, 2024 20:03
kibanamachine added a commit that referenced this pull request Feb 22, 2024
@kibanamachine @dplumlee
[8.13] [Security Solution] Per-field diffs test coverage (#177399) (#…
cb6716e 
…177645)

# Backport

This will backport the following commits from `main` to `8.13`:
- [[Security Solution] Per-field diffs test coverage
(#177399)](#177399)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Davis
Plumlee","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-02-22T19:57:41Z","message":"[Security
Solution] Per-field diffs test coverage (#177399)\n\n##
Summary\r\n\r\nAddresses test coverage acceptance criteria
for\r\nhttps://github.com//issues/166489\r\n\r\nAdds test
coverage in accordance to the recently merged
[test\r\nplan](https://github.com/elastic/kibana/pull/176474)\r\n\r\n[Flaky
test\r\nrunner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5279)","sha":"3c34b535ceac5a5c869719998d9e03a0d44ce21a","branchLabelMapping":{"^v8.14.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","test-coverage","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","Feature:Prebuilt Detection
Rules","v8.13.0","v8.14.0"],"title":"[Security Solution] Per-field diffs
test
coverage","number":177399,"url":"https://github.com/elastic/kibana/pull/177399","mergeCommit":{"message":"[Security
Solution] Per-field diffs test coverage (#177399)\n\n##
Summary\r\n\r\nAddresses test coverage acceptance criteria
for\r\nhttps://github.com//issues/166489\r\n\r\nAdds test
coverage in accordance to the recently merged
[test\r\nplan](https://github.com/elastic/kibana/pull/176474)\r\n\r\n[Flaky
test\r\nrunner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5279)","sha":"3c34b535ceac5a5c869719998d9e03a0d44ce21a"}},"sourceBranch":"main","suggestedTargetBranches":["8.13"],"targetPullRequestStates":[{"branch":"8.13","label":"v8.13.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.14.0","branchLabelMappingKey":"^v8.14.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/177399","number":177399,"mergeCommit":{"message":"[Security
Solution] Per-field diffs test coverage (#177399)\n\n##
Summary\r\n\r\nAddresses test coverage acceptance criteria
for\r\nhttps://github.com//issues/166489\r\n\r\nAdds test
coverage in accordance to the recently merged
[test\r\nplan](https://github.com/elastic/kibana/pull/176474)\r\n\r\n[Flaky
test\r\nrunner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5279)","sha":"3c34b535ceac5a5c869719998d9e03a0d44ce21a"}}]}]
BACKPORT-->

Co-authored-by: Davis Plumlee <[email protected]>
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
@dplumlee @fkanout
[Security Solution] Per-field diffs test coverage (elastic#177399)
0bce3ce 
## Summary

Addresses test coverage acceptance criteria for
elastic#166489

Adds test coverage in accordance to the recently merged [test
plan](elastic#176474)

[Flaky test
runner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5279)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maximpn maximpn maximpn approved these changes

Assignees

@dplumlee dplumlee

Labels
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-coverage issues & PRs for improving code test coverage v8.13.0 v8.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dplumlee @elasticmachine @kibana-ci @kibanamachine @maximpn