Skip to content

[Security Solution] Allow rewriting rule create props in Cypress tests#153474

Merged
maximpn merged 2 commits intoelastic:mainfrom
maximpn:rule-creator-prop-rewrites
Mar 27, 2023
Merged

[Security Solution] Allow rewriting rule create props in Cypress tests#153474
maximpn merged 2 commits intoelastic:mainfrom
maximpn:rule-creator-prop-rewrites

Conversation

@maximpn
Copy link
Copy Markdown
Contributor

@maximpn maximpn commented Mar 22, 2023 •
edited by kibanamachine
Loading

Relates to: #150553

Summary

This PR is based on the review comments in #150553. It allows to rewrite rule create properties.

Details

Rule create properties are returned by helper functions like getNewRule(), getNewThresholdRule() and so on. So instead of createRule({ ...getNewRule(), index: undefined, data_view_id: DATA_VIEW_ID, rule_id: '1' }) it allows to use a concise and a much more readable structure createRule(getNewRule({ index: undefined, data_view_id: DATA_VIEW_ID, rule_id: '1' )).

Possible improvements

The PR doesn't implement deep / nested fields merge. High level fields completely rewrite default values. Deep merge would allow to extend defaults with the provided rewrites. For example, overriding nested properties become tiresome quickly, as shown in the following code snippet:

const rule = {
  ...getNewTermsRule(),
  rule_id: 'new_rule_id',
  runsEvery: {
    interval: '1',
    ...getNewTermsRule().runsEvery,
  },
};

If we implement deep merge, the readability could be greatly improved:

const rule = getNewTermsRule({
  rule_id: 'new_rule_id',
  runsEvery: {
    interval: '1',
  },
});

While it looks as a good idea we should take into consideration the fact that complete rewriting of default values can be a desired behavior. Engineers could tend to switch to createRule({ ...getNewRule(), index: undefined, data_view_id: DATA_VIEW_ID, rule_id: '1' }) to overcome deep merge. So it should be analysed carefully before implementing it.

@maximpn maximpn added enhancement New value added to drive a business result release_note:skip Skip the PR/issue when compiling release notes 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 backport:prev-minor v8.8.0 labels Mar 22, 2023
@maximpn maximpn self-assigned this Mar 22, 2023
@maximpn maximpn marked this pull request as ready for review March 22, 2023 22:52
@maximpn maximpn requested review from a team as code owners March 22, 2023 22:52
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 22, 2023

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

@maximpn maximpn requested review from a team as code owners March 22, 2023 22:52
@maximpn maximpn requested a review from spong March 22, 2023 22:52
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 22, 2023

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

@maximpn maximpn requested a review from xcrzx March 22, 2023 22:52
xcrzx
xcrzx approved these changes Mar 23, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@xcrzx xcrzx left a comment

Choose a reason for hiding this comment

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

Thank you for this refactoring, @maximpn 👍

e40pud
e40pud approved these changes Mar 23, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@e40pud e40pud left a comment

Choose a reason for hiding this comment

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

LGTM!

stephmilovic
stephmilovic approved these changes Mar 23, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@stephmilovic stephmilovic left a comment

Choose a reason for hiding this comment

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

LGTM on explore changes, thank you!

@maximpn maximpn force-pushed the rule-creator-prop-rewrites branch from defdc3d to bcb9359 Compare March 23, 2023 15:31
@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Mar 23, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Tests #4 / Row renderers Selected renderer can be disabled and enabled

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 433 436 +3

Total ESLint disabled count

id before after diff
securitySolution 513 516 +3

History

  • 💚 Build #114752 succeeded defdc3d49dcec077623fcf09093ef29062e16e2e

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

cc @maximpn

@banderror banderror removed the enhancement New value added to drive a business result label Mar 23, 2023
@banderror banderror added refactoring test-coverage issues & PRs for improving code test coverage labels Mar 23, 2023
@banderror banderror changed the title [Security Solution] Allow rewriting rule create props [Security Solution] Allow rewriting rule create props in tests Mar 23, 2023
@banderror
Copy link
Copy Markdown
Contributor

banderror commented Mar 23, 2023

@maximpn please be mindful about backports 🙏

@banderror banderror mentioned this pull request Mar 24, 2023
Open
54 tasks
@banderror banderror changed the title [Security Solution] Allow rewriting rule create props in tests [Security Solution] Allow rewriting rule create props in Cypress tests Mar 24, 2023
@banderror banderror added test test_ui_functional technical debt Improvement of the software architecture and operational architecture and removed test-coverage issues & PRs for improving code test coverage labels Mar 24, 2023
spong
spong approved these changes Mar 24, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

Thank you for dev-ex optimization and detailed description here @maximpn 🙂 -- LGTM! 👍

@maximpn maximpn merged commit ca696ac into elastic:main Mar 27, 2023
@maximpn maximpn deleted the rule-creator-prop-rewrites branch March 27, 2023 09:09
@maximpn maximpn restored the rule-creator-prop-rewrites branch March 27, 2023 09:09
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Mar 27, 2023

💔 All backports failed

Status Branch Result
❌ 8.7 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 153474

Questions ?

Please refer to the Backport tool documentation

@maximpn maximpn deleted the rule-creator-prop-rewrites branch March 27, 2023 09:17
maximpn added a commit to maximpn/kibana that referenced this pull request Apr 4, 2023
@maximpn
[Security Solution] Allow rewriting rule create props in Cypress tests (
8fb0696 
elastic#153474)

**Relates to:** elastic#150553

## Summary

This PR is based on the review comments in elastic#150553. It allows to rewrite rule create properties.

## Details

Rule create properties are returned by helper functions like `getNewRule()`, `getNewThresholdRule()` and so on. So instead of `createRule({ ...getNewRule(), index: undefined, data_view_id: DATA_VIEW_ID, rule_id: '1' })` it allows to use a concise and a much more readable structure `createRule(getNewRule({ index: undefined, data_view_id: DATA_VIEW_ID, rule_id: '1' ))`.

## Possible improvements

The PR doesn't implement deep / nested fields merge. High level fields completely rewrite default values. Deep merge would allow to extend defaults with the provided rewrites. For example, overriding nested properties become tiresome quickly, as shown in the following code snippet:

```ts
const rule = {
  ...getNewTermsRule(),
  rule_id: 'new_rule_id',
  runsEvery: {
    interval: '1',
    ...getNewTermsRule().runsEvery,
  },
};
```

If we implement deep merge, the readability could be greatly improved:

```ts
const rule = getNewTermsRule({
  rule_id: 'new_rule_id',
  runsEvery: {
    interval: '1',
  },
});
```

While it looks as a good idea we should take into consideration the fact that complete rewriting of default values can be a desired behavior. Engineers could tend to switch to `createRule({ ...getNewRule(), index: undefined, data_view_id: DATA_VIEW_ID, rule_id: '1' })` to overcome deep merge. So it should be analysed carefully before implementing it.

(cherry picked from commit ca696ac)

# Conflicts:
#	x-pack/plugins/security_solution/cypress/e2e/detection_alerts/alerts_charts.cy.ts
#	x-pack/plugins/security_solution/cypress/e2e/detection_alerts/detection_page_filters.cy.ts
#	x-pack/plugins/security_solution/cypress/e2e/exceptions/add_edit_flyout/flyout_validation.cy.ts
@maximpn maximpn mentioned this pull request Apr 4, 2023
Merged
@maximpn
Copy link
Copy Markdown
Contributor Author

maximpn commented Apr 4, 2023

💚 All backports created successfully

Status Branch Result
✅ 8.7

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

Questions ?

Please refer to the Backport tool documentation

maximpn added a commit that referenced this pull request Apr 4, 2023
@maximpn
[8.7] [Security Solution] Allow rewriting rule create props in Cypres…
5b7d183 
…s tests (#153474) (#154332)

# Backport

This will backport the following commits from `main` to `8.7`:
- [[Security Solution] Allow rewriting rule create props in Cypress
tests (#153474)](#153474)

<!--- Backport version: 8.9.7 -->

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

<!--BACKPORT [{"author":{"name":"Maxim
Palenov","email":"maxim.palenov@elastic.co"},"sourceCommit":{"committedDate":"2023-03-27T09:08:38Z","message":"[Security
Solution] Allow rewriting rule create props in Cypress tests
(#153474)\n\n**Relates to:**
https://github.com/elastic/kibana/pull/150553\r\n\r\n##
Summary\r\n\r\nThis PR is based on the review comments in
#150553. It allows to rewrite rule
create properties.\r\n\r\n## Details\r\n\r\nRule create properties are
returned by helper functions like `getNewRule()`,
`getNewThresholdRule()` and so on. So instead of `createRule({
...getNewRule(), index: undefined, data_view_id: DATA_VIEW_ID, rule_id:
'1' })` it allows to use a concise and a much more readable structure
`createRule(getNewRule({ index: undefined, data_view_id: DATA_VIEW_ID,
rule_id: '1' ))`.\r\n\r\n## Possible improvements\r\n\r\nThe PR doesn't
implement deep / nested fields merge. High level fields completely
rewrite default values. Deep merge would allow to extend defaults with
the provided rewrites. For example, overriding nested properties become
tiresome quickly, as shown in the following code
snippet:\r\n\r\n```ts\r\nconst rule = {\r\n ...getNewTermsRule(),\r\n
rule_id: 'new_rule_id',\r\n runsEvery: {\r\n interval: '1',\r\n
...getNewTermsRule().runsEvery,\r\n },\r\n};\r\n```\r\n\r\nIf we
implement deep merge, the readability could be greatly
improved:\r\n\r\n```ts\r\nconst rule = getNewTermsRule({\r\n rule_id:
'new_rule_id',\r\n runsEvery: {\r\n interval: '1',\r\n
},\r\n});\r\n```\r\n\r\nWhile it looks as a good idea we should take
into consideration the fact that complete rewriting of default values
can be a desired behavior. Engineers could tend to switch to
`createRule({ ...getNewRule(), index: undefined, data_view_id:
DATA_VIEW_ID, rule_id: '1' })` to overcome deep merge. So it should be
analysed carefully before implementing
it.","sha":"ca696ac50c0591acf6723e130d2f9278c2d6ef65","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["test","refactoring","test_ui_functional","technical
debt","release_note:skip","Team:Detections and Resp","Team:
SecuritySolution","Team:Detection
Rules","backport:prev-minor","v8.8.0"],"number":153474,"url":"https://github.com/elastic/kibana/pull/153474","mergeCommit":{"message":"[Security
Solution] Allow rewriting rule create props in Cypress tests
(#153474)\n\n**Relates to:**
https://github.com/elastic/kibana/pull/150553\r\n\r\n##
Summary\r\n\r\nThis PR is based on the review comments in
#150553. It allows to rewrite rule
create properties.\r\n\r\n## Details\r\n\r\nRule create properties are
returned by helper functions like `getNewRule()`,
`getNewThresholdRule()` and so on. So instead of `createRule({
...getNewRule(), index: undefined, data_view_id: DATA_VIEW_ID, rule_id:
'1' })` it allows to use a concise and a much more readable structure
`createRule(getNewRule({ index: undefined, data_view_id: DATA_VIEW_ID,
rule_id: '1' ))`.\r\n\r\n## Possible improvements\r\n\r\nThe PR doesn't
implement deep / nested fields merge. High level fields completely
rewrite default values. Deep merge would allow to extend defaults with
the provided rewrites. For example, overriding nested properties become
tiresome quickly, as shown in the following code
snippet:\r\n\r\n```ts\r\nconst rule = {\r\n ...getNewTermsRule(),\r\n
rule_id: 'new_rule_id',\r\n runsEvery: {\r\n interval: '1',\r\n
...getNewTermsRule().runsEvery,\r\n },\r\n};\r\n```\r\n\r\nIf we
implement deep merge, the readability could be greatly
improved:\r\n\r\n```ts\r\nconst rule = getNewTermsRule({\r\n rule_id:
'new_rule_id',\r\n runsEvery: {\r\n interval: '1',\r\n
},\r\n});\r\n```\r\n\r\nWhile it looks as a good idea we should take
into consideration the fact that complete rewriting of default values
can be a desired behavior. Engineers could tend to switch to
`createRule({ ...getNewRule(), index: undefined, data_view_id:
DATA_VIEW_ID, rule_id: '1' })` to overcome deep merge. So it should be
analysed carefully before implementing
it.","sha":"ca696ac50c0591acf6723e130d2f9278c2d6ef65"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/153474","number":153474,"mergeCommit":{"message":"[Security
Solution] Allow rewriting rule create props in Cypress tests
(#153474)\n\n**Relates to:**
https://github.com/elastic/kibana/pull/150553\r\n\r\n##
Summary\r\n\r\nThis PR is based on the review comments in
#150553. It allows to rewrite rule
create properties.\r\n\r\n## Details\r\n\r\nRule create properties are
returned by helper functions like `getNewRule()`,
`getNewThresholdRule()` and so on. So instead of `createRule({
...getNewRule(), index: undefined, data_view_id: DATA_VIEW_ID, rule_id:
'1' })` it allows to use a concise and a much more readable structure
`createRule(getNewRule({ index: undefined, data_view_id: DATA_VIEW_ID,
rule_id: '1' ))`.\r\n\r\n## Possible improvements\r\n\r\nThe PR doesn't
implement deep / nested fields merge. High level fields completely
rewrite default values. Deep merge would allow to extend defaults with
the provided rewrites. For example, overriding nested properties become
tiresome quickly, as shown in the following code
snippet:\r\n\r\n```ts\r\nconst rule = {\r\n ...getNewTermsRule(),\r\n
rule_id: 'new_rule_id',\r\n runsEvery: {\r\n interval: '1',\r\n
...getNewTermsRule().runsEvery,\r\n },\r\n};\r\n```\r\n\r\nIf we
implement deep merge, the readability could be greatly
improved:\r\n\r\n```ts\r\nconst rule = getNewTermsRule({\r\n rule_id:
'new_rule_id',\r\n runsEvery: {\r\n interval: '1',\r\n
},\r\n});\r\n```\r\n\r\nWhile it looks as a good idea we should take
into consideration the fact that complete rewriting of default values
can be a desired behavior. Engineers could tend to switch to
`createRule({ ...getNewRule(), index: undefined, data_view_id:
DATA_VIEW_ID, rule_id: '1' })` to overcome deep merge. So it should be
analysed carefully before implementing
it.","sha":"ca696ac50c0591acf6723e130d2f9278c2d6ef65"}}]}] BACKPORT-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhurley14 dhurley14 dhurley14 approved these changes

@e40pud e40pud e40pud approved these changes

@spong spong spong approved these changes

@stephmilovic stephmilovic stephmilovic approved these changes

@michaelolo24 michaelolo24 michaelolo24 approved these changes

+1 more reviewer

@xcrzx xcrzx xcrzx approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@maximpn maximpn

Labels

refactoring 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. technical debt Improvement of the software architecture and operational architecture test_ui_functional test v8.7.1 v8.8.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.