From 7631b0cf3fd5a86edd57c430742f807e839ca334 Mon Sep 17 00:00:00 2001 From: Konrad Lalik Date: Tue, 22 Aug 2023 13:50:41 +0200 Subject: [PATCH] Alerting: Improve tests setup (#73316) --- .../unified/RuleEditorCloudRules.test.tsx | 79 +++++++------------ .../app/features/alerting/unified/TESTING.md | 37 +++++++++ .../rule-viewer/RuleViewer.v1.test.tsx | 25 +----- .../app/features/alerting/unified/mockApi.ts | 36 ++++++++- public/app/features/alerting/unified/mocks.ts | 25 +++++- .../alerting/unified/testSetup/datasources.ts | 16 ++++ .../unified/testSetup/featureDiscovery.ts | 31 ++++++++ public/app/types/unified-alerting-dto.ts | 1 + 8 files changed, 175 insertions(+), 75 deletions(-) create mode 100644 public/app/features/alerting/unified/TESTING.md create mode 100644 public/app/features/alerting/unified/testSetup/datasources.ts create mode 100644 public/app/features/alerting/unified/testSetup/featureDiscovery.ts diff --git a/public/app/features/alerting/unified/RuleEditorCloudRules.test.tsx b/public/app/features/alerting/unified/RuleEditorCloudRules.test.tsx index 228a0980ade..4a6960d35a0 100644 --- a/public/app/features/alerting/unified/RuleEditorCloudRules.test.tsx +++ b/public/app/features/alerting/unified/RuleEditorCloudRules.test.tsx @@ -1,22 +1,21 @@ import { screen, waitFor, waitForElementToBeRemoved, within } from '@testing-library/react'; -import userEvent, { PointerEventsCheckLevel } from '@testing-library/user-event'; +import userEvent from '@testing-library/user-event'; import React from 'react'; import { renderRuleEditor, ui } from 'test/helpers/alertingRuleEditor'; import { clickSelectOption } from 'test/helpers/selectOptionInTest'; import { byRole } from 'testing-library-selector'; -import { setDataSourceSrv } from '@grafana/runtime'; import { contextSrv } from 'app/core/services/context_srv'; -import { PromApplication } from 'app/types/unified-alerting-dto'; import { searchFolders } from '../../manage-dashboards/state/actions'; -import { discoverFeatures } from './api/buildInfo'; import { fetchRulerRules, fetchRulerRulesGroup, fetchRulerRulesNamespace, setRulerRuleGroup } from './api/ruler'; import { ExpressionEditorProps } from './components/rule-editor/ExpressionEditor'; -import { disableRBAC, mockDataSource, MockDataSourceSrv } from './mocks'; +import { mockApi, mockFeatureDiscoveryApi, setupMswServer } from './mockApi'; +import { disableRBAC, mockDataSource } from './mocks'; import { fetchRulerRulesIfNotFetchedYet } from './state/actions'; -import * as config from './utils/config'; +import { setupDataSources } from './testSetup/datasources'; +import { buildInfoResponse } from './testSetup/featureDiscovery'; jest.mock('./components/rule-editor/ExpressionEditor', () => ({ // eslint-disable-next-line react/display-name @@ -25,17 +24,9 @@ jest.mock('./components/rule-editor/ExpressionEditor', () => ({ ), })); -jest.mock('./api/buildInfo'); jest.mock('./api/ruler'); jest.mock('../../../../app/features/manage-dashboards/state/actions'); -// there's no angular scope in test and things go terribly wrong when trying to render the query editor row. -// lets just skip it -jest.mock('app/features/query/components/QueryEditorRow', () => ({ - // eslint-disable-next-line react/display-name - QueryEditorRow: () =>

hi

, -})); - jest.mock('./components/rule-editor/util', () => { const originalModule = jest.requireActual('./components/rule-editor/util'); return { @@ -45,29 +36,19 @@ jest.mock('./components/rule-editor/util', () => { }); const dataSources = { - default: mockDataSource( - { - type: 'prometheus', - name: 'Prom', - isDefault: true, - }, - { alerting: true } - ), + default: mockDataSource({ type: 'prometheus', name: 'Prom', isDefault: true }, { alerting: true }), }; -jest.mock('@grafana/runtime', () => ({ - ...jest.requireActual('@grafana/runtime'), - getDataSourceSrv: jest.fn(() => ({ - getInstanceSettings: () => dataSources.default, - get: () => dataSources.default, - })), -})); - jest.mock('app/core/components/AppChrome/AppChromeUpdate', () => ({ AppChromeUpdate: ({ actions }: { actions: React.ReactNode }) =>
{actions}
, })); -jest.spyOn(config, 'getAllDataSources'); +setupDataSources(dataSources.default); + +const server = setupMswServer(); + +mockFeatureDiscoveryApi(server).discoverDsFeatures(dataSources.default, buildInfoResponse.mimir); +mockApi(server).eval({ results: {} }); // these tests are rather slow because we have to wait for various API calls and mocks to be called // and wait for the UI to be in particular states, drone seems to time out quite often so @@ -76,10 +57,8 @@ jest.spyOn(config, 'getAllDataSources'); jest.setTimeout(60 * 1000); const mocks = { - getAllDataSources: jest.mocked(config.getAllDataSources), searchFolders: jest.mocked(searchFolders), api: { - discoverFeatures: jest.mocked(discoverFeatures), fetchRulerRulesGroup: jest.mocked(fetchRulerRulesGroup), setRulerRuleGroup: jest.mocked(setRulerRuleGroup), fetchRulerRulesNamespace: jest.mocked(fetchRulerRulesNamespace), @@ -100,8 +79,6 @@ describe('RuleEditor cloud', () => { disableRBAC(); it('can create a new cloud alert', async () => { - setDataSourceSrv(new MockDataSourceSrv(dataSources)); - mocks.getAllDataSources.mockReturnValue(Object.values(dataSources)); mocks.api.setRulerRuleGroup.mockResolvedValue(); mocks.api.fetchRulerRulesNamespace.mockResolvedValue([]); mocks.api.fetchRulerRulesGroup.mockResolvedValue({ @@ -124,12 +101,7 @@ describe('RuleEditor cloud', () => { }); mocks.searchFolders.mockResolvedValue([]); - mocks.api.discoverFeatures.mockResolvedValue({ - application: PromApplication.Cortex, - features: { - rulerApiEnabled: true, - }, - }); + const user = userEvent.setup(); renderRuleEditor(); await waitForElementToBeRemoved(screen.getAllByTestId('Spinner')); @@ -137,10 +109,13 @@ describe('RuleEditor cloud', () => { const removeExpressionsButtons = screen.getAllByLabelText('Remove expression'); expect(removeExpressionsButtons).toHaveLength(2); + // Needs to wait for featrue discovery API call to finish - Check if ruler enabled + await waitFor(() => expect(screen.getByText('Switch to data source-managed alert rule')).toBeInTheDocument()); + const switchToCloudButton = screen.getByText('Switch to data source-managed alert rule'); expect(switchToCloudButton).toBeInTheDocument(); - await userEvent.click(switchToCloudButton); + await user.click(switchToCloudButton); //expressions are removed after switching to data-source managed expect(screen.queryAllByLabelText('Remove expression')).toHaveLength(0); @@ -148,30 +123,30 @@ describe('RuleEditor cloud', () => { expect(screen.getByTestId('datasource-picker')).toBeInTheDocument(); const dataSourceSelect = ui.inputs.dataSource.get(); - await userEvent.click(byRole('combobox').get(dataSourceSelect)); + await user.click(byRole('combobox').get(dataSourceSelect)); await clickSelectOption(dataSourceSelect, 'Prom (default)'); await waitFor(() => expect(mocks.api.fetchRulerRules).toHaveBeenCalled()); - await userEvent.type(await ui.inputs.expr.find(), 'up == 1'); + await user.type(await ui.inputs.expr.find(), 'up == 1'); - await userEvent.type(ui.inputs.name.get(), 'my great new rule'); + await user.type(ui.inputs.name.get(), 'my great new rule'); await clickSelectOption(ui.inputs.namespace.get(), 'namespace2'); await clickSelectOption(ui.inputs.group.get(), 'group2'); - await userEvent.type(ui.inputs.annotationValue(0).get(), 'some summary'); - await userEvent.type(ui.inputs.annotationValue(1).get(), 'some description'); + await user.type(ui.inputs.annotationValue(0).get(), 'some summary'); + await user.type(ui.inputs.annotationValue(1).get(), 'some description'); // TODO remove skipPointerEventsCheck once https://github.com/jsdom/jsdom/issues/3232 is fixed - await userEvent.click(ui.buttons.addLabel.get(), { pointerEventsCheck: PointerEventsCheckLevel.Never }); + await user.click(ui.buttons.addLabel.get()); - await userEvent.type(getLabelInput(ui.inputs.labelKey(0).get()), 'severity{enter}'); - await userEvent.type(getLabelInput(ui.inputs.labelValue(0).get()), 'warn{enter}'); + await user.type(getLabelInput(ui.inputs.labelKey(0).get()), 'severity{enter}'); + await user.type(getLabelInput(ui.inputs.labelValue(0).get()), 'warn{enter}'); // save and check what was sent to backend - await userEvent.click(ui.buttons.save.get()); + await user.click(ui.buttons.save.get()); await waitFor(() => expect(mocks.api.setRulerRuleGroup).toHaveBeenCalled()); expect(mocks.api.setRulerRuleGroup).toHaveBeenCalledWith( - { dataSourceName: 'Prom', apiVersion: 'legacy' }, + { dataSourceName: 'Prom', apiVersion: 'config' }, 'namespace2', { name: 'group2', diff --git a/public/app/features/alerting/unified/TESTING.md b/public/app/features/alerting/unified/TESTING.md new file mode 100644 index 00000000000..d09526860db --- /dev/null +++ b/public/app/features/alerting/unified/TESTING.md @@ -0,0 +1,37 @@ +# Alerting testing + +## Mocking API requests + +We should strive to use **MSW** for mocking API as often as possible. +It gives us the closest behaviour to the real server. + +`public/app/features/alerting/unified/mockApi.ts` contains helper functions that speed up mocking API configuration with MSW. + +If you don't find a helper for an endpoint you're looking for, please add it. + +**Mocking using MSW forces developers to handle loading states in tests which gives us a chance to discover UI inconsistencies at very early stages** + +### Common API requests + +- `/buildinfo` + Use `mockFeatureDiscoveryApi` and `buildInfoResponse` object to mock the endpoint response according to your needs +- `api/v1/eval` used by AlertingQueryRunner + Use `mockApi.eval` Usually an empty response should do the trick + +## Mocking data sources + +`public/app/features/alerting/unified/testSetup/datasources.ts` file contains functions facilitating setting up mock data sources. + +## Mocking permissions + +By default tests should be written with RBAC enabled. This is the most common scenario for our users. +Testing with RBAC disabled should be considered as an additional option when we already have tests for enabled RBAC. + +To enable or disable Role Based Access Control in tests use +`enableRBAC` or `disableRBAC` from `public/app/features/alerting/unified/mocks.ts` + +To grant a permission to a user use `grantUserPermission` from the same file. + +## Common patterns + +TODO diff --git a/public/app/features/alerting/unified/components/rule-viewer/RuleViewer.v1.test.tsx b/public/app/features/alerting/unified/components/rule-viewer/RuleViewer.v1.test.tsx index f37feab4b4c..b5eb6ca1452 100644 --- a/public/app/features/alerting/unified/components/rule-viewer/RuleViewer.v1.test.tsx +++ b/public/app/features/alerting/unified/components/rule-viewer/RuleViewer.v1.test.tsx @@ -3,7 +3,7 @@ import React from 'react'; import { TestProvider } from 'test/helpers/TestProvider'; import { byRole, byText } from 'testing-library-selector'; -import { config, locationService, setBackendSrv, setDataSourceSrv } from '@grafana/runtime'; +import { locationService, setBackendSrv } from '@grafana/runtime'; import { GrafanaRouteComponentProps } from 'app/core/navigation/types'; import { backendSrv } from 'app/core/services/backend_srv'; import { contextSrv } from 'app/core/services/context_srv'; @@ -20,13 +20,13 @@ import { getGrafanaRule, grantUserPermissions, mockDataSource, - MockDataSourceSrv, mockPromAlertingRule, mockRulerAlertingRule, promRuleFromRulerRule, } from '../../mocks'; import { mockAlertmanagerChoiceResponse } from '../../mocks/alertmanagerApi'; import { mockPluginSettings } from '../../mocks/plugins'; +import { setupDataSources } from '../../testSetup/datasources'; import { SupportedPlugin } from '../../types/pluginBridges'; import * as ruleId from '../../utils/rule-id'; @@ -47,20 +47,6 @@ const mockRoute = (id?: string): GrafanaRouteComponentProps<{ id?: string; sourc staticContext: {}, }); -// jest.mock('../../hooks/useCombinedRule'); -jest.mock('@grafana/runtime', () => ({ - ...jest.requireActual('@grafana/runtime'), - getDataSourceSrv: () => { - return { - getInstanceSettings: () => ({ name: 'prometheus' }), - get: () => - Promise.resolve({ - filterQuery: () => true, - }), - }; - }, -})); - jest.mock('../../hooks/useIsRuleEditable'); jest.mock('../../api/buildInfo'); @@ -105,15 +91,12 @@ beforeAll(() => { // we need to mock this one for the "declare incident" button mockPluginSettings(server, SupportedPlugin.Incident); - const dsSettings = mockDataSource({ + const promDsSettings = mockDataSource({ name: dsName, uid: dsName, }); - config.datasources = { - [dsName]: dsSettings, - }; - setDataSourceSrv(new MockDataSourceSrv({ [dsName]: dsSettings })); + setupDataSources(promDsSettings); mockAlertRuleApi(server).rulerRules('grafana', { [mockGrafanaRule.namespace.name]: [ diff --git a/public/app/features/alerting/unified/mockApi.ts b/public/app/features/alerting/unified/mockApi.ts index cf20e20c109..4536b5a627d 100644 --- a/public/app/features/alerting/unified/mockApi.ts +++ b/public/app/features/alerting/unified/mockApi.ts @@ -2,8 +2,14 @@ import { rest } from 'msw'; import { setupServer, SetupServer } from 'msw/node'; import 'whatwg-fetch'; +import { DataSourceInstanceSettings } from '@grafana/data'; import { setBackendSrv } from '@grafana/runtime'; -import { PromRulesResponse, RulerRuleGroupDTO, RulerRulesConfigDTO } from 'app/types/unified-alerting-dto'; +import { + PromBuildInfoResponse, + PromRulesResponse, + RulerRuleGroupDTO, + RulerRulesConfigDTO, +} from 'app/types/unified-alerting-dto'; import { backendSrv } from '../../../core/services/backend_srv'; import { @@ -15,6 +21,8 @@ import { Route, } from '../../../plugins/datasource/alertmanager/types'; +import { AlertingQueryResponse } from './state/AlertingQueryRunner'; + class AlertmanagerConfigBuilder { private alertmanagerConfig: AlertmanagerConfig = { receivers: [] }; @@ -122,6 +130,14 @@ export function mockApi(server: SetupServer) { ) ); }, + + eval: (response: AlertingQueryResponse) => { + server.use( + rest.post('/api/v1/eval', (_, res, ctx) => { + return res(ctx.status(200), ctx.json(response)); + }) + ); + }, }; } @@ -149,6 +165,24 @@ export function mockAlertRuleApi(server: SetupServer) { }; } +/** + * Used to mock the response from the /api/v1/status/buildinfo endpoint + */ +export function mockFeatureDiscoveryApi(server: SetupServer) { + return { + /** + * + * @param dsSettings Use `mockDataSource` to create a faks data source settings + * @param response Use `buildInfoResponse` to get a pre-defined response for Prometheus and Mimir + */ + discoverDsFeatures: (dsSettings: DataSourceInstanceSettings, response: PromBuildInfoResponse) => { + server.use( + rest.get(`${dsSettings.url}/api/v1/status/buildinfo`, (_, res, ctx) => res(ctx.status(200), ctx.json(response))) + ); + }, + }; +} + // Creates a MSW server and sets up beforeAll, afterAll and beforeEach handlers for it export function setupMswServer() { const server = setupServer(); diff --git a/public/app/features/alerting/unified/mocks.ts b/public/app/features/alerting/unified/mocks.ts index fbd1222a80e..1526dc02369 100644 --- a/public/app/features/alerting/unified/mocks.ts +++ b/public/app/features/alerting/unified/mocks.ts @@ -1,12 +1,17 @@ import { produce } from 'immer'; +import { Observable } from 'rxjs'; import { + DataQuery, + DataQueryRequest, + DataQueryResponse, DataSourceApi, DataSourceInstanceSettings, DataSourceJsonData, DataSourcePluginMeta, DataSourceRef, ScopedVars, + TestDataSourceResponse, } from '@grafana/data'; import { config, DataSourceSrv, GetDataSourceListFilters } from '@grafana/runtime'; import { contextSrv } from 'app/core/services/context_srv'; @@ -56,12 +61,15 @@ export function mockDataSource { const id = partial.id ?? nextDataSourceId++; + const uid = partial.uid ?? `mock-ds-${nextDataSourceId}`; + return { id, - uid: `mock-ds-${nextDataSourceId}`, + uid, type: 'prometheus', name: `Prometheus-${id}`, access: 'proxy', + url: `/api/datasources/proxy/uid/${uid}`, jsonData: {} as T, meta: { info: { @@ -320,6 +328,20 @@ export const mockReceiversState = (partial: Partial = {}): Recei }; }; +class MockDataSourceApi extends DataSourceApi { + constructor(instanceSettings: DataSourceInstanceSettings) { + super(instanceSettings); + } + + query(request: DataQueryRequest): Promise | Observable { + throw new Error('Method not implemented.'); + } + testDatasource(): Promise { + throw new Error('Method not implemented.'); + } +} + +// TODO This should be eventually moved to public/app/features/alerting/unified/testSetup/datasources.ts export class MockDataSourceSrv implements DataSourceSrv { datasources: Record = {}; // @ts-ignore @@ -350,6 +372,7 @@ export class MockDataSourceSrv implements DataSourceSrv { if (dsSettings.isDefault) { this.defaultName = dsSettings.name; } + this.datasources[dsSettings.uid] = new MockDataSourceApi(dsSettings); } } diff --git a/public/app/features/alerting/unified/testSetup/datasources.ts b/public/app/features/alerting/unified/testSetup/datasources.ts new file mode 100644 index 00000000000..d70e0d8050b --- /dev/null +++ b/public/app/features/alerting/unified/testSetup/datasources.ts @@ -0,0 +1,16 @@ +import { keyBy } from 'lodash'; + +import { DataSourceInstanceSettings } from '@grafana/data'; +import { config, setDataSourceSrv } from '@grafana/runtime'; + +import { MockDataSourceSrv } from '../mocks'; + +/** + * Sets up the data sources for the tests. + * Sets up both config object from grafana/runtime and the data source server + * @param configs data source instance settings. Use **mockDataSource** to create mock settings + */ +export function setupDataSources(...configs: DataSourceInstanceSettings[]) { + config.datasources = keyBy(configs, (c) => c.name); + setDataSourceSrv(new MockDataSourceSrv(config.datasources)); +} diff --git a/public/app/features/alerting/unified/testSetup/featureDiscovery.ts b/public/app/features/alerting/unified/testSetup/featureDiscovery.ts new file mode 100644 index 00000000000..2147caf316f --- /dev/null +++ b/public/app/features/alerting/unified/testSetup/featureDiscovery.ts @@ -0,0 +1,31 @@ +import { PromBuildInfoResponse } from 'app/types/unified-alerting-dto'; + +export const buildInfoResponse: { prometheus: PromBuildInfoResponse; mimir: PromBuildInfoResponse } = { + prometheus: { + status: 'success', + data: { + version: '2.45.0', + revision: '8ef767e396bf8445f009f945b0162fd71827f445', + branch: 'HEAD', + buildUser: 'root@920118f645b7', + buildDate: '20230623-15:15:37', + goVersion: 'go1.20.5', + }, + }, + mimir: { + status: 'success', + data: { + application: 'Grafana Mimir', + version: 'r249-5bedc7a1-WIP', + revision: '5bedc7a1', + branch: 'weekly-r249', + goVersion: 'go1.20.5', + features: { + ruler_config_api: 'true', + alertmanager_config_api: 'true', + query_sharding: 'false', + federated_rules: 'false', + }, + }, + }, +}; diff --git a/public/app/types/unified-alerting-dto.ts b/public/app/types/unified-alerting-dto.ts index b3af0806f23..1fd948e70b9 100644 --- a/public/app/types/unified-alerting-dto.ts +++ b/public/app/types/unified-alerting-dto.ts @@ -79,6 +79,7 @@ export interface PromBuildInfoResponse { query_sharding?: 'true' | 'false'; federated_rules?: 'true' | 'false'; }; + [key: string]: unknown; }; status: 'success'; }