[Security Solution][Detections] Cleaning up mocks/tests #74920

rylnd · 2020-08-13T04:00:52Z

Summary

This is a chore PR that's a followup to #74582 and #75378 . Contents:

Followup to feedback on [Security Solution][Detections] Refactor ML calls for newest ML permissions #74582
Adds tests for useListsConfig hook
Adds .mock files in files/modules touched
Mocks cleanup
- Removes most uses of any with unknown
- Refactors our kibana_react mocks to be reusable
- Replaces our kibana_core mocks with mocks provided by core (which were just delegating directly to anyway)
- Removes other redundant mocks with existing ones
- Removes unnecessary mocks usage in tests
  - unnecessary calls to clearAll, etc.

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
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

* Simpler mock factory that returns an object instead of a thunk * We can use mockReturnValue instead of mockImplementation to accomplish the same * Allows us to replace createStartServices mock * Uses unknown instead of any for mocks

* Since our useKibana mock returns a consistent mock, we can modify its return value instead of re-mocking the entire thing * Removes unnecessary uses of clearing/resetting mocks * If your mocks are configured at the beginning of each test this is usually unnecessary. * I left one case of clearAllMocks in all_cases/index.test since it defined several mock functions that were persistent across tests, and it was easier than moving their definitions to a beforeEach * Removes some unnecessary overrides that seemed due to storage previously not being mocked

There's a good chance that the consumer might want the OTHER hook, so let's make that discoverable.

rylnd · 2020-08-13T04:05:41Z

x-pack/plugins/security_solution/public/cases/components/all_cases/index.test.tsx

@@ -97,23 +96,16 @@ describe('AllCases', () => {
  });
  /* eslint-enable no-console */
  beforeEach(() => {
-    jest.resetAllMocks();
+    jest.clearAllMocks();


I left this one instance of clearAllMocks as detailed in 0e5dfe3. Instead of defining mocks once and then clearing their state in a beforeEach, my preference is to instead define the mocks in the beforeEach. That's the pattern I followed for the other tests, this one was the exception.

rylnd · 2020-08-13T04:08:45Z

x-pack/plugins/security_solution/public/common/components/query_bar/index.test.tsx

      const Proxy = (props: QueryBarComponentProps) => (
        <TestProviders>
-          <KibanaWithStorageProvider services={{ storage: { get: jest.fn() } }}>


I think these tests must have been written before our kibana_react mocks (used by TestProviders) included storage mocks; this code has been unnecessary since that was added.

rylnd · 2020-08-13T04:11:37Z

x-pack/plugins/security_solution/public/common/mock/kibana_react.ts

@@ -66,61 +62,38 @@ export const createUseUiSettingMock = () => <T extends unknown = string>(
 export const createUseUiSetting$Mock = () => {
  const useUiSettingMock = createUseUiSettingMock();

-  return <T extends unknown = string>(


I removed these generics as they were a) wrong and b) not necessary since these are mocks; consumers can modify/cast these as necessary to fit their need.

One other note about these uiSettings mocks: they really shouldn't be needed, but we have >80 tests that rely on specific values being returned and I haven't yet had time to fix them individually.

* adds mocks for the hooks upon which it depends

Leverages this mock factory in our manual mock for this hook.

We're trying to consolidate mocks to this pattern so that they're easier to find and reuse.

This was only being consumed by our general createStartServicesMock.

This is just noise/tech debt, we should use the core mock directly when we can.

Instead let's just reference the core mocks themselves.

elasticmachine · 2020-08-20T17:27:21Z

Pinging @elastic/siem (Team:SIEM)

rylnd · 2020-08-20T17:27:36Z

@elasticmachine merge upstream

rylnd · 2020-08-20T20:43:24Z

@elasticmachine merge upstream

FrankHassanabad · 2020-08-24T15:56:09Z

@elasticmachine merge upstream

FrankHassanabad · 2020-08-24T16:00:24Z

x-pack/plugins/security_solution/public/cases/components/all_cases/index.test.tsx

@@ -28,7 +27,7 @@ jest.mock('../../containers/use_delete_cases');
 jest.mock('../../containers/use_get_cases');
 jest.mock('../../containers/use_get_cases_status');

-const useKibanaMock = useKibana as jest.Mock;
+const useKibanaMock = useKibana as jest.Mocked<typeof useKibana>;


FrankHassanabad · 2020-08-24T16:01:37Z

x-pack/plugins/security_solution/public/cases/components/all_cases/index.test.tsx

    useUpdateCasesMock.mockReturnValue(defaultUpdateCases);
    useGetCasesMock.mockReturnValue(defaultGetCases);
    useDeleteCasesMock.mockReturnValue(defaultDeleteCases);
    useGetCasesStatusMock.mockReturnValue(defaultCasesStatus);
    moment.tz.setDefault('UTC');
  });
+


Within the codebase there's like a weird style issue between newlines and not newlines between blocks of it and describe. No changes requested, just odd when I see two different styles and almost want a linter rule to happen at some point to just make it one way.

FrankHassanabad · 2020-08-24T16:04:02Z

x-pack/plugins/security_solution/public/common/components/super_date_picker/index.test.tsx

@@ -17,7 +17,7 @@ import {
  kibanaObservable,
  createSecuritySolutionStorageMock,
 } from '../../mock';
-import { createUseUiSetting$Mock } from '../../mock/kibana_react';
+import { createUseUiSetting$Mock } from '../../lib/kibana/kibana_react.mock';


Nice cleanup! Love how the mock files pattern is paying off more and more and how it gets easier to see where we can remove ours for theirs and reduce code in the code base.

FrankHassanabad · 2020-08-24T16:06:44Z

x-pack/plugins/security_solution/public/common/lib/kibana/kibana_react.mock.ts

+    return defaultValue;
+  }
+
+  throw new Error(`Unexpected config key: ${key}`);


nit: These are usually throw new TypeError?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypeError

No worries as I sometimes do this too here, just pointing it out as the better type.

Good catch, I'll change this to make it as accurate a mock as possible. I don't like having this mock, but as mentioned elsewhere there are a bunch of unit tests that are currently relying on "real" UI settings values, so removing it will take a little effort.

FrankHassanabad · 2020-08-24T16:07:55Z

...curity_solution/public/detections/containers/detection_engine/lists/use_lists_config.mock.ts

+
+import { UseListsConfigReturn } from './use_lists_config';
+
+export const getUseListsConfigMock: () => jest.Mocked<UseListsConfigReturn> = () => ({


👍 jest.Mocked<UseListsConfigReturn>

FrankHassanabad

Code clean up and removal and fixing of types! Who could ask for more on a fresh Monday morning.

LGTM!

Throw an error of the same type if an unexpected key is used.

kibanamachine · 2020-08-24T20:31:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 599dc15

Build metrics

async chunks size

id	value	diff	baseline
securitySolution	7.2MB	+36.0B	7.2MB

History

💚 Build #70044 succeeded 6688b3c
💚 Build #69535 succeeded 257adde
💚 Build #69273 succeeded 47acf27
💔 Build #67857 failed b1af143

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

* Simplify our kibana mocks * Simpler mock factory that returns an object instead of a thunk * We can use mockReturnValue instead of mockImplementation to accomplish the same * Allows us to replace createStartServices mock * Uses unknown instead of any for mocks * Clean up our manual use of kibana mocks in tests * Since our useKibana mock returns a consistent mock, we can modify its return value instead of re-mocking the entire thing * Removes unnecessary uses of clearing/resetting mocks * If your mocks are configured at the beginning of each test this is usually unnecessary. * I left one case of clearAllMocks in all_cases/index.test since it defined several mock functions that were persistent across tests, and it was easier than moving their definitions to a beforeEach * Removes some unnecessary overrides that seemed due to storage previously not being mocked * Rename some old occurrences of SIEM * Cross-reference similar hooks via JSDoc There's a good chance that the consumer might want the OTHER hook, so let's make that discoverable. * Adds jest tests for our useListsConfig hook * adds mocks for the hooks upon which it depends * Add a mock for our useListsConfig hook Leverages this mock factory in our manual mock for this hook. * Remove unneeded eslint exception * Move kibana_react mocks into their own .mock file We're trying to consolidate mocks to this pattern so that they're easier to find and reuse. * Remove intermediate mock factory This was only being consumed by our general createStartServicesMock. * Replace security_solution's alias for a core mock This is just noise/tech debt, we should use the core mock directly when we can. * Remove unnecessary wrapper around core mocks Instead let's just reference the core mocks themselves. * Remove outdated references from upstream * More accurate mock Throw an error of the same type if an unexpected key is used. Co-authored-by: Elastic Machine <[email protected]>

) * Simplify our kibana mocks * Simpler mock factory that returns an object instead of a thunk * We can use mockReturnValue instead of mockImplementation to accomplish the same * Allows us to replace createStartServices mock * Uses unknown instead of any for mocks * Clean up our manual use of kibana mocks in tests * Since our useKibana mock returns a consistent mock, we can modify its return value instead of re-mocking the entire thing * Removes unnecessary uses of clearing/resetting mocks * If your mocks are configured at the beginning of each test this is usually unnecessary. * I left one case of clearAllMocks in all_cases/index.test since it defined several mock functions that were persistent across tests, and it was easier than moving their definitions to a beforeEach * Removes some unnecessary overrides that seemed due to storage previously not being mocked * Rename some old occurrences of SIEM * Cross-reference similar hooks via JSDoc There's a good chance that the consumer might want the OTHER hook, so let's make that discoverable. * Adds jest tests for our useListsConfig hook * adds mocks for the hooks upon which it depends * Add a mock for our useListsConfig hook Leverages this mock factory in our manual mock for this hook. * Remove unneeded eslint exception * Move kibana_react mocks into their own .mock file We're trying to consolidate mocks to this pattern so that they're easier to find and reuse. * Remove intermediate mock factory This was only being consumed by our general createStartServicesMock. * Replace security_solution's alias for a core mock This is just noise/tech debt, we should use the core mock directly when we can. * Remove unnecessary wrapper around core mocks Instead let's just reference the core mocks themselves. * Remove outdated references from upstream * More accurate mock Throw an error of the same type if an unexpected key is used. Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

elasticmachine · 2021-09-23T14:27:01Z

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

rylnd added 4 commits August 12, 2020 22:30

Simplify our kibana mocks

d60f4d1

* Simpler mock factory that returns an object instead of a thunk * We can use mockReturnValue instead of mockImplementation to accomplish the same * Allows us to replace createStartServices mock * Uses unknown instead of any for mocks

Rename some old occurrences of SIEM

b6a426e

Cross-reference similar hooks via JSDoc

b1af143

There's a good chance that the consumer might want the OTHER hook, so let's make that discoverable.

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

rylnd self-assigned this Aug 13, 2020

rylnd commented Aug 13, 2020

View reviewed changes

rylnd requested review from spong and cnasikas August 13, 2020 04:15

rylnd added 9 commits August 19, 2020 17:06

Adds jest tests for our useListsConfig hook

476bd93

* adds mocks for the hooks upon which it depends

Add a mock for our useListsConfig hook

4c80f68

Leverages this mock factory in our manual mock for this hook.

Remove unneeded eslint exception

c932c90

Move kibana_react mocks into their own .mock file

3f26f06

We're trying to consolidate mocks to this pattern so that they're easier to find and reuse.

Remove intermediate mock factory

f925518

This was only being consumed by our general createStartServicesMock.

Replace security_solution's alias for a core mock

a609e69

This is just noise/tech debt, we should use the core mock directly when we can.

Remove unnecessary wrapper around core mocks

cc84735

Instead let's just reference the core mocks themselves.

Merge branch 'master' into mocks_cleanup

9ff3a4a

Remove outdated references from upstream

47acf27

rylnd requested a review from yctercero August 20, 2020 00:52

rylnd marked this pull request as ready for review August 20, 2020 17:27

rylnd requested review from a team as code owners August 20, 2020 17:27

Merge branch 'master' into mocks_cleanup

058b291

Merge branch 'master' into mocks_cleanup

257adde

Merge branch 'master' into mocks_cleanup

6688b3c

FrankHassanabad reviewed Aug 24, 2020

View reviewed changes

FrankHassanabad approved these changes Aug 24, 2020

View reviewed changes

More accurate mock

599dc15

Throw an error of the same type if an unexpected key is used.

rylnd merged commit 0758df8 into elastic:master Aug 24, 2020

rylnd mentioned this pull request Aug 24, 2020

[7.x] [Security Solution][Detections] Cleaning up mocks/tests (#74920) #75816

Merged

rylnd deleted the mocks_cleanup branch August 24, 2020 20:51

tylersmalley mentioned this pull request Aug 26, 2020

[Security Solution][Exceptions] - Improve UX for missing exception list associated with rule #75898

Merged

5 tasks

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] Cleaning up mocks/tests #74920

[Security Solution][Detections] Cleaning up mocks/tests #74920

rylnd commented Aug 13, 2020 •

edited

Loading

rylnd Aug 13, 2020

rylnd Aug 13, 2020

rylnd Aug 13, 2020

rylnd Aug 13, 2020

elasticmachine commented Aug 20, 2020

rylnd commented Aug 20, 2020

rylnd commented Aug 20, 2020

FrankHassanabad commented Aug 24, 2020

FrankHassanabad Aug 24, 2020

FrankHassanabad Aug 24, 2020

FrankHassanabad Aug 24, 2020

FrankHassanabad Aug 24, 2020

rylnd Aug 24, 2020

FrankHassanabad Aug 24, 2020

FrankHassanabad left a comment

kibanamachine commented Aug 24, 2020

elasticmachine commented Sep 23, 2021


		import { UseListsConfigReturn } from './use_lists_config';

		export const getUseListsConfigMock: () => jest.Mocked<UseListsConfigReturn> = () => ({

[Security Solution][Detections] Cleaning up mocks/tests #74920

[Security Solution][Detections] Cleaning up mocks/tests #74920

Conversation

rylnd commented Aug 13, 2020 • edited Loading

Summary

Checklist

For maintainers

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

elasticmachine commented Aug 20, 2020

rylnd commented Aug 20, 2020

rylnd commented Aug 20, 2020

FrankHassanabad commented Aug 24, 2020

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 left a comment

Choose a reason for hiding this comment

kibanamachine commented Aug 24, 2020

💚 Build Succeeded

Build metrics

async chunks size

History

elasticmachine commented Sep 23, 2021

rylnd commented Aug 13, 2020 •

edited

Loading