[Security Solution] JSON diffs test coverage #176770

nikitaindik · 2024-02-12T22:53:17Z

Resolves: #166163

Flaky test runner runs: 1, 2, 3, 4

Summary

This PR adds tests in accordance with the test plan that was merged earlier.

elasticmachine · 2024-02-15T02:53:01Z

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

elasticmachine · 2024-02-15T02:53:02Z

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

elasticmachine · 2024-02-15T02:53:03Z

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

...s/cypress/e2e/detection_response/rule_management/prebuilt_rules/prebuilt_rules_preview.cy.ts

maximpn

@nikitaindik thank you for covering JSON diffs component by test from the test plan 👍

I haven't found a Properties should be sorted alphabetically scenario. Is it missed or it's a part of some other scenario?

My main concern is unit tests overcomplicated by matchInOrder. If I'm not missing anything it should be possible to assert Diff's component textContent and avoid hardcoded colors in the test or even completely omit it.

...public/detection_engine/rule_management/components/rule_details/json_diff/json_diff.test.tsx

maximpn · 2024-02-15T12:03:53Z

...public/detection_engine/rule_management/components/rule_details/json_diff/json_diff.test.tsx

+ * otherwise `false`.
+ * Utilizes Lodash's `escapeRegExp` to handle special characters in the `strings` array.
+ */
+const matchInOrder =


Have you considered using Diff component's textContent instead?

I'm afraid this helper will make the test logic harder but it still doesn't protect again sensitivity to changes. Looking at the Diff's textContent below it's clear we should be able to implement many testing scenarios.

{{ "author": [ "author": [ "Alice", "Alice",- "Bob", "Charlie" "Charlie" ], ], "description": "some desc", "description": "some desc",Expand 15 unchanged lines ], ], "interval": "5m", "interval": "5m", "language": "kuery", "language": "kuery",+ "license": "GPLv3", "max_signals": 100, "max_signals": 100, "name": "Test rule", "name": "Test rule", "note": "", "note": "",Expand 19 unchanged lines ], ], "to": "now", "to": "now", "type": "query", "type": "query",- "version": 1+ "version": 2}}

For example normalizing it by removing extra spaces will make it simple to implement a test which verifies there is - "version": 1+ "version": 2. Of course it can break as well

We also shouldn't retest react-diff-view. It's enough to make sure the content is conceptually the same but how may components/tags and etc used isn't so important and will make the tests fragile.

Refactored this part in a follow-up commit

maximpn · 2024-02-15T12:17:01Z

...s/cypress/e2e/detection_response/rule_management/prebuilt_rules/prebuilt_rules_preview.cy.ts

+          .should('not.exist');
+      });
+
+      it('Dynamic properties should not be included in preview', () => {


This test only checks for execution_summary. What about the other fields specified in the test plan like revision and updated_at?

These are in the React Testing Library test called "Technical properties should not be included in preview". I separated "dynamic" from "technical" because I wanted to test the "dynamic" ones in a more realistic way with a Cypress E2E test.

maximpn · 2024-02-15T12:20:59Z

...public/detection_engine/rule_management/components/rule_details/json_diff/json_diff.test.tsx

+    };
+
+    render(<RuleDiffTab oldRule={oldRule} newRule={newRule} />);
+    ['revision', 'created_at', 'created_by', 'updated_at', 'updated_by'].forEach((property) => {


it.each() is intended to pass different test data or params. It allows to display modify the test name to something like should not include "revision" in preview to make it clear what fields are checked.

Addressed in a follow-up commit

...s/cypress/e2e/detection_response/rule_management/prebuilt_rules/prebuilt_rules_preview.cy.ts

jpdjere · 2024-02-15T12:23:59Z

...s/cypress/e2e/detection_response/rule_management/prebuilt_rules/prebuilt_rules_preview.cy.ts

+        cy.get('@currentRuleValue').then(($currentRuleValue) => {
+          const currentRuleLeftOffset = $currentRuleValue.offset()?.left || 0;
+          cy.get('@updateVersionValue').then(($updateVersionValue) => {
+            const updateVersionLeftOffset = $updateVersionValue.offset()?.left || 0;
+            expect(currentRuleLeftOffset).to.be.lessThan(updateVersionLeftOffset);
+          });
+        });


Same here, but for the Diff library. I think this is a level of detail greater than we should have for e2e tests as smoke tests.

Agreed. Removed.

jpdjere · 2024-02-15T12:30:47Z

...public/detection_engine/rule_management/components/rule_details/json_diff/json_diff.test.tsx

+    const removedBackgroundColor = 'rgb(255, 235, 233)';
+    const removedAccentColor = 'rgba(255, 129, 130, 0.4)';
+    const addedBackgroundColor = 'rgb(230, 255, 236)';
+    const addedAccentColor = 'rgb(171, 242, 188)';


Is it possible to make these importable from their implementation file? Do these constants already exist?

Addressed in a follow-up commit

...public/detection_engine/rule_management/components/rule_details/json_diff/json_diff.test.tsx

jpdjere · 2024-02-15T12:37:29Z

...public/detection_engine/rule_management/components/rule_details/json_diff/json_diff.test.tsx

+ *
+ * The Matcher function itself has the following signature:
+ * @param {string} content - The content string to be checked. Not directly used in the current implementation
+ * but is required for the matcher's signature.


If I understand correctly, content needs to be passed here cause we need to return a function that is passed either to getByText and queryAllByText, and both those function require that signature.

Took me a bit to understand, maybe add this additional detail in this comment?

You're right, it's too complicated. So I refactored a bit and removed this helper altogether.

...public/detection_engine/rule_management/components/rule_details/json_diff/json_diff.test.tsx

jpdjere · 2024-02-19T09:34:01Z

...public/detection_engine/rule_management/components/rule_details/json_diff/json_diff.test.tsx

+      /*
+      Helper function to find an element with a particular text content.
+      React Testing Library's doesn't provide an easy way to search by text if text is split into multiple DOM elements.
+    */
+      const getChildByTextContent = (parent: Element, textContent: string): HTMLElement => {
+        return Array.from(parent.querySelectorAll('*')).find(
+          (childElement) => childElement.textContent === textContent
+        ) as HTMLElement;
+      };


Any reason this function and findDiffLineContaining cannot be extracted out to the top as helper functions, outside of the tests? This one is redefined on each it clause.

I thought it would be a bit easier to read if these functions are local to where they are used. But I understand that it's a matter of preference and the benefit is small, so I have no trouble moving these to the top level.

jpdjere · 2024-02-19T09:35:21Z

...public/detection_engine/rule_management/components/rule_details/json_diff/json_diff.test.tsx

+
+      const removedLineAfter = getChildByTextContent(removedLine, '');
+      expect(
+        removedLineAfter ? window.getComputedStyle(removedLineAfter).backgroundColor : null


Not sure I understand why these ternary operators are needed here. How can null ever be ''?

Good catch! Looks like I forgot to change this line after refactoring.

jpdjere

Approving in general, left a couple questions and nits.

Thanks for covering this feature with tests 👍 ✅

…ore upgrading" E2E test

… E2E test

…upgrade" with an RTL test

…ed" with an RTL test

…odified" test

…E2E test

…as modified" with an RTL test

… an RTL test

kibana-ci · 2024-02-19T15:12:52Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 3f6bac9

Failed CI Steps

FTR Configs #2

Test Failures

[job] [logs] FTR Configs #2 / discover/group2 adhoc data views should notify about invalid filter reffs

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	+39.0B

History

💛 Build #194125 was flaky a37e992
💛 Build #193960 was flaky 2f54479

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

cc @nikitaindik

**Resolves: elastic#166163 Flaky test runner runs: [1](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5189), [2](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5190), [3](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5191), [4](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5192) ## Summary This PR adds tests in accordance with the [test plan](elastic#175958) that was merged earlier. (cherry picked from commit cd374d2)

kibanamachine · 2024-02-19T15:17:02Z

💚 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

# Backport This will backport the following commits from `main` to `8.13`: - [[Security Solution] JSON diffs test coverage (#176770)](#176770)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Nikita Indik <[email protected]>

**Resolves: elastic#166163 Flaky test runner runs: [1](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5189), [2](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5190), [3](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5191), [4](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5192) ## Summary This PR adds tests in accordance with the [test plan](elastic#175958) that was merged earlier.

nikitaindik force-pushed the json-diffs-test-coverage branch 3 times, most recently from 20a4463 to 0ae76c5 Compare February 15, 2024 02:43

nikitaindik self-assigned this Feb 15, 2024

nikitaindik marked this pull request as ready for review February 15, 2024 02:52

nikitaindik requested a review from a team as a code owner February 15, 2024 02:52

nikitaindik requested a review from jpdjere February 15, 2024 02:52

jpdjere reviewed Feb 15, 2024

View reviewed changes

...s/cypress/e2e/detection_response/rule_management/prebuilt_rules/prebuilt_rules_preview.cy.ts Outdated Show resolved Hide resolved

maximpn reviewed Feb 15, 2024

View reviewed changes

jpdjere reviewed Feb 15, 2024

View reviewed changes

nikitaindik force-pushed the json-diffs-test-coverage branch from 0ae76c5 to 2f54479 Compare February 15, 2024 21:42

jpdjere reviewed Feb 19, 2024

View reviewed changes

jpdjere approved these changes Feb 19, 2024

View reviewed changes

banderror force-pushed the json-diffs-test-coverage branch from 2f54479 to a37e992 Compare February 19, 2024 10:48

nikitaindik added v8.14.0 and removed v8.12.2 labels Feb 19, 2024

nikitaindik added 2 commits February 19, 2024 14:03

Add helpers to check and switch active flyout tab

d6a7be2

Adjust existing "User can see correct rule information in preview bef…

f6df61e

…ore upgrading" E2E test

nikitaindik added 12 commits February 19, 2024 14:03

Cover "User can see changes in a side-by-side JSON diff view" with an…

e126ccd

… E2E test

Rename "Base version" -> "Current rule"

2708a9b

Cover "User can see precisely how property values would change after …

0c55b12

…upgrade" with an RTL test

Cover "Rule actions and exception lists should not be shown as modifi…

ab217e3

…ed" with an RTL test

Update the "Rule actions and exception lists should not be shown as m…

cbdc8c7

…odified" test

Cover "Technical properties should not be included in preview"

8cf8e37

Cover "Dynamic properties should not be included in preview" with an …

ed3495a

…E2E test

Cover "Properties with semantically equal values should not be shown …

098c734

…as modified" with an RTL test

Cover "Unchanged sections of a rule should be hidden by default" with…

133544d

… an RTL test

Cover "Properties should be sorted alphabetically" with an RTL test

cea7c40

Address code review feedback

690ea42

Small improvements after CR

3f6bac9

nikitaindik force-pushed the json-diffs-test-coverage branch from a37e992 to 3f6bac9 Compare February 19, 2024 13:29

nikitaindik enabled auto-merge (squash) February 19, 2024 13:30

nikitaindik merged commit cd374d2 into elastic:main Feb 19, 2024
36 of 37 checks passed

kibanamachine mentioned this pull request Feb 19, 2024

[8.13] [Security Solution] JSON diffs test coverage (#176770) #177209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] JSON diffs test coverage #176770

[Security Solution] JSON diffs test coverage #176770

nikitaindik commented Feb 12, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Feb 15, 2024

elasticmachine commented Feb 15, 2024

elasticmachine commented Feb 15, 2024

maximpn left a comment

maximpn Feb 15, 2024

nikitaindik Feb 15, 2024

maximpn Feb 15, 2024

nikitaindik Feb 15, 2024

maximpn Feb 15, 2024

nikitaindik Feb 15, 2024

jpdjere Feb 15, 2024

nikitaindik Feb 15, 2024

jpdjere Feb 15, 2024

nikitaindik Feb 15, 2024

jpdjere Feb 15, 2024

nikitaindik Feb 15, 2024

jpdjere Feb 19, 2024

nikitaindik Feb 19, 2024

jpdjere Feb 19, 2024

nikitaindik Feb 19, 2024

jpdjere left a comment

kibana-ci commented Feb 19, 2024

kibanamachine commented Feb 19, 2024

[Security Solution] JSON diffs test coverage #176770

[Security Solution] JSON diffs test coverage #176770

Conversation

nikitaindik commented Feb 12, 2024 • edited by kibanamachine Loading

Summary

elasticmachine commented Feb 15, 2024

elasticmachine commented Feb 15, 2024

elasticmachine commented Feb 15, 2024

maximpn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpdjere left a comment

Choose a reason for hiding this comment

kibana-ci commented Feb 19, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

History

kibanamachine commented Feb 19, 2024

💚 All backports created successfully

Questions ?

nikitaindik commented Feb 12, 2024 •

edited by kibanamachine

Loading