Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add more rule_registry unit tests #120323

Merged
merged 8 commits into from
Dec 7, 2021

Conversation

mgiota
Copy link
Contributor

@mgiota mgiota commented Dec 3, 2021

Fixes #120322

@mgiota mgiota self-assigned this Dec 3, 2021
@mgiota mgiota added Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" Theme: rac label obsolete v8.0.0 v8.1.0 auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes labels Dec 3, 2021
import { AlertConsumers } from '@kbn/rule-data-utils/alerts_as_data_rbac';

import { Dataset } from './index_options';
jest.mock('../rule_data_client/rule_data_client');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weltenwort Before I open this PR for review, I would like your input regarding what's the recommended way to mock a class constructor. Later in my test I do

expect(
        jest.requireMock('../rule_data_client/rule_data_client').RuleDataClient
      ).toHaveBeenCalled();

and it works. I am just wondering if there's a better way.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like there already is a mock implementation of the rule data client in

export const createRuleDataClientMock = (
indexName: string = '.alerts-security.alerts'
): RuleDataClientMock => {
const bulk = jest.fn();
const search = jest.fn();
const getDynamicIndexPattern = jest.fn();
return {
indexName,
kibanaVersion: '7.16.0',
isWriteEnabled: jest.fn(() => true),
// @ts-ignore 4.3.5 upgrade
getReader: jest.fn((_options?: { namespace?: string }) => ({
search,
getDynamicIndexPattern,
})),
getWriter: jest.fn(() => ({
bulk,
})),
};
};
. So maybe you could use that like this:

jest.mock('../rule_data_client/rule_data_client', () => ({
  RuleDataClient: jest.fn().mockImplementation(createRuleDataClientMock),
}));

And further down I saw you're calling jest.mock again. Instead I would recommend to just import the RuleDataClient normally. The jest mocking mechanism will make sure it's replaced with the mock.

Similarly, it looks like you're calling jest.mock('./resource_installer', () => { inside of a test case. I think it's safer to only call jest.mock on the top level of the file as it needs to be evaluated before any import.

@mgiota mgiota marked this pull request as ready for review December 6, 2021 08:38
@mgiota
Copy link
Contributor Author

mgiota commented Dec 6, 2021

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @mgiota

import { RuleDataClient } from '../rule_data_client/rule_data_client';
import { createRuleDataClientMock as mockCreateRuleDataClient } from '../rule_data_client/rule_data_client.mock';

jest.mock('../rule_data_client/rule_data_client', () => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually this means we don't use any dependency injection model :)
Apart from this, the tests look good :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ersin-erdal Exactly. I had troubles mocking constructors like RuleDataClient or ResourceInstaller in the rule_data_plugin_service.test. I was thinking to refactor using dependency injection instead of directly importing the dependencies. Let's discuss about this during our today's meeting

@mgiota mgiota merged commit 93c445f into elastic:main Dec 7, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Dec 7, 2021
* add more rule_registry unit tests

* delete initializeService test

* refactor and use the existing ruleDataClient mock

* remove resource installer mock

* check ruleDataClient has been called with correct arguments

* resource installer attempt

* remove failing tests

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
8.0

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Dec 7, 2021
* add more rule_registry unit tests

* delete initializeService test

* refactor and use the existing ruleDataClient mock

* remove resource installer mock

* check ruleDataClient has been called with correct arguments

* resource installer attempt

* remove failing tests

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: mgiota <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 7, 2021
…-chromium-before-compiling-pdf

* 'main' of github.com:elastic/kibana: (121 commits)
  FTR should use the new kibana_system user (elastic#120436)
  [Lens] Temporarily exclude Mosaic/Waffle from the suggestions list (elastic#120488)
  [Reporting] Fix slow CSV with large max size bytes (elastic#120365)
  [CTI] Threat Intel Card on Overview page needs to accommodate Fleet TI integrations -  (elastic#120459)
  [Dashboard] Added KibanaThemeProvider.  (elastic#120122)
  add more rule_registry unit tests (elastic#120323)
  [Lens] update xpack.lens.pie.smallValuesWarningMessage text (elastic#120478)
  [Fleet] Simplified package policy create API, enriching default values (elastic#119739)
  mock `elastic-apm-node` in `@kbn/test` jest preset (elastic#120324)
  [RAC] Rename occurrences of alert_type to rule_type in Infra (elastic#120455)
  [Security Solutions] Removes tech debt of exporting all from linter rule for timeline plugin (elastic#120437)
  [Workplace Search] Add API Keys view to replace Access tokens (elastic#120147)
  [Security Solutions] Removes tech debt of exporting all from linter rule for cases plugin in the server section (elastic#120411)
  Add support for threat.feed.name (elastic#120250)
  [Rule Registry] Rewrite APM registry rules for Observability (elastic#117740)
  [docs] Fix artifact layout formatting (elastic#119386)
  [Workplace Search] Add a technical preview of connecting GitHub via GitHub apps (elastic#119764)
  add osquery notes for 7.16 (elastic#120407)
  chore(NA): splits types from code on @kbn/docs-utils (elastic#120533)
  [Reporting] Decouple screenshotting plugin from the reporting (elastic#120110)
  ...

# Conflicts:
#	x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.test.ts
#	x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.ts
#	x-pack/plugins/reporting/server/test_helpers/create_mock_browserdriverfactory.ts
TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
* add more rule_registry unit tests

* delete initializeService test

* refactor and use the existing ruleDataClient mock

* remove resource installer mock

* check ruleDataClient has been called with correct arguments

* resource installer attempt

* remove failing tests

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" Theme: rac label obsolete v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RAC] Add unit tests for rule_registry plugin
5 participants