[Security Solution][Detections] Integration test for Editing a Rule #77090

rylnd · 2020-09-09T17:31:14Z

Summary

I discovered several bugs with our Rule forms in the course of working #76138, a few of which were pretty significant. This adds an integration test around the main functionality of viewing/editing/saving an existing Rule in order to catch future regressions.

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

Right now this just navigates around and verifies that the form is correctly repopulated; next step will be to modify/asset some changes.

We already were asserting on the population of the Edit form after creation; this additionally makes modifications, saves them, and asserts the resulting values on the Rule Details page.

rylnd · 2020-09-09T17:40:11Z

x-pack/plugins/security_solution/cypress/tasks/edit_rule.ts

+  cy.get(ABOUT_RULE_DESCRIPTION).invoke('text').should('eql', rule.description);
+  cy.get(ABOUT_STEP).eq(ABOUT_SEVERITY).invoke('text').should('eql', rule.severity);
+  cy.get(ABOUT_STEP).eq(ABOUT_RISK).invoke('text').should('eql', rule.riskScore);
+  cy.get(ABOUT_STEP).eq(2).invoke('text').should('eql', expectedTags);


@MadameSheema due to the dynamic nature of our Rule Details views, I found that these hardcoded indexes (ABOUT_SEVERITY, RULE_TYPE, etc.) would only work for rules with specific data (i.e. rules created manually). For the rules in the custom_rules archive, many of these fields don't exist and so these indexes don't work (e.g. ABOUT_TAGS = 5 but there are only three fields for a custom rule (severity, risk_score, and tags).

While I can modify the custom_rules archive to include the data necessary to make this work, I was wondering if you had any other thoughts here: setting data-test-subj on these fields and querying them directly, or adding some kind of logic to dynamically determine the indexes both seem like less brittle approaches.

If is possible the best solution is to add data-test-subj to all the elements 😊

Can you please check if it can be done? If not we can think in a different solution.

@MadameSheema adding data-test-subj didn't seem feasible as EuiDescriptionList doesn't currently support its children having such a prop, so I went the "dynamic indexes" route, which I think turned out well: 569980b

rylnd · 2020-09-09T17:42:47Z

x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts

@@ -191,6 +208,33 @@ export const expectAboutFormToRepopulateAndContinue = (rule: CustomRule) => {
  cy.get(ABOUT_CONTINUE_BTN).should('not.exist');
 };

+export const expectDefineStepForm = (rule: CustomRule) => {


@MadameSheema what do you think of defining these (hopefully) reusable expectation functions?

Is there utility in naming/exporting these for reuse as opposed to inlining them in tests?

Should they live in their own folder (e.g. /cypress/assertions/) ?

Not using functions as expectations was a team agreement. When I refactored the tests for the first time I used your approach but the team prefers to don't have the expectations wrapped because if there is a failure with the assertions they don't have to dig into the code to understand what is failing.

Anyway it is a topic we can revisit again :)

Inlined expectations here: b174f44

rylnd · 2020-09-09T17:44:20Z

x-pack/plugins/security_solution/cypress/tasks/edit_rule.ts

+  const expectedIndexPatterns =
+    rule.index && rule.index.length
+      ? rule.index
+      : [


Since we don't seem to reference shared plugin constants within the tests, I'm wondering about the appropriate place for these kind of static, shared data.

elasticmachine · 2020-09-09T21:34:07Z

Pinging @elastic/siem (Team:SIEM)

So that expectation failures are less obfuscated, the decision was previously made to abstract user navigation into functions, but to leave expectations directly within the test body.

Rule Details are unfortunately unstructured: they're an array of <dt>s and <dd>s without any hierarchy. To address this, tests were previously hardcoding the order of these fields, and assertions were performed by querying for all <dd>s and then indexing with the hardcoded number (e.g. ABOUT_FALSE_POSITIVES). However, in addition to being unstructured, these fields are also _dynamic_, and will be present/absent depending on the data of the given rule. Thus, we started needing multiple orderings for the different combinations of rule fields/rule types. In the absence of refactoring how we build rule details, I'm introducing a simple helper function to fetch the relevant <dd> by the corresponding <dt>s text. This should be more robust to change and more declarative.

rylnd · 2020-09-14T14:51:11Z

@elasticmachine merge upstream

Conflicts: x-pack/plugins/security_solution/cypress/screens/create_new_rule.ts x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts

Lots of these variables no longer exist upstream and this new test needed to be refactored.

kibanamachine · 2020-09-15T20:22:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 85f55e9

Build metrics

async chunks size

id	value	diff	baseline
securitySolution	10.1MB	+317.0B	10.1MB

distributable file count

id	value	diff	baseline
default	45590	+2	45588

History

💔 Build #74559 failed 5dd2e9c
💚 Build #74168 succeeded 1645496
💔 Build #73904 failed 569980b
💚 Build #73490 succeeded e415125
💔 Build #73475 failed 6a87ca4

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

…lastic#77090) * Add cypress test around editing a detection rule Right now this just navigates around and verifies that the form is correctly repopulated; next step will be to modify/asset some changes. * Add assertions for editing a rule We already were asserting on the population of the Edit form after creation; this additionally makes modifications, saves them, and asserts the resulting values on the Rule Details page. * Remove unused imports * Inline our cypress expectations So that expectation failures are less obfuscated, the decision was previously made to abstract user navigation into functions, but to leave expectations directly within the test body. * Dynamically assert Rule Details based on titles Rule Details are unfortunately unstructured: they're an array of <dt>s and <dd>s without any hierarchy. To address this, tests were previously hardcoding the order of these fields, and assertions were performed by querying for all <dd>s and then indexing with the hardcoded number (e.g. ABOUT_FALSE_POSITIVES). However, in addition to being unstructured, these fields are also _dynamic_, and will be present/absent depending on the data of the given rule. Thus, we started needing multiple orderings for the different combinations of rule fields/rule types. In the absence of refactoring how we build rule details, I'm introducing a simple helper function to fetch the relevant <dd> by the corresponding <dt>s text. This should be more robust to change and more declarative. * Fix bad merge conflict Lots of these variables no longer exist upstream and this new test needed to be refactored. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…77090) (#77545) * Add cypress test around editing a detection rule Right now this just navigates around and verifies that the form is correctly repopulated; next step will be to modify/asset some changes. * Add assertions for editing a rule We already were asserting on the population of the Edit form after creation; this additionally makes modifications, saves them, and asserts the resulting values on the Rule Details page. * Remove unused imports * Inline our cypress expectations So that expectation failures are less obfuscated, the decision was previously made to abstract user navigation into functions, but to leave expectations directly within the test body. * Dynamically assert Rule Details based on titles Rule Details are unfortunately unstructured: they're an array of <dt>s and <dd>s without any hierarchy. To address this, tests were previously hardcoding the order of these fields, and assertions were performed by querying for all <dd>s and then indexing with the hardcoded number (e.g. ABOUT_FALSE_POSITIVES). However, in addition to being unstructured, these fields are also _dynamic_, and will be present/absent depending on the data of the given rule. Thus, we started needing multiple orderings for the different combinations of rule fields/rule types. In the absence of refactoring how we build rule details, I'm introducing a simple helper function to fetch the relevant <dd> by the corresponding <dt>s text. This should be more robust to change and more declarative. * Fix bad merge conflict Lots of these variables no longer exist upstream and this new test needed to be refactored. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…s-for-710 * 'master' of github.com:elastic/kibana: (95 commits) log request body in new ES client (elastic#77150) use `navigateToUrl` to navigate to recent nav links (elastic#77446) Move core config service to `kbn/config` package (elastic#76874) [UBI] Copy license to /licenses folder (elastic#77563) Skip flaky Events Viewer Cypress test [Lens] Remove dynamic names in telemetry fields (elastic#76988) [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366) [Enterprise Search] Add flag to restrict width of layout (elastic#77539) [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967) [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090) [Ingest pipelines] Polish pipeline debugging workflow (elastic#76058) [@kbn/utils] Adds missing dependency (elastic#77536) Add the Enterprise Search logo to the Overview search result (elastic#77514) [Logs UI] [Metrics UI] Remove saved object field mappings (elastic#75482) [Security Solution][Exceptions] Exception modal bulk close option only closes alerts generated by same rule (elastic#77402) Adds @kbn/utils package (elastic#76518) Map layout changes (elastic#77132) [Security Solution] [Detections] EQL Rule Creation (elastic#76831) Adding test user to maps tests under documents source folder (elastic#77245) Suppress error logs when clients connect over HTTP instead of HTTPS (elastic#77397) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/index.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/phases/warm_phase.tsx

* master: (55 commits) [Grok] Fix missing error message in error toasts (elastic#77499) [Alerting] Exempt Alerts pre 7.10 from RBAC on their Action execution until updated (elastic#75563) [ML] fix type in apply_influencer_filters_action (elastic#77495) [Lens] create reusable component for filters and range aggregation (elastic#77453) fix eslint violations Collapse alert chart previews by default (elastic#77479) [ML] Fixing field caps wrapper endpoint (elastic#77546) showing service maps when filte by environment not defined (elastic#77483) [Lens] Settings panel redesign and separate settings per y axis (elastic#76373) log request body in new ES client (elastic#77150) use `navigateToUrl` to navigate to recent nav links (elastic#77446) Move core config service to `kbn/config` package (elastic#76874) [UBI] Copy license to /licenses folder (elastic#77563) Skip flaky Events Viewer Cypress test [Lens] Remove dynamic names in telemetry fields (elastic#76988) [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366) [Enterprise Search] Add flag to restrict width of layout (elastic#77539) [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967) [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090) [Ingest pipelines] Polish pipeline debugging workflow (elastic#76058) ...

* master: (54 commits) [Grok] Fix missing error message in error toasts (elastic#77499) [Alerting] Exempt Alerts pre 7.10 from RBAC on their Action execution until updated (elastic#75563) [ML] fix type in apply_influencer_filters_action (elastic#77495) [Lens] create reusable component for filters and range aggregation (elastic#77453) fix eslint violations Collapse alert chart previews by default (elastic#77479) [ML] Fixing field caps wrapper endpoint (elastic#77546) showing service maps when filte by environment not defined (elastic#77483) [Lens] Settings panel redesign and separate settings per y axis (elastic#76373) log request body in new ES client (elastic#77150) use `navigateToUrl` to navigate to recent nav links (elastic#77446) Move core config service to `kbn/config` package (elastic#76874) [UBI] Copy license to /licenses folder (elastic#77563) Skip flaky Events Viewer Cypress test [Lens] Remove dynamic names in telemetry fields (elastic#76988) [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366) [Enterprise Search] Add flag to restrict width of layout (elastic#77539) [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967) [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090) [Ingest pipelines] Polish pipeline debugging workflow (elastic#76058) ...

* master: (76 commits) Fixing service maps API test (elastic#77586) [Grok] Fix missing error message in error toasts (elastic#77499) [Alerting] Exempt Alerts pre 7.10 from RBAC on their Action execution until updated (elastic#75563) [ML] fix type in apply_influencer_filters_action (elastic#77495) [Lens] create reusable component for filters and range aggregation (elastic#77453) fix eslint violations Collapse alert chart previews by default (elastic#77479) [ML] Fixing field caps wrapper endpoint (elastic#77546) showing service maps when filte by environment not defined (elastic#77483) [Lens] Settings panel redesign and separate settings per y axis (elastic#76373) log request body in new ES client (elastic#77150) use `navigateToUrl` to navigate to recent nav links (elastic#77446) Move core config service to `kbn/config` package (elastic#76874) [UBI] Copy license to /licenses folder (elastic#77563) Skip flaky Events Viewer Cypress test [Lens] Remove dynamic names in telemetry fields (elastic#76988) [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366) [Enterprise Search] Add flag to restrict width of layout (elastic#77539) [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967) [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090) ...

elasticmachine · 2021-09-23T14:25:54Z

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

rylnd added 2 commits September 8, 2020 17:42

Add cypress test around editing a detection rule

07d5f91

Right now this just navigates around and verifies that the form is correctly repopulated; next step will be to modify/asset some changes.

Add assertions for editing a rule

6a87ca4

We already were asserting on the population of the Edit form after creation; this additionally makes modifications, saves them, and asserts the resulting values on the Rule Details page.

rylnd added Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 9, 2020

rylnd self-assigned this Sep 9, 2020

rylnd commented Sep 9, 2020

View reviewed changes

rylnd requested a review from MadameSheema September 9, 2020 17:57

Remove unused imports

e415125

rylnd mentioned this pull request Sep 9, 2020

[Security Solution] [Detections] EQL Rule Creation #76831

Merged

13 tasks

rylnd marked this pull request as ready for review September 9, 2020 21:34

rylnd requested review from a team as code owners September 9, 2020 21:34

rylnd added 2 commits September 10, 2020 17:09

Inline our cypress expectations

b174f44

So that expectation failures are less obfuscated, the decision was previously made to abstract user navigation into functions, but to leave expectations directly within the test body.

Merge branch 'master' into cypress_edit_rule

1645496

MadameSheema approved these changes Sep 15, 2020

View reviewed changes

rylnd added 2 commits September 15, 2020 12:40

Merge branch 'master' into cypress_edit_rule

5dd2e9c

Conflicts: x-pack/plugins/security_solution/cypress/screens/create_new_rule.ts x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts

Fix bad merge conflict

85f55e9

Lots of these variables no longer exist upstream and this new test needed to be refactored.

rylnd merged commit 6dd558e into elastic:master Sep 15, 2020

rylnd deleted the cypress_edit_rule branch September 15, 2020 20:30

rylnd mentioned this pull request Sep 15, 2020

[7.x] [Security Solution][Detections] Integration test for Editing a Rule (#77090) #77545

Merged

rylnd mentioned this pull request Sep 18, 2020

[Security Solution] Improves detections tests #77295

Merged

MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Integration test for Editing a Rule #77090

[Security Solution][Detections] Integration test for Editing a Rule #77090

rylnd commented Sep 9, 2020

rylnd Sep 9, 2020 •

edited

Loading

MadameSheema Sep 10, 2020

rylnd Sep 11, 2020

rylnd Sep 9, 2020

MadameSheema Sep 10, 2020

rylnd Sep 11, 2020

rylnd Sep 9, 2020

elasticmachine commented Sep 9, 2020

rylnd commented Sep 14, 2020

kibanamachine commented Sep 15, 2020

elasticmachine commented Sep 23, 2021

[Security Solution][Detections] Integration test for Editing a Rule #77090

[Security Solution][Detections] Integration test for Editing a Rule #77090

Conversation

rylnd commented Sep 9, 2020

Summary

Checklist

For maintainers

rylnd Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

MadameSheema Sep 10, 2020

Choose a reason for hiding this comment

rylnd Sep 11, 2020

Choose a reason for hiding this comment

rylnd Sep 9, 2020

Choose a reason for hiding this comment

MadameSheema Sep 10, 2020

Choose a reason for hiding this comment

rylnd Sep 11, 2020

Choose a reason for hiding this comment

rylnd Sep 9, 2020

Choose a reason for hiding this comment

elasticmachine commented Sep 9, 2020

rylnd commented Sep 14, 2020

kibanamachine commented Sep 15, 2020

💚 Build Succeeded

Build metrics

async chunks size

distributable file count

History

elasticmachine commented Sep 23, 2021

rylnd Sep 9, 2020 •

edited

Loading