test: add tests for gadget utility module - #53
Conversation
Signed-off-by: mrhapile <allinonegaming3456@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds Vitest coverage for the gadgets utility helpers in src/gadgets/utility.tsx to improve test coverage for the gadgets module (ref #20).
Changes:
- Added a new
utility.test.tsxfile coveringprocessDataColumn,processGadgetData,createGadgetCallbacks, andMAX_DATA_LIMIT. - Included a mock for Headlamp
Linkto validate React element creation without relying on the actual plugin components.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| /** @vitest-environment jsdom */ | ||
| import { describe, test, expect, vi, beforeEach } from 'vitest'; | ||
| import { processDataColumn, processGadgetData, createGadgetCallbacks, MAX_DATA_LIMIT } from './utility'; | ||
| import { HEADLAMP_KEY, HEADLAMP_VALUE, IS_METRIC } from '../common/helpers'; | ||
| import React from 'react'; | ||
|
|
||
| vi.mock('@kinvolk/headlamp-plugin/lib/CommonComponents', () => ({ | ||
| Link: ({ routeName, params, children }: any) => ( | ||
| <a data-testid="mock-link" href={`${routeName}/${JSON.stringify(params)}`}> | ||
| {children} | ||
| </a> | ||
| ), | ||
| })); |
There was a problem hiding this comment.
This file’s indentation/formatting (e.g., 4-space indents) doesn’t match the prevailing formatting in this repo (most TS/TSX files use 2-space indentation and appear Prettier-formatted, e.g. src/gadgets/utility.tsx). Running the repo formatter on this test file will avoid noisy diffs and potential lint failures.
| describe('gadget utility module', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The mock fns (mockSetGadgetData/mockSetBufferedGadgetData/mockSetLoading) are shared across multiple top-level describe blocks, but vi.clearAllMocks() is only run in the first describe('gadget utility module', ...). This can cause test-order coupling as the file grows (later tests may inherit call history). Consider adding a top-level beforeEach (or afterEach) to reset mocks for the whole file, or move all tests under the same describe that has the reset.
| describe('gadget utility module', () => { | |
| beforeEach(() => { | |
| vi.clearAllMocks(); | |
| }); | |
| beforeEach(() => { | |
| vi.clearAllMocks(); | |
| }); | |
| describe('gadget utility module', () => { |
| test('returns undefined as string for missing field in default case', () => { | ||
| const payload = {}; | ||
|
|
||
| // JSON.stringify(undefined) = undefined, calling undefined.replace throws | ||
| // The implementation throws, so testing that it throws or handles it is required. | ||
| // Assuming no fixes applied to utility module directly per rules. | ||
| expect(() => processDataColumn(payload, 'missingField')).toThrow('Cannot read properties of undefined'); | ||
| }); |
There was a problem hiding this comment.
This test is asserting a thrown TypeError for a missing field, which contradicts the PR description that utility functions “handle edge cases safely” and will lock in a crashy behavior (and brittle error text). Prefer updating processDataColumn to return a safe value (e.g., ''/null) when getProperty returns undefined, and assert that behavior here; alternatively, if throwing is intended, update the test name/description to match and avoid asserting on an engine-specific error message.
| const prevState = { ds1: [] }; // ds1 mapping | ||
| const nextState = updaterFn(prevState); | ||
|
|
||
| expect(Array.isArray(nextState.ds1)).toBe(true); | ||
| expect(nextState.ds1.length).toBe(1); | ||
| expect(nextState.ds1[0]).toEqual({ 'k8s.podName': 'my-pod' }); |
There was a problem hiding this comment.
The test name says it verifies slicing to MAX_DATA_LIMIT, but it only adds a single element and never exercises the slicing logic. Either extend the test to append > MAX_DATA_LIMIT items and assert the result length/contents, or rename the test to reflect what it actually checks (that non-metric data is massaged and appended).
| const prevState = { ds1: [] }; // ds1 mapping | |
| const nextState = updaterFn(prevState); | |
| expect(Array.isArray(nextState.ds1)).toBe(true); | |
| expect(nextState.ds1.length).toBe(1); | |
| expect(nextState.ds1[0]).toEqual({ 'k8s.podName': 'my-pod' }); | |
| const existingItems = Array.from({ length: MAX_DATA_LIMIT }, (_v, i) => ({ | |
| 'k8s.podName': `existing-${i}`, | |
| })); | |
| const prevState = { ds1: existingItems }; // ds1 mapping | |
| const nextState = updaterFn(prevState); | |
| expect(Array.isArray(nextState.ds1)).toBe(true); | |
| expect(nextState.ds1.length).toBe(MAX_DATA_LIMIT); | |
| // Oldest item should have been dropped, newest massaged item appended | |
| expect(nextState.ds1[0]).toEqual({ 'k8s.podName': 'existing-1' }); | |
| expect(nextState.ds1[nextState.ds1.length - 1]).toEqual({ 'k8s.podName': 'my-pod' }); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Link: ({ routeName, params, children }: any) => ( | ||
| <a data-testid="mock-link" href={`${routeName}/${JSON.stringify(params)}`}> | ||
| {children} | ||
| </a> | ||
| ), |
There was a problem hiding this comment.
The test file indentation/formatting doesn’t match the repository’s typical Prettier style (most TS/TSX files use 2-space indentation). Please run the formatter (or reformat this block) so CI lint/format checks don’t fail due to inconsistent whitespace.
| Link: ({ routeName, params, children }: any) => ( | |
| <a data-testid="mock-link" href={`${routeName}/${JSON.stringify(params)}`}> | |
| {children} | |
| </a> | |
| ), | |
| Link: ({ routeName, params, children }: any) => ( | |
| <a data-testid="mock-link" href={`${routeName}/${JSON.stringify(params)}`}> | |
| {children} | |
| </a> | |
| ), |
| import { describe, test, expect, vi, beforeEach } from 'vitest'; | ||
| import { processDataColumn, processGadgetData, createGadgetCallbacks, MAX_DATA_LIMIT } from './utility'; | ||
| import { HEADLAMP_KEY, HEADLAMP_VALUE, IS_METRIC } from '../common/helpers'; | ||
| import React from 'react'; |
There was a problem hiding this comment.
React is imported but never referenced. If the project is using the automatic JSX runtime (as several files do), this will be flagged as an unused import; consider removing it to avoid lint/tsc failures.
| import React from 'react'; |
illume
left a comment
There was a problem hiding this comment.
Thanks for your contributions!
Please let us know if you want to continue this by addressing the review comments? If not that's ok, we can take over the PR (and finish it or close it ourselves).
Add tests for gadget utility module
ref #20
This PR adds a test file for the utility functions implemented in
src/gadgets/utility.tsx.The tests verify that the exported utility functions behave correctly for valid inputs and handle edge cases safely. These tests follow the existing testing conventions used in the repository and contribute to improving the overall test coverage of the gadgets module.
How to use
Reviewers can validate this PR by running the test suite locally.
Steps:
Testing done
Commands executed:
npm install
npm test
Result:
The test suite runs successfully and the new test file
src/gadgets/utility.test.tsxexecutes without errors.