Unit test for QueryUtils, application, plugin #96
Conversation
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
KishoreKicha14
requested review from
ansjcy,
jainankitk,
deshsidd and
dzane17
as code owners
common/utils/QueryUtils.test.ts
Outdated
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { retrieveQueryById } from './QueryUtils'; // Update with the correct path |
There was a problem hiding this comment.
Could you remove this?
// Update with the correct path
There was a problem hiding this comment.
I don't see it removed, please don't resolve my comments - the reviewer should resolve the comments once they verifies
| expect(addNavLinksMock).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should return empty start and stop methods', () => { |
There was a problem hiding this comment.
Verifies no unnecessary logic in start and stop which prevents future bugs
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
KishoreKicha14
force-pushed
the
unit-test-Applications-tsx
branch
from
d6b4c09 to
df9e021
Compare
common/utils/QueryUtils.test.ts
Outdated
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { retrieveQueryById } from './QueryUtils'; // Update with the correct path |
There was a problem hiding this comment.
I don't see it removed, please don't resolve my comments - the reviewer should resolve the comments once they verifies
common/utils/QueryUtils.test.ts
Outdated
| if (endpoint.includes('latency')) { | ||
| return Promise.resolve({ | ||
| response: { | ||
| top_queries: [ |
There was a problem hiding this comment.
Can't we just use the mockQueries in the utils? instead of creating it here.
public/application.test.tsx
Outdated
| paramsMock.element | ||
| ); | ||
|
|
||
| expect(typeof unmount).toBe('function'); |
There was a problem hiding this comment.
I don't see any value of thisexpect here, especially when we have the tests on line 48.
common/utils/QueryUtils.test.ts
Outdated
| const mockCore = { http: { get: mockHttpGet } }; | ||
|
|
||
| describe('retrieveQueryById', () => { | ||
| const dataSourceId = 'test-ds'; |
There was a problem hiding this comment.
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?
common/utils/QueryUtils.test.ts
Outdated
| const mockHttpGet = jest.fn(); | ||
| const mockCore = { http: { get: mockHttpGet } }; | ||
|
|
||
| describe('retrieveQueryById', () => { |
There was a problem hiding this comment.
Please improve this test description (retrieveQueryById ). even I'm confused on what this test is about by looking at the description.
common/utils/QueryUtils.test.ts
Outdated
| }, | ||
| }); | ||
| } | ||
| return Promise.resolve({ response: { top_queries: [] } }); |
There was a problem hiding this comment.
Could you assert that the mockHttpGet is called with the exact expected endpoint url?
common/utils/QueryUtils.test.ts
Outdated
| query: JSON.stringify({ match: { user_action: 'login_attempt' } }), | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
We also need to test how many times / type of requests (GET? POST?) the endpoint is called
common/utils/QueryUtils.test.ts
Outdated
| }); | ||
|
|
||
| it('should return null if no top queries are found', async () => { | ||
| mockHttpGet.mockResolvedValue({ response: { top_queries: [] } }); |
There was a problem hiding this comment.
Same as above: Could you assert that the mockHttpGet is called with the exact expected endpoint url?
public/plugin.test.tsx
Outdated
| }, | ||
| }, | ||
| getStartServices: jest.fn().mockResolvedValue([coreStartMock, {}]), | ||
| } as unknown) as jest.Mocked<CoreSetup>; |
There was a problem hiding this comment.
Let's not cast to unknown, maybe reuse the coreMock in OSD
public/plugin.test.tsx
Outdated
| getStartServices: jest.fn().mockResolvedValue([coreStartMock, {}]), | ||
| } as unknown) as jest.Mocked<CoreSetup>; | ||
|
|
||
| coreStartMock = {} as jest.Mocked<CoreStart>; |
There was a problem hiding this comment.
Same - reuse the coreMock in OSD
| expect(registerMock).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| id: PLUGIN_NAME, | ||
| title: 'Query Insights', |
There was a problem hiding this comment.
There a several other fields being registered, could you include them all?
public/plugin.test.tsx
Outdated
|
|
||
| await mountFunction(paramsMock); | ||
|
|
||
| expect(renderApp).toHaveBeenCalled(); |
There was a problem hiding this comment.
There's no validation check that renderApp receives valid arguments.
public/plugin.test.tsx
Outdated
|
|
||
| it('should add the navigation link to nav group', () => { | ||
| plugin.setup(coreSetupMock, {} as any); | ||
| expect(addNavLinksMock).toHaveBeenCalled(); |
There was a problem hiding this comment.
Could you add property checks here as well?
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Description
QueryUtils.tsapplication.tsxplugin.tsQueryUtils.tscoverage increased from0%to85.71%.100%coverage forapplication.tsxandplugin.ts.82.57%to85.71%.Issues Resolved
List any issues this PR will resolve, e.g. Closes [...].
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.