Skip to content

Commit

Permalink
Adds unit tests
Browse files Browse the repository at this point in the history
Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>
  • Loading branch information
ashwin-pc committed Jan 6, 2023
1 parent 654f937 commit 4b55e30
Show file tree
Hide file tree
Showing 15 changed files with 227 additions and 59 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,17 @@ export const Workspace: FC = ({ children }) => {
useEffect(() => {
async function loadExpression() {
const schemas = ui.containerConfig.data.schemas;
const [validSchema, schemaError] = validateSchemaState(schemas, rootState.visualization);
const [validAggs, aggregationError] = await validateAggregations(aggConfigs?.aggs || []);

if (!validSchema || !validAggs) {
const err = schemaError || aggregationError;
if (typeof err === 'string' && aggConfigs?.aggs.length) {
toasts.addWarning(err);
}
const noAggs = aggConfigs?.aggs?.length === 0;
const schemaValidation = validateSchemaState(schemas, rootState.visualization);
const aggValidation = validateAggregations(aggConfigs?.aggs || []);

if (noAggs || !aggValidation.valid || !schemaValidation.valid) {
const err = schemaValidation.errorMsg || aggValidation.errorMsg;

if (err) toasts.addWarning(err);
setExpression(undefined);

return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,4 @@ export const handlerEditorState = (store: Store, state: RootState, previousState
) {
store.dispatch(setEditorState({ state: 'dirty' }));
}

return state;
};
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export const handlerParentAggs = async (
store: Store,
state: RootState,
services: VisBuilderServices
): Promise<RootState> => {
) => {
const {
visualization: { activeVisualization, indexPattern = '' },
} = state;
Expand Down Expand Up @@ -64,6 +64,4 @@ export const handlerParentAggs = async (
);
}
});

return state;
};
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@

import { VisBuilderServices } from '../../../types';
import { createVisBuilderServicesMock } from '../mocks';
import { getPreloadedState } from './preload';
import { loadReduxState, saveReduxState } from './redux_persistence';
import { loadReduxState, persistReduxState } from './redux_persistence';

describe('test redux state persistence', () => {
let mockServices: jest.Mocked<VisBuilderServices>;
Expand Down Expand Up @@ -45,8 +44,8 @@ describe('test redux state persistence', () => {
expect(returnStates).toStrictEqual(reduxStateParams);
});

test('test save redux state', () => {
saveReduxState(reduxStateParams, mockServices);
test('test persist redux state', () => {
persistReduxState(reduxStateParams, mockServices);
const urlStates = mockServices.osdUrlStateStorage.get('_a');
expect(urlStates).toStrictEqual(reduxStateParams);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export const loadReduxState = async (services: VisBuilderServices) => {
return await getPreloadedState(services);
};

export const saveReduxState = (
export const persistReduxState = (
{ style, visualization, metadata },
services: VisBuilderServices
) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { reducer as styleReducer } from './style_slice';
import { reducer as visualizationReducer } from './visualization_slice';
import { reducer as metadataReducer } from './metadata_slice';
import { VisBuilderServices } from '../../..';
import { loadReduxState, saveReduxState } from './redux_persistence';
import { loadReduxState, persistReduxState } from './redux_persistence';
import { handlerEditorState } from './handlers/editor_state';
import { handlerParentAggs } from './handlers/parent_aggs';

Expand All @@ -35,20 +35,15 @@ export const getPreloadedStore = async (services: VisBuilderServices) => {
// Listen to changes
const handleChange = () => {
const state = store.getState();
persistReduxState(state, services);

if (isEqual(state, previousState)) return;

previousState = handlerEditorState(store, state, previousState);
// Side effects to apply after changes to the store are made
handlerEditorState(store, state, previousState);
handlerParentAggs(store, state, services);

saveReduxState(
{
style: store.getState().style,
visualization: store.getState().visualization,
metadata: store.getState().metadata,
},
services
);
previousState = state;
};

// the store subscriber will automatically detect changes and call handleChange function
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export const useSavedVisBuilderVis = (visualizationIdFromUrl: string | undefined
text: message,
});
};

const loadSavedVisBuilderVis = async () => {
try {
const savedVisBuilderVis = await getSavedVisBuilderVis(services, visualizationIdFromUrl);
Expand All @@ -73,11 +74,13 @@ export const useSavedVisBuilderVis = (visualizationIdFromUrl: string | undefined

const validateResult = validateVisBuilderState({ styleState, visualizationState });
if (!validateResult.valid) {
const err = validateResult.errors;
if (err) {
const errMsg = err[0].instancePath + ' ' + err[0].message;
throw new InvalidJSONProperty(errMsg);
}
throw new InvalidJSONProperty(
validateResult.errorMsg ||
i18n.translate('visBuilder.useSavedVisBuilderVis.genericJSONError', {
defaultMessage:
'Something went wrong while loading your saved object. The object may be corrupted or does not match the latest schema',
})
);
}

dispatch(setStyleState(styleState));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

export interface ValidationResult<T = boolean> {
errorMsg?: string;
valid: T;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { BUCKET_TYPES, IndexPattern, METRIC_TYPES } from '../../../../../data/public';
import { dataPluginMock } from '../../../../../data/public/mocks';
import { validateAggregations } from './validate_aggregations';

describe('validateAggregations', () => {
const fields = [
{
name: '@timestamp',
},
{
name: 'bytes',
},
];

const indexPattern = {
id: '1234',
title: 'logstash-*',
fields: {
getByName: (name: string) => fields.find((f) => f.name === name),
filter: () => fields,
},
} as any;

const dataStart = dataPluginMock.createStartContract();

test('Pipeline aggs should have a bucket agg as the last agg', () => {
const aggConfigs = dataStart.search.aggs.createAggConfigs(indexPattern as IndexPattern, [
{
id: '1',
enabled: true,
type: METRIC_TYPES.CUMULATIVE_SUM,
schema: 'metric',
params: {},
},
]);

const { valid, errorMsg } = validateAggregations(aggConfigs.aggs);

expect(valid).toBe(false);
expect(errorMsg).toMatchInlineSnapshot(
`"Add a bucket with \\"Date Histogram\\" or \\"Histogram\\" aggregation."`
);
});

test('Pipeline aggs should have a valid bucket agg', () => {
const aggConfigs = dataStart.search.aggs.createAggConfigs(indexPattern as IndexPattern, [
{
id: '0',
enabled: true,
type: BUCKET_TYPES.SIGNIFICANT_TERMS,
schema: 'segment',
params: {},
},
{
id: '1',
enabled: true,
type: METRIC_TYPES.CUMULATIVE_SUM,
schema: 'metric',
params: {},
},
]);

const { valid, errorMsg } = validateAggregations(aggConfigs.aggs);

expect(valid).toBe(false);
expect(errorMsg).toMatchInlineSnapshot(
`"Last bucket aggregation must be \\"Date Histogram\\" or \\"Histogram\\" when using \\"Cumulative Sum\\" metric aggregation."`
);
});

test('Valid pipeline aggconfigs', () => {
const aggConfigs = dataStart.search.aggs.createAggConfigs(indexPattern as IndexPattern, [
{
id: '0',
enabled: true,
type: BUCKET_TYPES.DATE_HISTOGRAM,
schema: 'segment',
params: {},
},
{
id: '1',
enabled: true,
type: METRIC_TYPES.CUMULATIVE_SUM,
schema: 'metric',
params: {},
},
]);

const { valid, errorMsg } = validateAggregations(aggConfigs.aggs);

expect(valid).toBe(true);
expect(errorMsg).not.toBeDefined();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,14 @@ import { i18n } from '@osd/i18n';
import { findLast } from 'lodash';
import { AggConfig, BUCKET_TYPES, IMetricAggType } from '../../../../../data/common';
import { search } from '../../../../../data/public';
import { ValidationResult } from './types';

export const validateAggregations = async (aggs: AggConfig[]): Promise<[boolean, string?]> => {
/**
* Validate if the aggregations to perform are possible
* @param aggs Aggregations to be performed
* @returns ValidationResult
*/
export const validateAggregations = (aggs: AggConfig[]): ValidationResult => {
// Pipeline aggs should have a valid bucket agg
const metricAggs = aggs.filter((agg) => agg.schema === 'metric');
const lastParentPipelineAgg = findLast(
Expand All @@ -18,13 +24,13 @@ export const validateAggregations = async (aggs: AggConfig[]): Promise<[boolean,
const lastBucket = findLast(aggs, (agg) => agg.type.type === 'buckets');

if (!lastBucket && lastParentPipelineAgg) {
return [
false,
i18n.translate('visBuilder.aggregation.mustHaveBucketErrorMessage', {
return {
valid: false,
errorMsg: i18n.translate('visBuilder.aggregation.mustHaveBucketErrorMessage', {
defaultMessage: 'Add a bucket with "Date Histogram" or "Histogram" aggregation.',
description: 'Date Histogram and Histogram should not be translated',
}),
];
};
}

// Last bucket in a Pipeline aggs should be either a date histogram or histogram
Expand All @@ -33,16 +39,16 @@ export const validateAggregations = async (aggs: AggConfig[]): Promise<[boolean,
lastParentPipelineAgg &&
!([BUCKET_TYPES.DATE_HISTOGRAM, BUCKET_TYPES.HISTOGRAM] as any).includes(lastBucket.type.name)
) {
return [
false,
i18n.translate('visBuilder.aggregation.wrongLastBucketTypeErrorMessage', {
return {
valid: false,
errorMsg: i18n.translate('visBuilder.aggregation.wrongLastBucketTypeErrorMessage', {
defaultMessage:
'Last bucket aggregation must be "Date Histogram" or "Histogram" when using "{type}" metric aggregation.',
values: { type: (lastParentPipelineAgg as AggConfig).type.title },
description: 'Date Histogram and Histogram should not be translated',
}),
];
};
}

return [true];
return { valid: true };
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { Schemas } from '../../../../../vis_default_editor/public';
import { VisualizationState } from '../state_management';
import { validateSchemaState } from './validate_schema_state';

describe('validateSchemaState', () => {
const schemas = new Schemas([
{
name: 'metrics',
group: 'metrics',
min: 1,
},
{
name: 'buckets',
group: 'buckets',
},
]);

test('should error if schema min agg requirement not met', () => {
const visState: VisualizationState = {
searchField: '',
activeVisualization: {
name: 'Test vis',
aggConfigParams: [],
},
};

const { valid, errorMsg } = validateSchemaState(schemas, visState);

expect(valid).toBe(false);
expect(errorMsg).toMatchInlineSnapshot(
`"The Test vis visualization needs at least 1 field(s) in the agg type \\"metrics\\""`
);
});

test('should be valid if schema requirements are met', () => {
const visState: VisualizationState = {
searchField: '',
activeVisualization: {
name: 'Test vis',
aggConfigParams: [
{
type: 'count',
schema: 'metrics',
},
],
},
};

const { valid, errorMsg } = validateSchemaState(schemas, visState);

expect(valid).toBe(true);
expect(errorMsg).not.toBeDefined();
});
});
Loading

0 comments on commit 4b55e30

Please sign in to comment.