[SIEM] [Detection Engine] Increase unit test coverage #58274

dhurley14 · 2020-02-21T20:13:43Z

Summary

Updates unit test coverage to 100% for routes and other files that had pre-existing test files written. Fixes some bugs / removal of dead code.

Checklist

Delete any items that are not applicable to this PR.

~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~

Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~
~~- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser~~
~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-02-21T20:34:57Z

Pinging @elastic/siem (Team:SIEM)

FrankHassanabad · 2020-02-24T06:13:57Z

@elasticmachine merge upstream

yctercero

Thanks for going over this with us! Added comment we discussed over zoom.

yctercero · 2020-02-21T21:23:38Z

...legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts

+    clients.alertsClient.get.mockResolvedValue(getResult());
+    clients.actionsClient.create.mockResolvedValue(createActionResult());
+    clients.alertsClient.create.mockResolvedValue(getResult());
+    jest.spyOn(readRules, 'readRules').mockImplementation(async () => {


I like this. Much cleaner than the way I did it on import_rules_route test 👍

yctercero · 2020-02-24T17:05:12Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts

@@ -256,8 +254,6 @@ export const getTupleDuplicateErrorsAndUniqueRules = (
            );
          }
          acc.rulesAcc.set(ruleId, parsedRule);
-        } else {


See why it would seem this is never hit when running end to end tests since it stops you from creating a request without rule_id, but can hit it on unit tests. May be good to keep as it is * possibly * an attainable case.

Good call. I added it back here 640d041 and added an additional unit test to show the validation on the route when ruleId is missing from rules on import.

…r catching errors at the route level for prepackaged rules route

…sulted in 200, but was still throwing an error), adds tests for covering case where a rule with a matching rule id is found to have already existed, adds a test covering the case where createRules throws an error and that error is caught in the catch block, and appears in the response

…tate used by spyOn is not carried over between tests, increases test coverage for create rules route

… error on patch rules route

…Rules throws an error on read_rules_route

…Removed unreachable code.

…onversion and returns arrays of tranformers to be used in createPromiseFromStreams so that unhandled promise rejections can be handled appropriately.

…ause we are no longer passing in the current status, we are passing in the whole saved object.

…t by object ids

…undefined bug in catch block

…les function and adds a test case for when an attempt to import rules with missing ruleIds is made, and the expected outcome.

FrankHassanabad · 2020-02-24T23:54:43Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_route.test.ts

@@ -29,8 +32,12 @@ describe('create_rules', () => {
  let clients = clientsServiceMock.createClients();

  beforeEach(() => {
+    // jest carries state between mocked implementations when using
+    // spyOn. So now we're doing all three of these.
+    // https://github.com/facebook/jest/issues/7136#issuecomment-565976599


Thanks for the link and the interesting comment that talks about it. I think that maybe our tests and techniques are growing to such a degree we almost need a bit more stable location for all of our test utils and workaround otherwise I would maybe move this section to a localized __mocks__/utils.ts to reduce boilerplate?

Optional: this is still fine as is.

FrankHassanabad · 2020-02-24T23:56:29Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_route.test.ts

+      clients.alertsClient.find.mockResolvedValue(getFindResult());
+      clients.alertsClient.get.mockResolvedValue(getResult());
+      clients.alertsClient.create.mockResolvedValue(getResult());
+      clients.savedObjectsClient.find.mockResolvedValue(getFindResultStatus());


As some of these become repeating patterns of:

clients.actionsClient.create.mockResolvedValue(createActionResult()); clients.alertsClient.find.mockResolvedValue(getFindResult()); clients.alertsClient.get.mockResolvedValue(getResult()); clients.alertsClient.create.mockResolvedValue(getResult()); clients.savedObjectsClient.find.mockResolvedValue(getFindResultStatus());

I wonder if they could be moved into a test utils or moved into a local function for this file? Optional, just thinking of ways we could reduce boilerplate as we gain collections of tests. Sometimes you can't as each block of code is just so slightly different than the other ones.

@FrankHassanabad @dhurley14 since this is all changing in #58292 my preference would be to defer any significant refactoring here.

To your point, though, my plan is to define a createMockRequestHandlerContext helper that's prepopulated with the plugin-defined mocks. As tests need to override these behaviors, they can do so in a similar manner to what we have now:

describe('a test', () => { let context = createMockRequestHandlerContext(); beforeEach(() => { context = createMockRequestHandlerContext(); context.alerting.getAlertsClient.mockReturnValue(myMockAlertsClient); }); it('works', () => { const request = httpServerMock.createKibanaRequest(); const responseSpy = { ok: jest.fn() }; handler(context, request, responseSpy); expect(responseSpy.ok).toHaveBeenCalledWith(...); }); });

While I can't yet speak to the extend of the issue, some cursory investigation showed that some of these mocked values are unnecessary (I commented out a few mocks in a few tests, and most of the tests continued to pass); my hope is that these third-party mocks will cut down significantly on the verbosity of these tests.

Sounds good @rylnd , totally agree.

FrankHassanabad · 2020-02-24T23:57:21Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/delete_rules_bulk_route.ts

@@ -58,7 +58,7 @@ export const createDeleteRulesBulkRoute = (getClients: GetScopedClients): Hapi.S
              ruleStatuses.saved_objects.forEach(async obj =>
                savedObjectsClient.delete(ruleStatusSavedObjectType, obj.id)
              );
-              return transformOrBulkError(idOrRuleIdOrUnknown, rule);
+              return transformOrBulkError(idOrRuleIdOrUnknown, rule, ruleStatuses);


Looks like a bug fix we are going to be backporting 👍here found by tests

FrankHassanabad · 2020-02-25T00:00:10Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/find_rules_status_route.ts

+          status_code: error.statusCode,
+        })
+        .code(error.statusCode);
+    }


I see, I see.... This is all wrapped in a try / catch for safety. 👍

This is good.

FrankHassanabad · 2020-02-25T00:06:39Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts

@@ -195,7 +193,7 @@ export const transformOrBulkError = (
  ruleStatus?: unknown
 ): Partial<OutputRuleAlertRest> | BulkError => {
  if (isAlertType(alert)) {
-    if (isRuleStatusFindType(ruleStatus)) {
+    if (isRuleStatusFindType(ruleStatus) && ruleStatus?.saved_objects.length > 0) {


Looking at the isRuleStatusFindType(ruleStatus) you shouldn't have to do a double tap with ruleStatus? as that is check should return false and never an undefined or null if ruleStatus is null?

But it's ok to keep as the line below has it and you're doing a back port. But with all the tests you could push in a null to this test or the isRuleStatusFindType and see that it will return a boolean false and not a true for ruleStatus

Ah yes that is true. I also can probably remove the nullish coalescing and just grab the first object from the saved_objects array. With the isRuleStatusFindType I am pretty sure that should remove any need for having the nullish coalescing.

Working through this fix I found a pre-existing bug which I will open a fix for in a separate PR.

FrankHassanabad · 2020-02-25T00:25:11Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/utils.ts

+    const newKey = snakeCase(item);
+    return { ...acc, [newKey]: obj[item] };
+  }, {});
+};


You can clean this up where you don't have that return null and you're not using an any:

export const convertToSnakeCase = <T extends Record<string, unknown>>(obj: T): Partial<T> => { return Object.keys(obj).reduce((acc, item) => { const newKey = snakeCase(item); return { ...acc, [newKey]: obj[item] }; }, {}); };

Worked on my machine at least.

FrankHassanabad · 2020-02-25T00:40:05Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/read_rules.test.ts

+      const alertsClient = alertsClientMock.create();
+      const { alertTypeId, ...rest } = getResult();
+      // @ts-ignore
+      alertsClient.get.mockImplementation(() => rest);


Typescript is the biggest best friends of best friends here. Never ts-ignore your friends. ;-)

This looks like what you really intended for was this:

const { alertTypeId, ...rest } = getResult(); alertsClient.get.mockResolvedValue((rest as unknown) as RuleAlertType);

or another way that is still type safe would be just a mutatious delete like so:

const alertsClient = alertsClientMock.create(); const result = getResult(); delete result.alertTypeId; alertsClient.get.mockResolvedValue(result);

Then you don't have to worry about doing odd casting since delete is a side effect and get no unknown.

What's tricky is that this test is incredibly easy to have false positives and by deleting this code the test still passes, so buyer beware.

I will revisit this one. I definitely don't want false positives.

FrankHassanabad · 2020-02-25T00:43:43Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/read_rules.test.ts

+      const alertsClient = alertsClientMock.create();
+      alertsClient.get.mockResolvedValue(getResult());
+      // @ts-ignore
+      alertsClient.find.mockResolvedValue({ data: [] });


I really would avoid ts-ignore at all if possible. It is a really terrible thing to use as only last resort.

In this unit test you can either do the unknown cast trick or just flush it out normally:

alertsClient.find.mockResolvedValue({ data: [], page: 0, perPage: 1, total: 0 });

I would flush it out normally as a regular test case.

Also you can just use the typical find result mock which already returns an empty value:

alertsClient.find.mockResolvedValue(getFindResult());

It's just poorly named as getFindResult() when really it returns an empty find.

FrankHassanabad · 2020-02-25T00:45:22Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/read_rules.ts

@@ -31,7 +31,7 @@ export const readRules = async ({
        return null;
      }
    } catch (err) {
-      if (err.output.statusCode === 404) {
+      if (err?.output?.statusCode === 404) {


FrankHassanabad

Left some comments and I only really feel strongly about any ts-ignore's being added.

Outside of those, everything else I think is great and I still approve this pull request as so many great unit tests are so greatly appreciated.

kibanamachine · 2020-02-25T01:10:08Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 640d041

History

💚 Build #28362 succeeded c1c724e
💚 Build #28233 succeeded 05011ec

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

* updates read privileges route tests * fixes test for 404 when alertsClient is not present, adds new test for catching errors at the route level for prepackaged rules route * fixes test for happy path on create rules bulk route (missing mock resulted in 200, but was still throwing an error), adds tests for covering case where a rule with a matching rule id is found to have already existed, adds a test covering the case where createRules throws an error and that error is caught in the catch block, and appears in the response * adds more jest functions to beforeEach to ensure mockImplementation state used by spyOn is not carried over between tests, increases test coverage for create rules route * updates unit test coverage for delete rules route and bulk delete * increase unit test coverage for find_rules_route * add test case to get_prepackaged_rules where findRules throws an error * adds unit test for missing alertsClient in patch_rules_bulk route * adds unit test coverage for transform error and pathRules throwing an error on patch rules route * adds unit test coverage for rule not found, transform error, and readRules throws an error on read_rules_route * adds unit test coverage for update rules (bulk) routes * increases unit test coverage for open close signals route * updates coverage for signals query route * adds unit tests for rules status route, updates utils test coverage. Removed unreachable code. * updates test coverage, removes usage of pipes from ndjson to stream conversion and returns arrays of tranformers to be used in createPromiseFromStreams so that unhandled promise rejections can be handled appropriately. * fixes type errors * fix bug on transform when rulestatus saved objects array is empty because we are no longer passing in the current status, we are passing in the whole saved object. * adds unit test for when readRules throws an error inside of get export by object ids * adds unit tests for catching errors on read rules and fixes property undefined bug in catch block * removes unused function from utils * adds the 'else' clause back to the getTupleDuplicateErrorsAndUniqueRules function and adds a test case for when an attempt to import rules with missing ruleIds is made, and the expected outcome.

* updates read privileges route tests * fixes test for 404 when alertsClient is not present, adds new test for catching errors at the route level for prepackaged rules route * fixes test for happy path on create rules bulk route (missing mock resulted in 200, but was still throwing an error), adds tests for covering case where a rule with a matching rule id is found to have already existed, adds a test covering the case where createRules throws an error and that error is caught in the catch block, and appears in the response * adds more jest functions to beforeEach to ensure mockImplementation state used by spyOn is not carried over between tests, increases test coverage for create rules route * updates unit test coverage for delete rules route and bulk delete * increase unit test coverage for find_rules_route * add test case to get_prepackaged_rules where findRules throws an error * adds unit test for missing alertsClient in patch_rules_bulk route * adds unit test coverage for transform error and pathRules throwing an error on patch rules route * adds unit test coverage for rule not found, transform error, and readRules throws an error on read_rules_route * adds unit test coverage for update rules (bulk) routes * increases unit test coverage for open close signals route * updates coverage for signals query route * adds unit tests for rules status route, updates utils test coverage. Removed unreachable code. * updates test coverage, removes usage of pipes from ndjson to stream conversion and returns arrays of tranformers to be used in createPromiseFromStreams so that unhandled promise rejections can be handled appropriately. * fixes type errors * fix bug on transform when rulestatus saved objects array is empty because we are no longer passing in the current status, we are passing in the whole saved object. * adds unit test for when readRules throws an error inside of get export by object ids * adds unit tests for catching errors on read rules and fixes property undefined bug in catch block * removes unused function from utils * adds the 'else' clause back to the getTupleDuplicateErrorsAndUniqueRules function and adds a test case for when an attempt to import rules with missing ruleIds is made, and the expected outcome. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

dhurley14 self-assigned this Feb 21, 2020

dhurley14 added release_note:skip Skip the PR/issue when compiling release notes Team:SIEM v7.6.1 v7.7.0 labels Feb 21, 2020

FrankHassanabad mentioned this pull request Feb 21, 2020

[SIEM][Detection Engine][Tech Debt] Technical must haves and nice to haves #56798

Closed

10 tasks

yctercero reviewed Feb 24, 2020

View reviewed changes

dhurley14 added 21 commits February 24, 2020 18:18

updates read privileges route tests

b22677e

fixes test for 404 when alertsClient is not present, adds new test fo…

37d6f3d

…r catching errors at the route level for prepackaged rules route

adds more jest functions to beforeEach to ensure mockImplementation s…

cf5e4b5

…tate used by spyOn is not carried over between tests, increases test coverage for create rules route

updates unit test coverage for delete rules route and bulk delete

27222ae

increase unit test coverage for find_rules_route

7a79510

add test case to get_prepackaged_rules where findRules throws an error

c46caf6

adds unit test for missing alertsClient in patch_rules_bulk route

8d7cff2

adds unit test coverage for transform error and pathRules throwing an…

32c6d41

… error on patch rules route

adds unit test coverage for rule not found, transform error, and read…

ff2de45

…Rules throws an error on read_rules_route

adds unit test coverage for update rules (bulk) routes

d6f98d3

increases unit test coverage for open close signals route

c12ce9e

updates coverage for signals query route

3c4ea2a

adds unit tests for rules status route, updates utils test coverage. …

89a35b2

…Removed unreachable code.

updates test coverage, removes usage of pipes from ndjson to stream c…

adc250c

…onversion and returns arrays of tranformers to be used in createPromiseFromStreams so that unhandled promise rejections can be handled appropriately.

fixes type errors

db0aaaf

fix bug on transform when rulestatus saved objects array is empty bec…

b049a84

…ause we are no longer passing in the current status, we are passing in the whole saved object.

adds unit test for when readRules throws an error inside of get expor…

e05ff4d

…t by object ids

adds unit tests for catching errors on read rules and fixes property …

17affb2

…undefined bug in catch block

removes unused function from utils

82835b9

adds the 'else' clause back to the getTupleDuplicateErrorsAndUniqueRu…

640d041

…les function and adds a test case for when an attempt to import rules with missing ruleIds is made, and the expected outcome.

dhurley14 force-pushed the inc-unit-tests branch from c1c724e to 640d041 Compare February 24, 2020 23:18

FrankHassanabad added the v8.0.0 label Feb 24, 2020

FrankHassanabad reviewed Feb 24, 2020

View reviewed changes

FrankHassanabad reviewed Feb 25, 2020

View reviewed changes

FrankHassanabad approved these changes Feb 25, 2020

View reviewed changes

dhurley14 merged commit 10bf90f into elastic:master Feb 25, 2020

dhurley14 mentioned this pull request Feb 25, 2020

[7.x] [SIEM] [Detection Engine] Increase unit test coverage (#58274) #58480

Merged

dhurley14 mentioned this pull request Feb 25, 2020

[7.6] [SIEM] [Detection Engine] Increase unit test coverage (#58274) #58544

Merged

dhurley14 mentioned this pull request Feb 27, 2020

[SIEM] [Detection Engine] Remove unnecessary ts-ignores #58689

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SIEM] [Detection Engine] Increase unit test coverage #58274

[SIEM] [Detection Engine] Increase unit test coverage #58274

dhurley14 commented Feb 21, 2020

elasticmachine commented Feb 21, 2020

FrankHassanabad commented Feb 24, 2020

yctercero left a comment

yctercero Feb 21, 2020

dhurley14 Feb 24, 2020

yctercero Feb 24, 2020

dhurley14 Feb 24, 2020

FrankHassanabad Feb 24, 2020

FrankHassanabad Feb 24, 2020

rylnd Feb 25, 2020

FrankHassanabad Feb 25, 2020

FrankHassanabad Feb 24, 2020

FrankHassanabad Feb 25, 2020

FrankHassanabad Feb 25, 2020

dhurley14 Feb 25, 2020

dhurley14 Feb 25, 2020

FrankHassanabad Feb 25, 2020

FrankHassanabad Feb 25, 2020 •

edited

Loading

dhurley14 Feb 25, 2020

FrankHassanabad Feb 25, 2020

FrankHassanabad Feb 25, 2020

FrankHassanabad left a comment

kibanamachine commented Feb 25, 2020

[SIEM] [Detection Engine] Increase unit test coverage #58274

[SIEM] [Detection Engine] Increase unit test coverage #58274

Conversation

dhurley14 commented Feb 21, 2020

Summary

Checklist

For maintainers

elasticmachine commented Feb 21, 2020

FrankHassanabad commented Feb 24, 2020

yctercero 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

FrankHassanabad Feb 25, 2020 • edited Loading

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

FrankHassanabad left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 25, 2020

💚 Build Succeeded

History

FrankHassanabad Feb 25, 2020 •

edited

Loading