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

Unit test for QueryUtils, application, plugin #96

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions common/utils/QueryUtils.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { retrieveQueryById } from './QueryUtils'; // Update with the correct path
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove this?

// Update with the correct path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Copy link
Member

@ansjcy ansjcy Feb 13, 2025

Choose a reason for hiding this comment

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

I don't see it removed, please don't resolve my comments - the reviewer should resolve the comments once they verifies

const mockHttpGet = jest.fn();
const mockCore = { http: { get: mockHttpGet } };

describe('retrieveQueryById', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Please improve this test description (retrieveQueryById ). even I'm confused on what this test is about by looking at the description.

const dataSourceId = 'test-ds';
Copy link
Member

Choose a reason for hiding this comment

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

Could you add comments here on what those values represents?
Also. I believe those will be used across different tests. Can you move them into a shared util?

const start = '2025-01-01T00:00:00Z';
const end = '2025-01-31T23:59:59Z';
const id = '1e5fde5b-c85f-419e-b8b6-3f43b3da4d59';

afterEach(() => {
jest.clearAllMocks();
});

it('should return the top query', async () => {
mockHttpGet.mockImplementation((endpoint) => {
if (endpoint.includes('latency')) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not precise, could you use the exact url endpoint?

return Promise.resolve({
response: {
top_queries: [
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just use the mockQueries in the utils? instead of creating it here.

{
id: '1e5fde5b-c85f-419e-b8b6-3f43b3da4d59',
query: JSON.stringify({ match: { user_action: 'login_attempt' } }),
},
],
},
Copy link
Member

Choose a reason for hiding this comment

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

We also need to test how many times / type of requests (GET? POST?) the endpoint is called

});
}
return Promise.resolve({ response: { top_queries: [] } });
Copy link
Member

Choose a reason for hiding this comment

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

Could you assert that the mockHttpGet is called with the exact expected endpoint url?

});

const result = await retrieveQueryById(mockCore, dataSourceId, start, end, id);
expect(result).toEqual({
id: '1e5fde5b-c85f-419e-b8b6-3f43b3da4d59',
query: JSON.stringify({ match: { user_action: 'login_attempt' } }),
});
});

it('should return null if no top queries are found', async () => {
mockHttpGet.mockResolvedValue({ response: { top_queries: [] } });
Copy link
Member

Choose a reason for hiding this comment

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

Same as above: Could you assert that the mockHttpGet is called with the exact expected endpoint url?


const result = await retrieveQueryById(mockCore, dataSourceId, start, end, id);
expect(result).toBeNull();
});

it('should handle API errors gracefully and return null', async () => {
mockHttpGet.mockRejectedValue(new Error('API error'));
Copy link
Member

Choose a reason for hiding this comment

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

Please check if our code logs errors, if so, we need to verify logging occurred.


const result = await retrieveQueryById(mockCore, dataSourceId, start, end, id);
expect(result).toBeNull();
});
Copy link
Member

Choose a reason for hiding this comment

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

Could you add additional test cases for edge cases? Like, when the response structure is different, to ensure robustness?

});
54 changes: 54 additions & 0 deletions public/application.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import React from 'react';
import { HashRouter as Router } from 'react-router-dom';
import * as ReactDOM from 'react-dom';
import { renderApp } from './application';
import { QueryInsightsDashboardsApp } from './components/app';

jest.mock('react-dom', () => {
const actualReactDOM = jest.requireActual('react-dom');
return {
...actualReactDOM,
render: jest.fn(),
unmountComponentAtNode: jest.fn(),
};
});
describe('renderApp', () => {
const coreMock = {};
Copy link
Member

Choose a reason for hiding this comment

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

OpenSearch-Dashboards has a built in core mock, could you do some research and reuse that one instead of creating this empty object?

const depsStartMock = {};
const paramsMock = { element: document.createElement('div') };
const dataSourceManagementMock = {};

afterEach(() => {
jest.clearAllMocks();
});

it('should render the QueryInsightsDashboardsApp component inside Router', () => {
const unmount = renderApp(coreMock, depsStartMock, paramsMock, dataSourceManagementMock);

expect(ReactDOM.render).toHaveBeenCalledWith(
<Router>
<QueryInsightsDashboardsApp
core={coreMock}
depsStart={depsStartMock}
params={paramsMock}
dataSourceManagement={dataSourceManagementMock}
/>
</Router>,
paramsMock.element
);

expect(typeof unmount).toBe('function');
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any value of thisexpect here, especially when we have the tests on line 48.

});

it('should unmount the component when the returned function is called', () => {
const unmount = renderApp(coreMock, depsStartMock, paramsMock, dataSourceManagementMock);
unmount();

expect(ReactDOM.unmountComponentAtNode).toHaveBeenCalledWith(paramsMock.element);
});
});
76 changes: 76 additions & 0 deletions public/plugin.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { CoreSetup, CoreStart } from '../../../src/core/public';
import { QueryInsightsDashboardsPlugin } from './plugin';
import { PLUGIN_NAME } from '../common';
import { renderApp } from './application';

jest.mock('./application', () => ({
renderApp: jest.fn(),
}));

describe('QueryInsightsDashboardsPlugin', () => {
let plugin: QueryInsightsDashboardsPlugin;
let coreSetupMock: jest.Mocked<CoreSetup>;
let coreStartMock: jest.Mocked<CoreStart>;
let registerMock: jest.Mock;
let addNavLinksMock: jest.Mock;

beforeEach(() => {
coreSetupMock = ({
application: {
register: jest.fn(),
},
chrome: {
navGroup: {
addNavLinksToGroup: jest.fn(),
},
},
getStartServices: jest.fn().mockResolvedValue([coreStartMock, {}]),
} as unknown) as jest.Mocked<CoreSetup>;
Copy link
Member

Choose a reason for hiding this comment

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

Let's not cast to unknown, maybe reuse the coreMock in OSD


coreStartMock = {} as jest.Mocked<CoreStart>;
Copy link
Member

Choose a reason for hiding this comment

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

Same - reuse the coreMock in OSD


plugin = new QueryInsightsDashboardsPlugin();
registerMock = coreSetupMock.application.register;
addNavLinksMock = coreSetupMock.chrome.navGroup.addNavLinksToGroup;
});

it('should register the application in setup', () => {
plugin.setup(coreSetupMock, {} as any);

expect(registerMock).toHaveBeenCalledWith(
expect.objectContaining({
id: PLUGIN_NAME,
title: 'Query Insights',
Copy link
Member

Choose a reason for hiding this comment

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

There a several other fields being registered, could you include them all?

})
);
});

it('should mount the application correctly', async () => {
plugin.setup(coreSetupMock, {} as any);

const appRegistration = registerMock.mock.calls[0][0];
expect(appRegistration).toBeDefined();

const paramsMock = { element: document.createElement('div') };
const mountFunction = appRegistration.mount;

await mountFunction(paramsMock);

expect(renderApp).toHaveBeenCalled();
Copy link
Member

Choose a reason for hiding this comment

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

There's no validation check that renderApp receives valid arguments.

});

it('should add the navigation link to nav group', () => {
plugin.setup(coreSetupMock, {} as any);
expect(addNavLinksMock).toHaveBeenCalled();
Copy link
Member

Choose a reason for hiding this comment

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

Could you add property checks here as well?

});

it('should return empty start and stop methods', () => {
Copy link
Member

Choose a reason for hiding this comment

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

why is this test needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verifies no unnecessary logic in start and stop which prevents future bugs

expect(plugin.start(coreStartMock)).toEqual({});
expect(plugin.stop()).toBeUndefined();
});
});
Loading