Update vitest to v4 and related dependencies in thunder frontend#1300
Update vitest to v4 and related dependencies in thunder frontend#1300DonOmalVindula wants to merge 1 commit intoasgardeo:mainfrom
Conversation
DonOmalVindula
force-pushed
the
migrate-vitest-v4
branch
from
e4ebb23 to
6ee2f29
Compare
DonOmalVindula
added
the
skip-changelog
There was a problem hiding this comment.
Pull request overview
This pull request upgrades Vitest from v3 to v4 across the Thunder frontend monorepo, with a focus on improving type safety in test files. The upgrade includes updating the browser testing provider configuration and modernizing test mock implementations.
Changes:
- Upgraded Vitest and related packages from v3.2.4 to v4.0.17, including adding the new
@vitest/browser-playwrightdependency for browser-based testing - Enhanced type safety in test files by adding explicit generic types to
vi.fn()mock declarations and replacing TypeScript eslint-disable comments with explicit type casts - Improved test reliability by adding setup steps to reset hoisted mock implementations before each test in rich text plugin tests
- Updated Vitest browser provider configuration from string-based
'playwright'to function-basedplaywright()API in vitest.config.ts files
Reviewed changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/pnpm-workspace.yaml | Updated catalog versions for vitest (4.0.17), @vitest/coverage-istanbul (4.0.17), and added @vitest/browser-playwright |
| frontend/pnpm-lock.yaml | Dependency lock file updates reflecting the Vitest v4 upgrade and new dependencies |
| frontend/packages/thunder-test-utils/src/mocks/i18n.ts | Added explicit return types to mock functions for better type safety |
| frontend/packages/thunder-shared-hooks/vitest.config.ts | Updated browser provider from string 'playwright' to function call playwright() |
| frontend/packages/thunder-shared-hooks/package.json | Added @vitest/browser-playwright dependency |
| frontend/packages/thunder-shared-contexts/vitest.config.ts | Updated browser provider configuration to use function call |
| frontend/packages/thunder-shared-contexts/package.json | Added @vitest/browser-playwright dependency |
| frontend/packages/thunder-shared-branding/vitest.config.ts | Updated browser provider configuration to use function call |
| frontend/packages/thunder-shared-branding/package.json | Added @vitest/browser-playwright dependency |
| frontend/apps/thunder-gate/src/tests/main.test.tsx | Changed QueryClient mock from returning empty object to empty function (potential issue) |
| frontend/apps/thunder-develop/src/hooks/useDataGridLocaleText.ts | Replaced process.env.NODE_ENV with Vite-standard import.meta.env.DEV |
| frontend/apps/thunder-develop/src/features/login-flow/hooks/tests/*.test.ts | Added explicit types to mock function declarations for improved type safety |
| frontend/apps/thunder-develop/src/features/flows/components/resources/steps/execution/tests/Execution.test.tsx | Simplified mockUseNodeId typing using generic type parameter syntax |
| frontend/apps/thunder-develop/src/features/flows/components/resource-property-panel/rich-text/helper-plugins/tests/*.test.tsx | Added beforeEach setup to reset hoisted mocks to default implementations |
| frontend/apps/thunder-develop/src/features/flows/api/tests/*.test.tsx | Added explicit generic types to mockHttpRequest declarations |
| frontend/apps/thunder-develop/src/features/integrations/api/tests/useIdentityProviders.test.tsx | Added explicit generic type to mockHttpRequest |
| frontend/apps/thunder-develop/src/features/applications/components/edit-application/token-settings/EditTokenSettings.tsx | Changed ref type from NodeJS.Timeout to ReturnType<typeof setTimeout> for better portability |
| frontend/apps/thunder-develop/src/features/applications/components/tests/ApplicationsList.test.tsx | Added type cast for navigate mock to match expected return type |
| frontend/apps/thunder-develop/src/features/applications/api/tests/*.test.tsx | Added explicit generic types to mock functions and replaced eslint-disable comments with explicit type casts |
Files not reviewed (1)
- frontend/pnpm-lock.yaml: Language not supported
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1300 +/- ##
==========================================
- Coverage 89.67% 87.74% -1.94%
==========================================
Files 649 655 +6
Lines 42534 43432 +898
Branches 2454 4288 +1834
==========================================
- Hits 38144 38111 -33
+ Misses 2375 2373 -2
- Partials 2015 2948 +933
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DonOmalVindula
force-pushed
the
migrate-vitest-v4
branch
from
6ee2f29 to
97a8a09
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR comprehensively refines TypeScript types in test files throughout the frontend codebase, transitions Vitest browser configuration from string-based to function-based provider instantiation, updates environment detection from Node.js to Vite semantics, and significantly expands test coverage for components, hooks, and pages across multiple features with hundreds of new test cases. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes The diff spans ~20+ files with heterogeneous changes across typing refinements, configuration updates, and extensive test additions. While many individual changes follow repetitive patterns (especially type refinements and similar test structures), the breadth across features and components, combined with new test logic requiring individual reasoning, warrants moderate-to-high review effort. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@frontend/apps/thunder-develop/src/features/login-flow/hooks/__tests__/useTemplateAndWidgetLoading.test.ts`:
- Line 174: The mock implementation for mockValidateEdges does not match its
declared signature; update the mock for validateEdges so it accepts both
parameters (edges: Edge[], nodes: Node[]) and returns the expected value (e.g.,
return edges) — replace the existing vi.fn((edges) => edges) with vi.fn((edges,
nodes) => edges) (or a minimal implementation that uses both args) to match the
declared type and prevent hidden bugs when validateEdges is called with two
arguments.
🧹 Nitpick comments (10)
frontend/apps/thunder-develop/src/features/flows/components/resource-property-panel/rich-text/helper-plugins/__tests__/CustomLinkPluginPredefinedUrls.test.tsx (1)
121-137: LGTM! Consistent with the other test file.The mock reset pattern matches
CustomLinkPlugin.test.tsx, ensuring both test files have identical isolation guarantees.Consider extracting the shared mock setup (vi.hoisted definitions + beforeEach resets) to a common test utility file to reduce duplication across both CustomLinkPlugin test files. This would make future maintenance easier if mock signatures change.
frontend/apps/thunder-gate/src/__tests__/components/SignUp/SignUpBox.test.tsx (3)
1656-1660: Consider adding assertion for SELECT component.The test includes a SELECT component with label 'Country' in the components array but only asserts on Phone, OTP, and Resend. Adding an assertion for the SELECT would improve test coverage.
💡 Suggested improvement
render(<SignUpBox />); expect(screen.getByLabelText(/Phone/)).toBeInTheDocument(); expect(screen.getByText('OTP')).toBeInTheDocument(); + expect(screen.getByText('Country')).toBeInTheDocument(); expect(screen.getByText('Resend')).toBeInTheDocument(); });
1708-1722: Test assertion could be more specific.This test is named "renders branded logo with alt fallback" but only asserts that the signup container exists. Consider adding an assertion that actually verifies the logo is rendered or that the fallback alt text is applied.
1756-1774: Test assertion could verify the button renders.This test verifies handling of a social login trigger with missing label and image, but only asserts that the container exists. Consider adding an assertion to verify how the component handles the missing properties (e.g., button still renders, uses fallback text, or doesn't render at all).
frontend/apps/thunder-gate/src/__tests__/components/AcceptInvite/AcceptInviteBox.test.tsx (2)
1274-1368: Test names imply key assertions, but only rendering is checked.These three tests don’t verify key behavior; they only verify that components render. Consider renaming to match intent or add an assertion (e.g., no React key warning).
💡 Suggested rename to match current assertions
-it('uses fallback index keys when components have undefined id', () => { +it('renders components when ids are undefined', () => { ... -it('uses fallback keys for EMAIL_INPUT with undefined id in form block', () => { +it('renders EMAIL_INPUT when id is undefined', () => { ... -it('uses fallback keys for SELECT with undefined id in form block', () => { +it('renders SELECT when id is undefined', () => {
1370-1429: Branding test assertions don’t match their titles.These tests mention centered alignment and logo alt/size, but only assert container presence. Either assert the expected styles/attributes or rename.
✅ Example assertions aligned with titles
it('renders with branding enabled and centered text alignment', () => { @@ - render(<AcceptInviteBox />); - expect(screen.getByText('Accept Invitation')).toBeInTheDocument(); + render(<AcceptInviteBox />); + const heading = screen.getByText('Accept Invitation'); + expect(heading).toBeInTheDocument(); + expect(heading).toHaveStyle({ textAlign: 'center' }); }); @@ it('renders branded logo with alt fallback', () => { @@ - render(<AcceptInviteBox />); - expect(screen.getByTestId('asgardeo-accept-invite')).toBeInTheDocument(); + render(<AcceptInviteBox />); + const logo = screen.getByRole('img'); + expect(logo).toHaveAttribute('src', 'https://example.com/logo.png'); + expect(logo).toHaveAttribute('alt'); }); @@ it('renders branded logo with custom alt, height, and width', () => { @@ - render(<AcceptInviteBox />); - expect(screen.getByTestId('asgardeo-accept-invite')).toBeInTheDocument(); + render(<AcceptInviteBox />); + const logo = screen.getByRole('img'); + expect(logo).toHaveAttribute('src', 'https://example.com/logo.png'); + expect(logo).toHaveAttribute('alt', 'Custom Alt'); + expect(logo).toHaveAttribute('height', '50'); + expect(logo).toHaveAttribute('width', '120'); });frontend/apps/thunder-develop/src/features/login-flow/hooks/__tests__/useNodeTypes.test.tsx (1)
95-101: Simplify mock typing using Vitest's genericvi.fnsyntax.The current intersection type pattern works but is verbose. Vitest's generic
vi.fnaccepts a function signature directly, which simplifies the typing:let mockOnAddElementToView = vi.fn<(element: Element, viewId: string) => void>(); let mockOnAddElementToForm = vi.fn<(element: Element, formId: string) => void>();Then in
beforeEach, simply reassign with the same generic:mockOnAddElementToView = vi.fn<(element: Element, viewId: string) => void>(); mockOnAddElementToForm = vi.fn<(element: Element, formId: string) => void>();This eliminates the need for manual intersection types and casts, making the code more idiomatic and readable.
frontend/apps/thunder-gate/src/__tests__/components/SignIn/SignInBox.test.tsx (3)
1499-1536: Add an assertion that the fallback-key path avoids list-key warnings.
Right now the test only checks rendering; it would still pass if keys were missing and React logged warnings. Consider asserting that no key-related warning is emitted (or rename the test if you intend it as a smoke test).💡 Example: guard against key warnings
it('uses fallback index keys when components have undefined id', () => { + const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); mockSignInRenderProps = createMockSignInRenderProps({ components: [ { type: 'TEXT', label: 'Welcome', variant: 'H1', }, { type: 'BLOCK', components: [ { type: 'TEXT_INPUT', ref: 'username', label: 'Username', required: false, }, { type: 'PASSWORD_INPUT', ref: 'password', label: 'Password', required: false, }, { type: 'ACTION', eventType: 'SUBMIT', label: 'Continue', variant: 'PRIMARY', }, ], }, ], }); render(<SignInBox />); expect(screen.getByText('Welcome')).toBeInTheDocument(); expect(screen.getByLabelText(/Username/)).toBeInTheDocument(); expect(screen.getByLabelText(/Password/)).toBeInTheDocument(); + expect(consoleErrorSpy).not.toHaveBeenCalledWith( + expect.stringContaining('unique "key"') + ); + consoleErrorSpy.mockRestore(); });
1688-1828: Fallback/branding tests should assert the fallback attributes.
These cases currently only assert that rendering succeeds; they won’t fail if alt/size/placeholder fallbacks regress. Consider asserting the expectedalt,height/width, and placeholder values to make the tests meaningful (or rename them to “renders without crashing”).
1830-1889: “Field error” test doesn’t verify error UI state.
It only checks thatonSubmitisn’t called. Consider asserting error indicators (e.g.,aria-invalid, helper text, or error styling) so the test validates the stated behavior.
.../thunder-develop/src/features/login-flow/hooks/__tests__/useTemplateAndWidgetLoading.test.ts
Outdated
Show resolved
Hide resolved
DonOmalVindula
force-pushed
the
migrate-vitest-v4
branch
from
97a8a09 to
06deed9
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@frontend/apps/thunder-develop/src/features/login-flow/hooks/__tests__/useFlowInitialization.test.ts`:
- Line 181: The mockValidateEdges currently only accepts a single parameter and
ignores the nodes argument, which can hide bugs; update the mock declaration in
useFlowInitialization.test.ts so mockValidateEdges is typed and implemented to
accept both (edges: Edge[], nodes: Node[]) and return edges (e.g., vi.fn((edges:
Edge[], nodes: Node[]) => edges)) so the mocked function signature matches the
real validateEdges and the nodes parameter is available to tests; keep the
variable name mockValidateEdges and the Edge/Node types when updating the mock.
In
`@frontend/apps/thunder-gate/src/__tests__/components/SignUp/SignUpBox.test.tsx`:
- Around line 1724-1741: The test only asserts the container renders but must
validate the branded logo attributes; update the SignUpBox.test that uses
mockUseBranding so after render(<SignUpBox />) you query the image (e.g.,
screen.getByRole('img', { name: /Custom Alt/i }) or screen.getByAltText('Custom
Alt')) and add assertions that its alt equals 'Custom Alt' and its height/width
reflect 50 and 120 (or check the rendered img attributes/props/style), keeping
the existing mocked branding in place and replacing the weak container-only
expect with these image attribute checks.
- Around line 1682-1706: The test "renders with branding enabled and centered
text alignment" currently only verifies the text exists; either rename it to
"renders with branding enabled" or add an assertion that checks the rendered
text's alignment (e.g., inspect the DOM node returned by
screen.getByText('Create Account') or its container from SignUpBox for a
centered text style/class), updating the test that uses mockUseBranding and
mockSignUpRenderProps/createMockSignUpRenderProps so the assertion targets the
correct element returned by SignUpBox and verifies text-align:center (or the
expected CSS class) rather than only existence.
- Around line 1657-1660: The test currently only asserts Phone/OTP/Resend but
the test title references SELECT and TRIGGER with an undefined id; add
assertions that the SELECT combobox and TRIGGER button are rendered and have no
id. Locate where the SELECT and TRIGGER are created in the SignUpBox test and
add expectations using screen.getByRole('combobox') (or screen.getByLabelText
for the select) and screen.getByRole('button', { name: /trigger/i }) to assert
they are in the document and that their id attribute is null/undefined (e.g.,
element.getAttribute('id') === null).
- Around line 1708-1722: The test only checks the container; update it to locate
the actual logo image rendered by SignUpBox (e.g., use
within(screen.getByTestId('asgardeo-signup')).getByRole('img') or
getByTestId('branding-logo') if present) and assert the image's src includes the
mocked logo url ('https://example.com/logo.png') and that its alt attribute
matches the expected fallback text (or at minimum is non-empty) to verify both
rendering and alt fallback behavior while keeping mockUseBranding as currently
defined.
...d/apps/thunder-develop/src/features/login-flow/hooks/__tests__/useFlowInitialization.test.ts
Outdated
Show resolved
Hide resolved
frontend/apps/thunder-gate/src/__tests__/components/SignUp/SignUpBox.test.tsx
Show resolved
Hide resolved
frontend/apps/thunder-gate/src/__tests__/components/SignUp/SignUpBox.test.tsx
Show resolved
Hide resolved
frontend/apps/thunder-gate/src/__tests__/components/SignUp/SignUpBox.test.tsx
Show resolved
Hide resolved
frontend/apps/thunder-gate/src/__tests__/components/SignUp/SignUpBox.test.tsx
Show resolved
Hide resolved
DonOmalVindula
force-pushed
the
migrate-vitest-v4
branch
from
06deed9 to
a9a3450
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@frontend/apps/thunder-develop/src/components/Sidebar/__tests__/SideMenuMobile.test.tsx`:
- Around line 111-116: The test currently only checks button count and can miss
the notification bell; modify the MenuButton component used for the notification
bell (MenuButton in MenuButton.tsx) to include an accessible name such as
aria-label="notifications" on the notification MenuButton, then update the test
in SideMenuMobile.test.tsx (the 'renders notification bell button when open'
case) to explicitly assert the presence of that button by querying
getByRole('button', { name: /notifications/i }) or similar instead of relying on
buttons.length.
In `@frontend/pnpm-workspace.yaml`:
- Line 58: Update the Vitest integration to meet v4 requirements: ensure the
project Node.js runtime is bumped to >=20.0.0 and Vite to >=6.0.0, then audit
any Vitest config entries referencing removed options
(coverage.ignoreEmptyLines, coverage.experimentalAstAwareRemapping,
coverage.all, coverage.extensions) and replace them with the new semantics (use
coverage.include for file selection and remove the removed flags), and adjust
coverage hint usage for TypeScript+esbuild by preserving legal comments (e.g.,
/* `@preserve` */) where necessary; update the vitest version entry (the vitest:
4.0.17 line) only after confirming Node/Vite compatibility and then run the
test/coverage suite to validate coverage number changes due to the new AST-based
remapping.
- Around line 28-29: CI lacks Playwright browsers for Vitest's
`@vitest/browser-playwright` provider used by packages thunder-shared-hooks,
thunder-shared-contexts, thunder-shared-branding, and thunder-i18n; update the
CI workflow to run the Playwright browser installation (e.g., run npx playwright
install --with-deps or the equivalent playwright install command) before any
frontend test step that invokes pnpm test:coverage (notably in the
thunder-develop and thunder-gate test steps), ensuring the installation step
precedes pnpm test:coverage so Vitest can launch browsers during CI runs.
🧹 Nitpick comments (7)
frontend/apps/thunder-develop/src/components/Sidebar/__tests__/MenuContent.test.tsx (2)
197-212: Test name doesn't match what's being tested.The test is named "shows tooltips with item text in mini mode" but only verifies icon centering (
justifyContent: 'center'), not tooltip content. Consider renaming to "centers list item icons in mini mode" or actually testing tooltip behavior (e.g., by hovering and checking tooltip text appears).
214-228: Test name doesn't match what's being tested.The test is named "does not show tooltips in expanded mode" but verifies text visibility (
opacity,display) instead. Consider renaming to "shows text labels with full opacity in expanded mode" to accurately describe the assertion.frontend/apps/thunder-develop/src/components/Header/__tests__/LanguageSwitcher.test.tsx (1)
116-117: Consider addressing the type casting pattern for language codes.The repeated
as unknown as 'en-US'casts suggest thatSupportedLanguagetype may be too restrictive for test scenarios. While this works, it could be cleaner to either:
- Extend the mock's type to accept broader language code strings
- Add test-specific language codes to the
SupportedLanguageunion type if the component is designed to support arbitrary languagesThis is a minor maintenance concern and doesn't affect test correctness.
Also applies to: 137-137, 185-185, 211-214, 260-261, 285-286, 314-314
frontend/apps/thunder-develop/src/components/Header/__tests__/Header.test.tsx (2)
76-81: Fragile test relying on MUI internal class names.Asserting
stacks.length >= 1based on.MuiStack-rootclass selectors is fragile because:
- MUI internal class names may change between versions
- The assertion
>= 1is too permissive to catch layout regressionsConsider testing layout behavior through more stable means like role queries or data-testid attributes.
83-88: Redundant test case.This test duplicates the assertion on line 58 which already verifies
screen.getByRole('button', {name: /open notifications/i})is present.Consider removing or differentiating this test
Either remove this test entirely since it's covered in "renders all header components", or enhance it to test additional notification-specific behavior (e.g., tooltip content, interaction).
frontend/apps/thunder-develop/src/components/Navbar/__tests__/NavbarBreadcrumbs.test.tsx (1)
211-225: Avoid brittle class-based assertions and redundant coverage.
This test overlaps with existing “Develop + Users” checks and relies on.MuiTypography-root, which is an implementation detail. Consider removing it or asserting via semantic structure/text instead.frontend/apps/thunder-develop/src/components/Sidebar/__tests__/SideMenuMobile.test.tsx (1)
103-109: Avoid brittle MUI class selectors; prefer accessible queries.
.MuiAvatar-rootand.MuiDivider-rootare implementation details and can break on MUI upgrades. Prefer RTL queries that reflect user-visible semantics (e.g., alt text and separator role).♻️ Suggested test adjustments
- // Drawer renders in a portal, so use document instead of container - const avatar = document.querySelector('.MuiAvatar-root'); - expect(avatar).toBeInTheDocument(); + const avatar = screen.getByAltText('Riley Carter'); + expect(avatar).toBeInTheDocument();- // Drawer renders in a portal, so use document instead of container - const dividers = document.querySelectorAll('.MuiDivider-root'); - expect(dividers.length).toBeGreaterThan(0); + const dividers = screen.getAllByRole('separator'); + expect(dividers.length).toBeGreaterThan(0);Also applies to: 119-124
frontend/apps/thunder-develop/src/components/Sidebar/__tests__/SideMenuMobile.test.tsx
Outdated
Show resolved
Hide resolved
DonOmalVindula
force-pushed
the
migrate-vitest-v4
branch
from
a9a3450 to
08f6573
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/apps/thunder-develop/src/features/login-flow/hooks/__tests__/useFlowSave.test.ts (1)
325-342:⚠️ Potential issue | 🟡 MinorTest doesn't effectively verify pending state.
The test uses
vi.doMockandvi.resetModules()but doesn't reimport the hook, sorenderUseFlowSave()still uses the original mock. The comment on line 336 acknowledges this. The assertion only checks the type, not the actual value.Consider removing or rewriting this test to avoid misleading coverage. One approach is to expose the pending state through props or use a different testing strategy.
🤖 Fix all issues with AI agents
In
`@frontend/apps/thunder-develop/src/components/Header/__tests__/Search.test.tsx`:
- Around line 112-119: The test "preserves content after re-render" currently
only checks static attributes; update it to type into the Search input before
calling rerender and then assert the typed content still exists after rerender.
Specifically, in the test for the Search component use userEvent.type (or
fireEvent.change) on the element returned by
screen.getByPlaceholderText('Search...') (or screen.getByLabelText('Search')),
call rerender(<Search />), then assert the input's value (via
expect(...).toHaveValue(...) or checking element.value) equals the typed string
to verify content preservation.
In
`@frontend/apps/thunder-develop/src/components/Navbar/__tests__/AppNavbar.test.tsx`:
- Around line 145-152: The test in AppNavbar.test.tsx currently only checks for
the base class 'MuiAppBar-root' which doesn't confirm the box shadow is removed;
update the 'renders app bar with no box shadow' test to assert the actual style
on the rendered AppNavbar (render(<AppNavbar />)) by selecting the element
(container.querySelector('.MuiAppBar-root') or getByRole) and expecting the
computed style to show no shadow (e.g., expect(appBar).toHaveStyle('box-shadow:
none') or assert absence of elevation classes/styles), or alternatively check
that elevation-related classes are not present; modify the test assertions
accordingly to verify the boxShadow: 0 behavior.
🧹 Nitpick comments (17)
frontend/packages/thunder-test-utils/src/mocks/i18n.ts (1)
44-48: Consider adding an explicit return type for consistency.The other mock functions now have explicit return type annotations. For consistency, you could add one here as well:
export const mockUseDataGridLocaleText = (): { noRowsLabel: string; noResultsOverlayLabel: string; paginationRowsPerPage: string; } => ({ // ... });frontend/apps/thunder-develop/src/components/Sidebar/__tests__/MenuContent.test.tsx (4)
197-212: Test name does not match assertions.The test is named "shows tooltips with item text in mini mode" but only verifies that icons have
justifyContent: 'center'. It doesn't actually verify tooltip content or behavior. Consider either:
- Renaming to reflect what's actually being tested (e.g., "centers list item icons in mini mode")
- Adding assertions for actual tooltip behavior
Note: This duplicates the assertion in the "centers list item buttons in mini mode" test (lines 265-278) which also checks centering in mini mode.
214-228: Test name misleading and overlaps with existing test.This test is named "does not show tooltips in expanded mode" but actually verifies text visibility (
opacity: '1',display: 'block'). This logic is already covered by the existing test "shows text labels in expanded mode" (lines 124-143). Consider removing this test or renaming it to clarify its distinct purpose if it tests something different.
326-339: Test name does not match assertion.The test is named "sets icon margin to spacing in expanded mode" but only asserts
minWidth: 0. To match the name, it should verifymarginRightequals the spacing value (e.g.,24pxfor MUI'smr: 3). Consider either renaming the test or updating the assertion.💡 Suggested fix to match the test name
it('sets icon margin to spacing in expanded mode', async () => { const {default: SidebarContext} = await import('../context/SidebarContext'); const {container} = render( <SidebarContext.Provider value={{mini: false, fullyExpanded: true, fullyCollapsed: false, hasDrawerTransitions: true}}> <MenuContent /> </SidebarContext.Provider> ); const listItemIcons = container.querySelectorAll('.MuiListItemIcon-root'); listItemIcons.forEach((icon) => { - expect(icon).toHaveStyle({minWidth: 0}); + expect(icon).toHaveStyle({minWidth: 0, marginRight: '24px'}); }); });
341-354: Partial overlap with existing test.This test overlaps with "hides text labels in mini mode" (lines 103-122) which already verifies
display: 'none'. The additionalopacity: 0assertion provides incremental value. Consider consolidating these into a single comprehensive test.frontend/apps/thunder-develop/src/components/Header/__tests__/LanguageSwitcher.test.tsx (2)
116-117: Consider defining test language types for better type safety.The
as unknown as 'en-US'double cast pattern bypasses type checking. While acceptable in tests, consider creating a test helper or type for mock language data to avoid scattering these casts throughout.♻️ Suggested approach
Define a helper at the top of the file or in test-utils:
// Helper to create mock language for testing const createMockLanguage = ( code: string, name: string, nativeName: string, direction: 'ltr' | 'rtl' = 'ltr' ) => ({ code, name, nativeName, direction, } as const); // Usage in tests: availableLanguages: [ createMockLanguage('en-US', 'English (United States)', 'English (US)'), createMockLanguage('es-ES', 'Spanish (Spain)', 'Español (España)'), ],
332-337: Consider using a more resilient selector for the icon.The CSS class selector
svg.lucide-languagesis coupled to the icon library's implementation. If Lucide changes its class naming convention, this test would break.♻️ Alternative approach
Consider adding a
data-testidto the icon in the component, or accept that this is a reasonable trade-off for testing icon presence:- const languagesIcon = container.querySelector('svg.lucide-languages'); + // Verify SVG icon is present within the button + const button = screen.getByRole('button', {name: /change language/i}); + const languagesIcon = button.querySelector('svg'); expect(languagesIcon).toBeInTheDocument();frontend/apps/thunder-develop/src/components/Header/__tests__/Header.test.tsx (4)
62-67: Same concern as LanguageSwitcher: CSS class selector for icon.Using
svg.lucide-bellis implementation-specific. Consider the same refactor suggestion as noted for the LanguageSwitcher tests.
76-81: Layout assertions are tightly coupled to MUI implementation.These tests rely on MUI's internal CSS class names (
.MuiStack-root) and computed styles. Such tests can break with MUI version updates without any functional change to your component.Consider whether these layout tests provide sufficient value, or if the functional tests (checking that expected elements render) already provide adequate coverage.
♻️ Alternative approach
If layout verification is critical, consider:
- Adding
data-testidattributes to key layout containers- Using snapshot testing for structural verification
- Relying on visual regression testing for layout concerns
- it('renders with correct layout structure', () => { - const {container} = render(<Header />); - - const stacks = container.querySelectorAll('.MuiStack-root'); - expect(stacks.length).toBeGreaterThanOrEqual(1); - }); + it('renders with correct layout structure', () => { + const {container} = render(<Header />); + + // Verify the header renders as a flex container + const header = container.firstChild; + expect(header).toBeInTheDocument(); + });Also applies to: 90-97
83-88: Duplicate test assertion.This test ("renders notifications tooltip") checks the same assertion as the existing test on line 58 in "renders all header components". Consider removing this test to avoid redundancy, or expand it to verify tooltip-specific behavior.
♻️ Suggested fix
Either remove this test, or enhance it to test actual tooltip behavior:
- it('renders notifications tooltip', () => { - render(<Header />); - - const notificationsButton = screen.getByRole('button', {name: /open notifications/i}); - expect(notificationsButton).toBeInTheDocument(); - }); + it('shows tooltip on notifications button hover', async () => { + render(<Header />); + + const notificationsButton = screen.getByRole('button', {name: /open notifications/i}); + fireEvent.mouseEnter(notificationsButton); + + await waitFor(() => { + expect(screen.getByRole('tooltip')).toBeInTheDocument(); + }); + });
99-104: Duplicate assertions already covered in "renders all header components".Lines 59 and 57 in the existing test already verify the language button and theme toggle presence. These separate tests don't add new coverage.
♻️ Consider consolidating or removing
If component isolation testing is the goal, the tests are acceptable. Otherwise, consider removing to reduce test maintenance burden:
- it('renders LanguageSwitcher component', () => { - render(<Header />); - - const languageButton = screen.getByRole('button', {name: /change language/i}); - expect(languageButton).toBeInTheDocument(); - }); - - it('renders ColorSchemeToggle component', () => { - render(<Header />); - - expect(screen.getByTestId('theme-toggle')).toBeInTheDocument(); - });Also applies to: 106-110
frontend/apps/thunder-develop/src/components/Header/__tests__/Search.test.tsx (1)
81-100: Consider using semantic queries instead of MUI class selectors.These tests rely on MUI-specific CSS classes (
.MuiOutlinedInput-root,.MuiInputAdornment-positionStart,.MuiInputBase-sizeSmall) which are implementation details that could break if MUI changes its internal naming conventions. The existing tests in this file appropriately use semantic queries likegetByPlaceholderTextandgetByLabelText.For the start adornment test (lines 88-93), you could verify the icon exists within the input's structure using role queries or the existing
svg.lucide-searchselector from line 41.frontend/apps/thunder-develop/src/components/Navbar/__tests__/AppNavbar.test.tsx (1)
180-186: Test name and assertion are mismatched.The test claims to verify a gradient background but only checks
borderRadius. Either rename the test or assert the gradient-related style to match intent.frontend/apps/thunder-gate/src/__tests__/components/SignUp/SignUpBox.test.tsx (1)
1562-1607: Test name claims fallback keys but only asserts rendering.Consider asserting the absence of React key warnings (or rename the test to match current coverage).
💡 Optional assertion to validate key handling
it('uses fallback index keys when components have undefined id', () => { + const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); mockSignUpRenderProps = createMockSignUpRenderProps({ components: [ { type: 'TEXT', label: 'Create Account', variant: 'H1', }, { type: 'BLOCK', components: [ { type: 'TEXT_INPUT', ref: 'firstName', label: 'First Name', placeholder: 'Enter first name', required: false, }, { type: 'PASSWORD_INPUT', ref: 'password', label: 'Password', placeholder: 'Enter password', required: false, }, { type: 'EMAIL_INPUT', ref: 'email', label: 'Email', placeholder: 'Enter email', required: false, }, { type: 'ACTION', eventType: 'SUBMIT', label: 'Register', variant: 'SECONDARY', }, ], }, ], }); render(<SignUpBox />); expect(screen.getByText('Create Account')).toBeInTheDocument(); expect(screen.getByLabelText(/First Name/)).toBeInTheDocument(); + expect(consoleErrorSpy).not.toHaveBeenCalledWith( + expect.stringMatching(/unique "key"/i), + ); + consoleErrorSpy.mockRestore(); });frontend/apps/thunder-gate/src/__tests__/components/AcceptInvite/AcceptInviteBox.test.tsx (2)
1274-1368: Fallback‑key tests don’t actually validate key behavior.React keys aren’t visible in the DOM, so these assertions can pass even if the fallback keys are broken. Consider asserting that no “unique key” warning is emitted to make the tests match their intent.
🔍 Possible tweak (apply similarly to the other fallback‑key tests)
it('uses fallback index keys when components have undefined id', () => { + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); mockAcceptInviteRenderProps = createMockAcceptInviteRenderProps({ components: [ { type: 'TEXT', @@ }); render(<AcceptInviteBox />); + const warnings = consoleSpy.mock.calls.flat().join(' '); + expect(warnings).not.toMatch(/unique "key"/i); + consoleSpy.mockRestore(); expect(screen.getByText('Accept Invite')).toBeInTheDocument(); expect(screen.getByLabelText(/Full Name/)).toBeInTheDocument(); });
1396-1429: Branding logo tests only assert container presence.The test names mention alt/size behavior, but they don’t assert the logo element itself. Consider asserting the rendered logo’s attributes (alt/src/height/width) so the tests exercise the intended behavior.
frontend/apps/thunder-develop/src/hooks/__tests__/useDataGridLocaleText.test.tsx (1)
323-351: Prefer restoring local spies instead ofvi.restoreAllMocks().
vi.restoreAllMocks()can reset unrelated spies created elsewhere in the file or shared test setup. It’s safer to restore only the spies you create here.♻️ Targeted restoration
- const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); - - vi.spyOn(i18nModule.default, 'getResourceBundle').mockImplementation((lng: string, ns: string) => { + const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const getResourceBundleSpy = vi + .spyOn(i18nModule.default, 'getResourceBundle') + .mockImplementation((lng: string, ns: string) => { const bundle = originalGetResourceBundle(lng, ns) as Record<string, unknown>; return { ...bundle, // Override a function key with a string value to trigger the DEV warning 'dataTable.toolbarFiltersTooltipActive': 'not-a-function', }; }); ... - consoleSpy.mockRestore(); - vi.restoreAllMocks(); + consoleSpy.mockRestore(); + getResourceBundleSpy.mockRestore();- vi.spyOn(i18nModule.default, 'getResourceBundle').mockImplementation(() => ({})); + const getResourceBundleSpy = vi + .spyOn(i18nModule.default, 'getResourceBundle') + .mockImplementation(() => ({})); ... - vi.restoreAllMocks(); + getResourceBundleSpy.mockRestore();Also applies to: 353-371
frontend/apps/thunder-develop/src/components/Header/__tests__/Search.test.tsx
Outdated
Show resolved
Hide resolved
frontend/apps/thunder-develop/src/components/Navbar/__tests__/AppNavbar.test.tsx
Outdated
Show resolved
Hide resolved
DonOmalVindula
force-pushed
the
migrate-vitest-v4
branch
2 times, most recently
from
25565b6 to
a267c29
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/apps/thunder-develop/src/features/organization-units/components/edit-organization-unit/__tests__/EditGroups.test.tsx (1)
24-28:⚠️ Potential issue | 🟡 MinorMock doesn't accept the
organizationUnitIdparameter that the component passes, so the test can't verify it's being propagated correctly.The hook is called with
organizationUnitId(line 53 of EditGroups.tsx), but the mock's default export doesn't accept any parameters. The test named "should pass organizationUnitId to the API hook" (lines 193-197) only checks for text rendering, not parameter passing. Update the mock to capture the parameter and assert it's called with the expected ID.✅ Proposed fix
const mockUseGetOrganizationUnitGroups = vi.fn(); vi.mock('../../../api/useGetOrganizationUnitGroups', () => ({ - default: () => mockUseGetOrganizationUnitGroups() as {data: GroupListResponse | undefined; isLoading: boolean}, + default: (organizationUnitId: string) => + mockUseGetOrganizationUnitGroups(organizationUnitId) as {data: GroupListResponse | undefined; isLoading: boolean}, }));it('should pass organizationUnitId to the API hook', () => { renderWithProviders(<EditGroups organizationUnitId="different-ou" />); expect(screen.getByText('Groups')).toBeInTheDocument(); + expect(mockUseGetOrganizationUnitGroups).toHaveBeenCalledWith('different-ou'); });frontend/apps/thunder-develop/src/features/organization-units/components/edit-organization-unit/__tests__/EditUsers.test.tsx (1)
24-28:⚠️ Potential issue | 🟡 MinorMock hook should accept and forward the
organizationUnitIdparameter for proper testing.The mocked hook currently ignores parameters passed to it. The test at lines 193-197 is named "should pass organizationUnitId to the API hook" but only checks rendering—it doesn't verify the parameter is actually passed. Update the mock to accept
organizationUnitIdand add an assertion to confirm the hook receives the correct value:Proposed fix
const mockUseGetOrganizationUnitUsers = vi.fn(); vi.mock('../../../api/useGetOrganizationUnitUsers', () => ({ - default: () => mockUseGetOrganizationUnitUsers() as {data: UserListResponse | undefined; isLoading: boolean}, + default: (organizationUnitId: string) => + mockUseGetOrganizationUnitUsers(organizationUnitId) as {data: UserListResponse | undefined; isLoading: boolean}, }));it('should pass organizationUnitId to the API hook', () => { renderWithProviders(<EditUsers organizationUnitId="different-ou" />); expect(screen.getByText('Users')).toBeInTheDocument(); + expect(mockUseGetOrganizationUnitUsers).toHaveBeenCalledWith('different-ou'); });Also applies to: 193-197
🤖 Fix all issues with AI agents
In
`@frontend/apps/thunder-develop/src/features/organization-units/api/__tests__/useCreateOrganizationUnit.test.ts`:
- Around line 241-257: The test doesn't assert that
queryClient.invalidateQueries was actually invoked, so store the spy returned by
vi.spyOn(queryClient, 'invalidateQueries') in a const (e.g., const invalidateSpy
= ...) before stubbing it, call result.current.mutate(createRequest) as before,
then add an assertion like expect(invalidateSpy).toHaveBeenCalled() after the
waitFor to confirm the rejection path is exercised; reference
useCreateOrganizationUnit, queryClient.invalidateQueries, and
result.current.mutate when adding the assertion.
In
`@frontend/apps/thunder-develop/src/features/organization-units/components/edit-organization-unit/__tests__/EditChildOUs.test.tsx`:
- Around line 307-311: The test name claims to verify that organizationUnitId is
passed to the API hook but only checks the rendered title; update the test to
either (A) actually assert the hook received the prop by mocking the API hook
used by EditChildOUs (e.g., jest.mock the module and assert the mocked
hook/function was called with "test-ou-id"), referencing the component
EditChildOUs and the API hook name used inside it, or (B) if you don't want to
mock the hook here, rename the test to reflect the current assertion (e.g.,
"should render with custom organizationUnitId") so the test name matches what is
validated.
In
`@frontend/apps/thunder-gate/src/__tests__/components/SignUp/SignUpBox.test.tsx`:
- Around line 1756-1774: The test currently only asserts that the container
('asgardeo-signup') exists but intends to verify the social login trigger;
update the assertion to check for the trigger element associated with the
provider button (e.g., query for an element tied to the component id
'provider-btn' or the rendered trigger text/role) after rendering SignUpBox with
mockSignUpRenderProps created via createMockSignUpRenderProps, or alternatively
rename the test to "does not crash when label/image missing" if the intent is
only to ensure no crash rather than presence of a trigger.
In `@frontend/packages/thunder-shared-branding/vitest.config.ts`:
- Around line 19-28: The vitest config uses the provider function playwright()
from `@vitest/browser-playwright` but the Playwright runtime is not declared in
devDependencies; add either "playwright" or "playwright-core" to devDependencies
(matching the Playwright version required by Vitest v4's browser-playwright peer
dependency) and install it (e.g., npm/yarn/pnpm add -D playwright or
playwright-core) so the provider playwright() can resolve at runtime; update
package.json devDependencies and reinstall to update lockfile.
In `@frontend/packages/thunder-shared-contexts/vitest.config.ts`:
- Around line 19-28: The package is importing and using playwright() from
`@vitest/browser-playwright` (see the import of playwright and the call to
playwright() in vitest.config), but playwright is missing from devDependencies;
add "playwright": "catalog:" to this package's devDependencies (matching the
workspace catalog version) so the peerDependency requirement for
`@vitest/browser-playwright` is satisfied.
🧹 Nitpick comments (18)
frontend/apps/thunder-develop/src/components/Sidebar/__tests__/MenuContent.test.tsx (3)
197-212: Test name is misleading — it verifies icon styling, not tooltip content.The test is named "shows tooltips with item text in mini mode" but only verifies that
ListItemIconelements havejustifyContent: 'center'. It doesn't actually assert anything about tooltip titles or their presence.Consider either renaming the test to accurately describe what it verifies (e.g., "centers list item icons in mini mode") or actually testing the tooltip behavior.
♻️ Option 1: Rename to match actual behavior
- it('shows tooltips with item text in mini mode', async () => { + it('centers list item icons in mini mode', async () => { const {default: SidebarContext} = await import('../context/SidebarContext'); render(
326-339: Test name doesn't match assertion.The test is named "sets icon margin to spacing in expanded mode" but asserts
minWidth: 0instead of any margin-related property. According to the relevant code snippet fromMenuContent.tsx, in expanded mode the icon'smarginRightshould not be'auto'(unlike mini mode).Consider updating the test name or the assertion to match the intended behavior.
♻️ Suggested fix: Rename test to match assertion
- it('sets icon margin to spacing in expanded mode', async () => { + it('sets icon minWidth to 0 in expanded mode', async () => { const {default: SidebarContext} = await import('../context/SidebarContext');
214-228: Test name doesn't match the assertions.The test is named "does not show tooltips in expanded mode" but verifies that
ListItemTextelements haveopacity: '1'anddisplay: 'block'. The assertions don't verify tooltip absence.Consider renaming to accurately reflect what's being tested.
♻️ Suggested fix
- it('does not show tooltips in expanded mode', async () => { + it('shows text labels with full opacity in expanded mode', async () => { const {default: SidebarContext} = await import('../context/SidebarContext');frontend/apps/thunder-develop/src/components/Header/__tests__/LanguageSwitcher.test.tsx (1)
339-349: Consider usingscreen.getByRolefor consistency.The test correctly verifies the menu ID, but using
document.getElementByIdis slightly inconsistent with thescreenquery patterns used elsewhere in this file.♻️ Optional: Use screen query with attribute check
await waitFor(() => { - const menu = document.getElementById('language-menu'); - expect(menu).toBeInTheDocument(); + const menu = screen.getByRole('menu'); + expect(menu).toHaveAttribute('id', 'language-menu'); });frontend/apps/thunder-develop/src/features/organization-units/api/__tests__/useGetOrganizationUnitGroups.test.ts (1)
217-227: Minor inconsistency:isFetchingvsisLoading.This test uses
isFetchingon line 224, while the similar existing test "should not fetch when organizationUnitId is undefined" (line 92) usesisLoading. Consider aligning these for consistency across similar test cases.♻️ Suggested fix for consistency
- expect(result.current.isFetching).toBe(false); + expect(result.current.isLoading).toBe(false);frontend/apps/thunder-develop/src/components/Navbar/__tests__/NavbarBreadcrumbs.test.tsx (1)
266-282: Consider the brittleness of testing MUI internal styles.Line 281 asserts on
alignItems: 'center'which appears to be MUI's default internal styling for the breadcrumbsolelement rather than an explicit style set by the component. This test could become flaky if MUI updates their internal styles in a future version.If the intent is to verify custom styling applied by
StyledBreadcrumbs, consider testing only the styles explicitly defined in that styled component. If verifying the general visual structure, the existence of the element (line 280) may be sufficient.frontend/apps/thunder-develop/src/features/organization-units/api/__tests__/useGetOrganizationUnitUsers.test.ts (2)
217-227: Minor inconsistency: usesisFetchinginstead ofisLoading.The similar test for
undefinedon line 92 usesisLoading, while this test usesisFetching. Both assertions are valid for disabled queries, but usingisLoadingwould maintain consistency with the existing test pattern in this file.♻️ Optional: align with existing test pattern
- expect(result.current.isFetching).toBe(false); + expect(result.current.isLoading).toBe(false);
229-249: Consider using explicitinitialPropspattern for clearer re-render tests.The current closure-based approach works, but React Testing Library's explicit
initialPropspattern is more readable and makes the prop change explicit. This avoids relying on closure capture semantics.♻️ Optional: use explicit initialProps pattern
it('should re-fetch when organizationUnitId changes', async () => { mockHttpRequest.mockResolvedValue({data: mockUserList}); - let ouId = 'ou-123'; - const {result, rerender} = renderHook(() => useGetOrganizationUnitUsers(ouId)); + const {result, rerender} = renderHook( + ({id}: {id: string}) => useGetOrganizationUnitUsers(id), + {initialProps: {id: 'ou-123'}}, + ); await waitFor(() => { expect(result.current.data).toEqual(mockUserList); }); - ouId = 'ou-456'; - rerender(); + rerender({id: 'ou-456'}); await waitFor(() => { expect(mockHttpRequest).toHaveBeenCalledWith( expect.objectContaining({ url: expect.stringContaining('/organization-units/ou-456/users') as unknown, }), ); }); });frontend/apps/thunder-develop/src/features/organization-units/api/__tests__/useGetOrganizationUnit.test.ts (2)
187-198: Combine this with the existing “enabled is false” test to avoid duplication.You already have a test at Lines 94-105 covering
enabled === falsewith a valid id. Consider extending that test to assertisFetching === falseinstead of adding a near-duplicate case.
190-206: Avoid real timers; usewaitFor(or fake timers) for deterministic tests.The 100ms
setTimeoutintroduces avoidable latency/flakiness. PreferwaitForassertions so the test resolves as soon as the state settles.♻️ Proposed change (use
waitForinstead ofsetTimeout)- // Wait a bit to ensure query doesn't execute - await new Promise((resolve) => { - setTimeout(resolve, 100); - }); - - expect(result.current.isFetching).toBe(false); - expect(result.current.data).toBeUndefined(); - expect(mockHttpRequest).not.toHaveBeenCalled(); + await waitFor(() => { + expect(result.current.isFetching).toBe(false); + expect(result.current.data).toBeUndefined(); + expect(mockHttpRequest).not.toHaveBeenCalled(); + });- // Wait a bit to ensure query doesn't execute - await new Promise((resolve) => { - setTimeout(resolve, 100); - }); - - expect(result.current.isFetching).toBe(false); - expect(result.current.data).toBeUndefined(); - expect(mockHttpRequest).not.toHaveBeenCalled(); + await waitFor(() => { + expect(result.current.isFetching).toBe(false); + expect(result.current.data).toBeUndefined(); + expect(mockHttpRequest).not.toHaveBeenCalled(); + });frontend/apps/thunder-develop/src/hooks/__tests__/useDataGridLocaleText.test.tsx (1)
27-464: Consider addingafterEachfor consistent mock cleanup.While each test properly calls
vi.restoreAllMocks(), adding a shared cleanup at thedescribelevel would provide a safety net if future tests forget cleanup or if a test fails before reaching the cleanup code:describe('useDataGridLocaleText', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + it('should return localized text object for DataGrid', () => {This is optional but improves test resilience.
frontend/apps/thunder-develop/src/features/organization-units/pages/__tests__/OrganizationUnitEditPage.test.tsx (1)
925-953: Test relies on conditional execution that may silently pass if button not found.The
if (nameEditButton)pattern means this test will pass without executing assertions if the button isn't found. Consider failing explicitly if the button is missing.💡 Suggested improvement
const nameEditButton = editButtons.find( (btn) => btn.querySelector('svg') && btn.closest('div')?.textContent?.includes('Test Organization Unit'), ); - if (nameEditButton) { + expect(nameEditButton).toBeDefined(); + if (nameEditButton) { fireEvent.click(nameEditButton);Alternatively, use
expect(nameEditButton).toBeDefined()before the conditional to ensure the test fails properly if the button isn't found.frontend/apps/thunder-develop/src/features/organization-units/components/__tests__/OrganizationUnitsList.test.tsx (1)
572-591: Test data intentionally omitsdescriptionproperty.The test at line 579 creates an organization unit without a
descriptionproperty to test undefined handling. This is valid but could benefit from an inline comment for clarity.📝 Consider adding a clarifying comment
organizationUnits: [ + // Intentionally omit description to test undefined handling {id: 'ou-1', handle: 'test', name: 'Test OU', parent: null}, ],frontend/apps/thunder-gate/src/__tests__/components/SignUp/SignUpBox.test.tsx (1)
1562-1607: Consider asserting no React key warnings for fallback-key coverage.This test’s intent is about fallback keys, but it only checks rendering. You can make the intent concrete by asserting no key warning is logged.
Proposed test enhancement
it('uses fallback index keys when components have undefined id', () => { + const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); mockSignUpRenderProps = createMockSignUpRenderProps({ components: [ { type: 'TEXT', label: 'Create Account', @@ render(<SignUpBox />); expect(screen.getByText('Create Account')).toBeInTheDocument(); expect(screen.getByLabelText(/First Name/)).toBeInTheDocument(); + expect(consoleErrorSpy).not.toHaveBeenCalledWith( + expect.stringContaining('Each child in a list should have a unique "key"') + ); + consoleErrorSpy.mockRestore(); });frontend/apps/thunder-develop/src/features/organization-units/api/__tests__/useGetChildOrganizationUnits.test.ts (1)
229-250: Consider usinginitialPropspattern for clearer re-render testing.The mutable closure variable approach works but is less explicit. Using
renderHookwithinitialPropsmakes the test intent clearer and is the recommended pattern from Testing Library.♻️ Suggested refactor using initialProps pattern
it('should re-fetch when parentId changes', async () => { mockHttpRequest.mockResolvedValue({data: mockChildOUList}); - let parentId = 'parent-1'; - const {result, rerender} = renderHook(() => useGetChildOrganizationUnits(parentId)); + const {result, rerender} = renderHook( + ({parentId}) => useGetChildOrganizationUnits(parentId), + {initialProps: {parentId: 'parent-1'}}, + ); await waitFor(() => { expect(result.current.data).toEqual(mockChildOUList); }); // Change parentId and re-render - parentId = 'parent-2'; - rerender(); + rerender({parentId: 'parent-2'}); await waitFor(() => { expect(mockHttpRequest).toHaveBeenCalledWith( expect.objectContaining({ url: expect.stringContaining('/organization-units/parent-2/ous') as unknown, }), ); }); });frontend/apps/thunder-develop/src/features/organization-units/components/edit-organization-unit/__tests__/EditChildOUs.test.tsx (1)
286-297: Consider consolidating with existing avatar test.This test is similar to the existing test at lines 207-216 ("should render avatars for each child OU"). The only difference is the assertion
>= 2vs> 0. Consider whether both tests are needed or if the existing test should be updated with the more specific assertion.frontend/apps/thunder-develop/src/features/organization-units/components/edit-organization-unit/__tests__/EditGroups.test.tsx (1)
173-184: Consider consolidating duplicate avatar assertions.This overlaps with the earlier “should render avatars for each group” test. You could merge to keep one strong assertion (e.g., count ≥ expected rows).
frontend/apps/thunder-develop/src/features/organization-units/components/edit-organization-unit/__tests__/EditUsers.test.tsx (1)
173-184: Consider consolidating duplicate avatar assertions.This overlaps with the earlier avatar test; merging can reduce redundancy.
...nder-develop/src/features/organization-units/api/__tests__/useCreateOrganizationUnit.test.ts
Outdated
Show resolved
Hide resolved
...eatures/organization-units/components/edit-organization-unit/__tests__/EditChildOUs.test.tsx
Outdated
Show resolved
Hide resolved
frontend/apps/thunder-gate/src/__tests__/components/SignUp/SignUpBox.test.tsx
Show resolved
Hide resolved
DonOmalVindula
force-pushed
the
migrate-vitest-v4
branch
from
a267c29 to
e89c8b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In
`@frontend/apps/thunder-develop/src/components/Header/__tests__/LanguageSwitcher.test.tsx`:
- Around line 254-402: The test suite uses vi.clearAllMocks() which only clears
call history; change the beforeEach to call vi.resetAllMocks() (or explicitly
reset mockUseLanguage's implementation) so mockReturnValue overrides in tests do
not leak between cases; update the file's beforeEach that currently calls
vi.clearAllMocks() to use vi.resetAllMocks() or add a helper that calls
mockUseLanguage.mockReset() and re-establishes a default implementation,
ensuring references to mockUseLanguage, mockSetLanguage, and other mocks are
fully reset before each test.
In
`@frontend/apps/thunder-develop/src/features/applications/components/edit-application/token-settings/__tests__/TokenUserAttributesSection.test.tsx`:
- Around line 411-435: The test TokenUserAttributesSection.test.tsx claims
newAttribute should NOT appear when activeTokenType ("id") doesn't match
tokenType ("access") but only asserts 'email' is present; update the test for
TokenUserAttributesSection to also assert that the pending addition value (e.g.,
'newAttribute') is NOT present in container.textContent when passing
pendingAdditions and activeTokenType !== tokenType, ensuring the negative case
for pendingAdditions is verified alongside the existing positive assertion for
'email'.
In
`@frontend/apps/thunder-develop/src/features/applications/pages/__tests__/ApplicationCreatePage.test.tsx`:
- Around line 1102-1153: The test’s vi.doMock call has no effect because
ApplicationCreatePage/ApplicationCreateProvider already imported useGetUserTypes
earlier; replace the dynamic per-test doMock by using a hoisted mutable mock
(e.g., define mockUserTypesData via vi.hoisted and have the vi.mock for
'../../../user-types/api/useGetUserTypes' return that object) and in this test
mutate mockUserTypesData.schemas = [{name: 'customer', displayName: 'Customer'}]
before calling renderWithProviders(); alternatively, if you prefer isolating
imports per-test, call vi.resetModules() then dynamically import
ApplicationCreatePage/ApplicationCreateProvider after registering vi.doMock for
useGetUserTypes so the new mock is used.
In
`@frontend/apps/thunder-develop/src/features/applications/pages/__tests__/ApplicationEditPage.test.tsx`:
- Around line 1060-1122: The tests overwrite navigator.clipboard but never
restore it, risking global test leakage; save the original clipboard reference
before mutating (e.g., const originalClipboard = navigator.clipboard) and
restore it in an afterEach/afterAll teardown inside the same "Copy to Clipboard"
describe block; for each test where you call Object.defineProperty(navigator,
'clipboard', { value: { writeText: mockWriteText }, ... }), restore by
re-defining navigator.clipboard back to originalClipboard (or delete the
property if originalClipboard is undefined) so functions like mockWriteText and
renderComponent/EditGeneralSettingsMock do not leak state to other tests.
In
`@frontend/apps/thunder-develop/src/features/organization-units/components/edit-organization-unit/__tests__/EditGroups.test.tsx`:
- Around line 193-197: The test "should pass organizationUnitId to the API hook"
doesn't assert the hook was called with the expected ID; update the test for
EditGroups to verify mockUseGetOrganizationUnitGroups received "different-ou"
(or adjust the mock to capture its argument) when rendering via
renderWithProviders(<EditGroups organizationUnitId="different-ou" />), by spying
on or implementing the mock for mockUseGetOrganizationUnitGroups to record its
call and asserting
expect(mockUseGetOrganizationUnitGroups).toHaveBeenCalledWith(expect.objectContaining({
organizationUnitId: 'different-ou' })) or remove the test if capturing the
argument is not feasible with the current mock structure.
In `@frontend/packages/thunder-shared-branding/package.json`:
- Around line 67-68: The package.json currently lists
"@vitest/browser-playwright" and "vitest" but is missing the required peer
dependency for Playwright; add "playwright": "catalog:" to the devDependencies
section of this package.json so `@vitest/browser-playwright` has its peer
satisfied (update the devDependencies object alongside the existing
"@vitest/browser-playwright" and "vitest" entries).
In `@frontend/pnpm-workspace.yaml`:
- Around line 28-29: Add a pinned playwright entry to the pnpm workspace catalog
so `@vitest/browser-playwright` can consume it as a peer dependency; update the
catalog block that contains '@vitest/browser-playwright' and
'@vitest/coverage-istanbul' by adding a new key "playwright" with a fixed
version (for example "1.36.1") so packages can reference it via "playwright":
"catalog:".
🧹 Nitpick comments (15)
frontend/apps/thunder-develop/src/features/login-flow/hooks/__tests__/useFlowSave.test.ts (1)
84-86: Type casts correctly match declarations.The explicit casts ensure type consistency between declaration and initialization.
Consider extracting a type alias if this pattern expands to more mocks, though current usage is reasonable.
♻️ Optional: Type alias to reduce repetition
+type MockFn<T extends (...args: unknown[]) => unknown> = ReturnType<typeof vi.fn> & T; + describe('useFlowSave', () => { - let mockShowError: ReturnType<typeof vi.fn> & ((message: string) => void); - let mockShowSuccess: ReturnType<typeof vi.fn> & ((message: string) => void); - let mockSetOpenValidationPanel: ReturnType<typeof vi.fn> & ((open: boolean) => void); + let mockShowError: MockFn<(message: string) => void>; + let mockShowSuccess: MockFn<(message: string) => void>; + let mockSetOpenValidationPanel: MockFn<(open: boolean) => void>; beforeEach(() => { vi.clearAllMocks(); vi.useFakeTimers(); - mockShowError = vi.fn() as ReturnType<typeof vi.fn> & ((message: string) => void); - mockShowSuccess = vi.fn() as ReturnType<typeof vi.fn> & ((message: string) => void); - mockSetOpenValidationPanel = vi.fn() as ReturnType<typeof vi.fn> & ((open: boolean) => void); + mockShowError = vi.fn() as MockFn<(message: string) => void>; + mockShowSuccess = vi.fn() as MockFn<(message: string) => void>; + mockSetOpenValidationPanel = vi.fn() as MockFn<(open: boolean) => void>; });frontend/apps/thunder-develop/src/features/applications/components/edit-application/general-settings/__tests__/AccessSection.test.tsx (1)
289-323: Consider making the assertion more specific about the value passed.The test correctly validates that
onFieldChangeis called for the URL field after rerender, but the assertion at lines 320-321 only verifies that at least one call was made. Asserting the exact value would strengthen the test's coverage and align with the more specific assertion pattern used inContactsSection.test.tsx.✨ Proposed refinement for stronger assertion
// The useEffect sees that the form's watched url !== currentUrl (which is now 'https://changed.com') // so it calls onFieldChange with the form's current value await waitFor(() => { - const urlCalls = mockOnFieldChange.mock.calls.filter((call) => call[0] === 'url'); - expect(urlCalls.length).toBeGreaterThan(0); + expect(mockOnFieldChange).toHaveBeenCalledWith('url', 'https://example.com'); });frontend/apps/thunder-develop/src/i18n/__tests__/I18nProvider.test.tsx (2)
423-465: Consider asserting call count to verify memoization behavior.The test comments indicate the intent is to "exercise the React Compiler memoization skip paths," but the assertions only verify the arguments passed to
mockAddResourceBundle, not how many times it was called. To confirm memoization is preventing redundant calls across re-renders, consider adding a call count assertion:// Translations should still be merged correctly after re-renders + // Verify memoization prevents duplicate calls on re-render + expect(mockAddResourceBundle).toHaveBeenCalledTimes(1); expect(mockAddResourceBundle).toHaveBeenCalledWith( 'en-US', 'common', expect.objectContaining({greeting: 'Hello'}), true, true, );Without this, the test verifies the component handles re-renders gracefully but doesn't prove the memoization is working as intended.
467-502: Assertions don't verify cached memoization behavior.The test description and comments state the goal is to "exercise cached paths" for the React Compiler's internal cache, but the only assertions check that the text remains in the document—which would pass regardless of whether memoization is working.
To meaningfully verify caching behavior, consider asserting that
mockAddResourceBundleis called only once despite multiple rerenders:expect(getByText('Test')).toBeInTheDocument(); + + // Verify memoization: addResourceBundle should not be called again on rerender + expect(mockAddResourceBundle).toHaveBeenCalledTimes(1); });Alternatively, if the intent is simply to ensure the component doesn't break on rerender (smoke test), consider updating the test name and comments to reflect that purpose rather than referencing memoization.
frontend/apps/thunder-develop/src/features/applications/components/edit-application/token-settings/__tests__/TokenUserAttributesSection.test.tsx (1)
446-465: Test lacks meaningful assertions for toggle behavior.The test clicks the accordion button twice but doesn't verify that the accordion actually toggles. It only asserts that the button exists. Consider adding assertions to verify the expanded/collapsed state or content visibility changes.
Suggested improvement with state verification
it('should toggle default attributes accordion when clicked', async () => { const user = userEvent.setup(); render(<TestWrapper />); // Find the Default Attributes accordion header const defaultAttributesHeader = screen.getByText('Default Attributes'); const accordionButton = defaultAttributesHeader.closest('button'); expect(accordionButton).toBeDefined(); if (accordionButton) { + // Verify accordion is initially in expected state (expanded by component default) + expect(accordionButton).toHaveAttribute('aria-expanded', 'true'); + // Click to collapse await user.click(accordionButton); + expect(accordionButton).toHaveAttribute('aria-expanded', 'false'); + // Click to expand again await user.click(accordionButton); + expect(accordionButton).toHaveAttribute('aria-expanded', 'true'); } });frontend/apps/thunder-develop/src/features/organization-units/pages/__tests__/OrganizationUnitEditPage.test.tsx (2)
937-952: Consider using an assertion instead of conditional block to avoid silent test passes.The
if (nameEditButton)pattern will cause the test to pass silently without executing any assertions if the button is not found. This pattern exists in other tests in this file, but for new tests, consider failing explicitly if the required element is missing.♻️ Proposed refactor to fail explicitly
- if (nameEditButton) { - fireEvent.click(nameEditButton); + expect(nameEditButton).toBeDefined(); + fireEvent.click(nameEditButton!); - await waitFor(() => { - expect(screen.getByDisplayValue('Test Organization Unit')).toBeInTheDocument(); - }); + await waitFor(() => { + expect(screen.getByDisplayValue('Test Organization Unit')).toBeInTheDocument(); + }); - const textbox = screen.getByDisplayValue('Test Organization Unit'); - fireEvent.change(textbox, {target: {value: ' '}}); - fireEvent.keyDown(textbox, {key: 'Enter'}); + const textbox = screen.getByDisplayValue('Test Organization Unit'); + fireEvent.change(textbox, {target: {value: ' '}}); + fireEvent.keyDown(textbox, {key: 'Enter'}); - // Should not show unsaved changes since empty names are not saved - await waitFor(() => { - expect(screen.getByText('Test Organization Unit')).toBeInTheDocument(); - }); - } + // Should not show unsaved changes since empty names are not saved + await waitFor(() => { + expect(screen.getByText('Test Organization Unit')).toBeInTheDocument(); + });
1067-1072: Prefer Testing Library queries over direct DOM access.Using
document.querySelector('textarea')bypasses Testing Library's accessibility-focused queries. Consider usingscreen.getByRole('textbox')or a similar query for consistency with the rest of the test file.♻️ Proposed refactor
await waitFor(() => { - const textbox = document.querySelector('textarea'); - expect(textbox).toBeInTheDocument(); + expect(screen.getByRole('textbox')).toBeInTheDocument(); }); - const textbox = document.querySelector('textarea')!; + const textbox = screen.getByRole('textbox'); fireEvent.change(textbox, {target: {value: 'New description'}});frontend/apps/thunder-develop/src/features/organization-units/api/__tests__/useGetOrganizationUnits.test.ts (2)
238-246: Consider usinginitialPropspattern for cleaner re-render testing.The current approach using
let paramsreassignment relies on closure behavior. While this works, the idiomatic pattern usinginitialPropsmakes the test's intent more explicit:♻️ Suggested refactor using initialProps pattern
- let params = {limit: 10, offset: 0}; - const {result, rerender} = renderHook(() => useGetOrganizationUnits(params)); + const {result, rerender} = renderHook( + (props: {limit: number; offset: number}) => useGetOrganizationUnits(props), + {initialProps: {limit: 10, offset: 0}}, + ); await waitFor(() => { expect(result.current.data).toEqual(mockOrganizationUnitList); }); - params = {limit: 10, offset: 10}; - rerender(); + rerender({limit: 10, offset: 10});
248-253:as unknowncast appears inconsistent with other assertions in this file.The
as unknowncast on line 251 is not used in similarexpect.objectContainingassertions elsewhere in this file (e.g., lines 78-82, 91-96, 106-111). If this cast is required for Vitest v4 type compatibility, consider applying it consistently across all similar assertions, or removing it if unnecessary.frontend/apps/thunder-develop/src/features/organization-units/components/edit-organization-unit/__tests__/EditGroups.test.tsx (2)
173-184: Duplicate test coverage with existing test.This test is nearly identical to the test at lines 145-154 (
should render avatars for each group). Both render the component, wait for group names, then query.MuiAvatar-rootand assert the count. Consider removing one of them to avoid redundant coverage.
220-231: Test doesn't meaningfully verify re-render behavior.The comment says "exercise memoization update paths" but the only assertion after rerender checks for static text ("Groups") that doesn't change between renders. Consider asserting that the component reflects the new prop or that the hook is called with the updated ID.
frontend/apps/thunder-develop/src/features/organization-units/api/__tests__/useDeleteOrganizationUnit.test.ts (1)
195-204: Consider restoring the spy after the test.The spy on
queryClient.invalidateQueriesis not explicitly restored. Whilevi.clearAllMocks()inafterEachresets mock call history, it does not restore the original implementation of spied methods. For better test isolation, consider restoring the spy.♻️ Suggested improvement
// Spy on invalidateQueries to make it reject const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries').mockRejectedValue(new Error('Invalidation failed')); result.current.mutate('ou-123'); await waitFor(() => { expect(result.current.isSuccess).toBe(true); }); // Mutation should still succeed despite invalidation failure expect(invalidateSpy).toHaveBeenCalled(); + + // Restore the original implementation + invalidateSpy.mockRestore(); });Alternatively, you can add
vi.restoreAllMocks()to theafterEachblock to restore all spies globally.frontend/apps/thunder-develop/src/components/Navbar/__tests__/NavbarBreadcrumbs.test.tsx (2)
179-209: Consolidate the repeated page cases with a parameterized test.This trims duplication while keeping coverage identical.
♻️ Suggested refactor
- it('renders breadcrumbs for organization-units page', async () => { - const mockUseNavigation = await import('@/layouts/contexts/useNavigation'); - vi.mocked(mockUseNavigation.default).mockReturnValue({ - currentPage: 'organization-units', - setCurrentPage: vi.fn(), - sidebarOpen: false, - setSidebarOpen: vi.fn(), - toggleSidebar: vi.fn(), - }); - - render(<NavbarBreadcrumbs />); - - expect(screen.getByText('Develop')).toBeInTheDocument(); - expect(screen.getByText('Organization Units')).toBeInTheDocument(); - }); - - it('renders breadcrumbs for flows page', async () => { - const mockUseNavigation = await import('@/layouts/contexts/useNavigation'); - vi.mocked(mockUseNavigation.default).mockReturnValue({ - currentPage: 'flows', - setCurrentPage: vi.fn(), - sidebarOpen: false, - setSidebarOpen: vi.fn(), - toggleSidebar: vi.fn(), - }); - - render(<NavbarBreadcrumbs />); - - expect(screen.getByText('Develop')).toBeInTheDocument(); - expect(screen.getByText('Flows')).toBeInTheDocument(); - }); + it.each([ + {currentPage: 'organization-units', label: 'Organization Units'}, + {currentPage: 'flows', label: 'Flows'}, + ])('renders breadcrumbs for $currentPage page', async ({currentPage, label}) => { + const mockUseNavigation = await import('@/layouts/contexts/useNavigation'); + vi.mocked(mockUseNavigation.default).mockReturnValue({ + currentPage, + setCurrentPage: vi.fn(), + sidebarOpen: false, + setSidebarOpen: vi.fn(), + toggleSidebar: vi.fn(), + }); + + render(<NavbarBreadcrumbs />); + + expect(screen.getByText('Develop')).toBeInTheDocument(); + expect(screen.getByText(label)).toBeInTheDocument(); + });
223-282: Avoid relying on MUI-generated class names in assertions.Selectors like
.MuiTypography-body1,.MuiBreadcrumbs-ol,.MuiBox-root, and.MuiBreadcrumbs-separatorare implementation details and may change with MUI upgrades or class name generator tweaks. Prefer semantic queries (role/label/text) or add stable test IDs for structure-specific assertions.frontend/packages/thunder-shared-hooks/package.json (1)
63-64: Explicitly declareplaywrightfor clarity and robustness.While
@vitest/browser-playwrightdeclaresplaywrightas a peer dependency (v1.58.1), it is currently being auto-resolved by pnpm. For clarity and to avoid potential issues with stricter peer dependency configurations, add"playwright": "catalog:"todevDependencies. Note:playwrightis already available in the lock file and the build currently works.🔧 Proposed addition
"@vitest/browser-playwright": "catalog:", + "playwright": "catalog:", "vitest": "catalog:"Also add
playwrightto the workspace catalog infrontend/pnpm-workspace.yamlif not already present for version consistency.
frontend/apps/thunder-develop/src/components/Header/__tests__/LanguageSwitcher.test.tsx
Outdated
Show resolved
Hide resolved
...ons/components/edit-application/token-settings/__tests__/TokenUserAttributesSection.test.tsx
Outdated
Show resolved
Hide resolved
...pps/thunder-develop/src/features/applications/pages/__tests__/ApplicationCreatePage.test.tsx
Outdated
Show resolved
Hide resolved
.../apps/thunder-develop/src/features/applications/pages/__tests__/ApplicationEditPage.test.tsx
Outdated
Show resolved
Hide resolved
.../features/organization-units/components/edit-organization-unit/__tests__/EditGroups.test.tsx
Outdated
Show resolved
Hide resolved
DonOmalVindula
force-pushed
the
migrate-vitest-v4
branch
2 times, most recently
from
86069b3 to
ca82712
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@frontend/apps/thunder-develop/src/features/applications/components/edit-application/integration-guides/__tests__/TechnologyGuide.test.tsx`:
- Around line 344-347: The test uses an ineffective regex (/\ .js$/) to assert
no filename is rendered; update the assertion to directly check for the specific
filename text that would appear (e.g.,
expect(screen.queryByText('index.js')).not.toBeInTheDocument()) or assert the
filename container is absent (e.g.,
expect(screen.queryByTestId('filename')).not.toBeInTheDocument()). Replace the
current screen.queryByText(/\.js$/) check and keep the existing codeBlocks
assertion unchanged.
In
`@frontend/apps/thunder-develop/src/features/applications/pages/__tests__/ApplicationEditPage.test.tsx`:
- Around line 556-583: The test "should open logo modal when edit button on
avatar is clicked" currently falls back to clicking the avatar if the avatar
edit button isn't found; change it to assert the edit button exists so the
edit-button path is always exercised: locate the avatar container (using
getByRole('img') and avatarSection?.querySelectorAll('button')), add an explicit
assertion that buttonsInAvatarSection exists and has length > 0 (fail the test
if not), then perform await user.click(buttonsInAvatarSection[0]) and waitFor
the 'logo-update-modal' to be visible; remove the fallback branch that clicks
the avatar directly.
In
`@frontend/apps/thunder-develop/src/hooks/__tests__/useDataGridLocaleText.test.tsx`:
- Around line 323-351: The test fails because import.meta.env.DEV is false
during Vitest runs so the DEV-only console.warn in useDataGridLocaleText never
runs; fix by either (A) making the test run in DEV by mocking the env before
rendering the hook (e.g., in the test for useDataGridLocaleText, set
import.meta.env.DEV to true via a test-time mock/stub so the console.warn path
in useDataGridLocaleText executes and your consoleSpy assertion passes) or (B)
avoid relying on DEV-only behavior by removing the console warning assertion
from the test (or instead assert the non-function result only). Locate the
failing test in useDataGridLocaleText.test.tsx and adjust the setup around
renderHook/useDataGridLocaleText accordingly or update vite.test define config
to set import.meta.env.DEV to 'true' if you prefer a global change.
🧹 Nitpick comments (19)
frontend/apps/thunder-develop/src/features/applications/components/edit-application/token-settings/__tests__/TokenValidationSection.test.tsx (1)
316-324: Duplicate test case.This test duplicates the existing test at lines 113-121 ("should render helper text when no error"), which already verifies that the default hint text is displayed when there's no error for the
sharedtoken type.Consider removing this duplicate or, if the intent is to group all hint/error-related assertions together, removing the original test from the "Rendering" block instead.
♻️ Suggested removal
describe('Error States', () => { // ... error tests ... - it('should display default hint when no error', () => { - render( - <TestWrapper> - {({control, errors}) => <TokenValidationSection control={control} errors={errors} tokenType="shared" />} - </TestWrapper>, - ); - - expect(screen.getByText('Token validity period in seconds (e.g., 3600 for 1 hour)')).toBeInTheDocument(); - }); });frontend/apps/thunder-develop/src/features/applications/components/edit-application/advanced-settings/__tests__/OAuth2ConfigSection.test.tsx (1)
303-327: Consider adding explicit assertions for the absence of chips.The comments on lines 313 and 326 indicate that no chips should be rendered, but there are no explicit assertions to verify this. Adding assertions would make the tests more robust.
💡 Suggested assertions
render(<OAuth2ConfigSection oauth2Config={oauth2Config} />); expect(screen.getByText('applications:edit.advanced.labels.grantTypes')).toBeInTheDocument(); - // No chips should be rendered for grant types + // Verify no grant type chips are rendered + expect(screen.queryByText('authorization_code')).not.toBeInTheDocument(); }); it('should handle undefined response_types gracefully', () => { const oauth2Config = { grant_types: ['authorization_code'], pkce_required: false, public_client: false, } as OAuth2Config; render(<OAuth2ConfigSection oauth2Config={oauth2Config} />); expect(screen.getByText('applications:edit.advanced.labels.responseTypes')).toBeInTheDocument(); - // No chips should be rendered for response types + // Verify no response type chips are rendered (only the grant type chip should exist) + expect(screen.queryByText('code')).not.toBeInTheDocument(); + expect(screen.queryByText('token')).not.toBeInTheDocument(); });frontend/apps/thunder-gate/src/__tests__/components/AcceptInvite/AcceptInviteBox.test.tsx (2)
1370-1394: Test name is misleading - consider renaming or adding alignment assertion.The test is named "renders with branding enabled and centered text alignment" but doesn't verify text alignment. It only checks that the text is rendered. Consider either:
- Renaming to "renders TEXT component with branding enabled"
- Adding an assertion that verifies the alignment (e.g., checking the style or class)
1396-1429: Consider adding assertions for logo attributes to match test names.These tests verify that the component renders with various branding configurations, but don't assert on the specific attributes mentioned in the test names (alt fallback, custom alt/height/width). While they're valuable as smoke tests, consider adding assertions like:
const logo = screen.getByRole('img'); expect(logo).toHaveAttribute('alt', 'Custom Alt'); expect(logo).toHaveAttribute('height', '50');This would make the tests more specific and aligned with their names.
frontend/apps/thunder-develop/src/components/Header/__tests__/LanguageSwitcher.test.tsx (1)
395-402: Test may be affected by mock state from previous test.This test relies on the default mock implementation, but the previous test (line 377) sets
availableLanguages: []. Sincevi.clearAllMocks()doesn't reset implementations, this test may run with an empty languages array.However, this test only checks that the button exists (not the menu content), so it would still pass. Consider adding an explicit mock reset or explicit mock setup here for test isolation and clarity.
♻️ Suggested improvement for test isolation
it('should preserve button after re-render', () => { + mockUseLanguage.mockReturnValue({ + currentLanguage: 'en-US' as const, + availableLanguages: [ + {code: 'en-US' as const, name: 'English (United States)', nativeName: 'English (US)', direction: 'ltr' as const}, + ], + setLanguage: vi.fn(), + }); + const {rerender} = render(<LanguageSwitcher />); rerender(<LanguageSwitcher />); const button = screen.getByRole('button', {name: /change language/i}); expect(button).toBeInTheDocument(); });frontend/apps/thunder-develop/src/components/Header/__tests__/Search.test.tsx (1)
81-110: Tests rely on MUI internal class names which may change.These tests use selectors like
.MuiOutlinedInput-root,.MuiInputAdornment-positionStart,.MuiInputBase-sizeSmall, and.MuiFormControl-root. While functional, these are implementation details that could break if MUI changes its class naming convention.Consider whether these structural tests add value beyond the existing behavioral tests (lines 24-79) that already verify the component renders correctly with the expected functionality.
frontend/apps/thunder-develop/src/components/Navbar/__tests__/AppNavbar.test.tsx (2)
180-186: Test name and assertion mismatch.The test is named "renders with gradient background styling" but only asserts the
borderRadiusproperty. WhileborderRadius: '999px'is part of the component's styling, it doesn't verify the gradient background. Consider either:
- Renaming the test to match what it actually verifies (e.g., "renders with circular border radius")
- Adding an assertion for the gradient background if that's the intended behavior to test
✏️ Suggested rename for accuracy
- it('renders with gradient background styling', () => { + it('renders with circular border radius styling', () => { const {container} = render(<CustomIcon />); const box = container.querySelector('.MuiBox-root'); expect(box).toBeInTheDocument(); expect(box).toHaveStyle({borderRadius: '999px'}); });
132-136: Consider consolidating redundant test.This test for rendering
SideMenuMobileis redundant since the same assertion (screen.getByTestId('side-menu-mobile')) is already made in the "opens mobile menu when menu button is clicked" test (line 91) and "closes mobile menu when toggleDrawer is called with false" test (line 107).frontend/apps/thunder-develop/src/features/applications/components/__tests__/ApplicationsList.test.tsx (3)
495-525: Test may not validate what it claims.The
mockRejectedValueOncesetup is semantically incorrect foruseNavigate. React Router'snavigatefunction is synchronous and doesn't return a Promise, so setting up a rejected Promise doesn't actually test error handling in a meaningful way. The test passes because the component never awaits the result.If the intent is to test component resilience, consider either:
- Removing this test if navigation errors aren't a realistic scenario
- Testing a scenario where the navigate function throws synchronously instead
♻️ Suggested alternative if testing synchronous throw
it('should handle navigation error gracefully', async () => { const user = userEvent.setup(); - const navigationError = new Error('Navigation failed'); - mockNavigate.mockRejectedValueOnce(navigationError); + mockNavigate.mockImplementationOnce(() => { + throw new Error('Navigation failed'); + });
527-543: Assertion doesn't verify the claimed behavior.The comment states "The onError handler should set src to empty string," but the assertion only checks that the element remains in the document. This would pass regardless of whether the error handling actually works.
♻️ Suggested fix for stronger assertion
if (avatars[0]) { avatars[0].dispatchEvent(new Event('error')); - // The onError handler should set src to empty string - expect(avatars[0]).toBeInTheDocument(); + // Verify the onError handler sets src to empty string + expect(avatars[0]).toHaveAttribute('src', ''); }
359-368: Conditional guards may silently skip assertions.The
if (rows[1])check means if the row doesn't exist (due to a bug or mock change), the test passes without validating navigation behavior. Consider asserting the row exists before clicking.♻️ Suggested fix
const rows = screen.getAllByRole('row'); - // Click on first data row (index 1, as 0 is the header) - if (rows[1]) { - await user.click(rows[1]); - - await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith('/applications/app-1'); - }); - } + // Click on first data row (index 0 since mock DataGrid has no header row) + expect(rows[0]).toBeDefined(); + await user.click(rows[0]); + + await waitFor(() => { + expect(mockNavigate).toHaveBeenCalledWith('/applications/app-1'); + });frontend/apps/thunder-develop/src/features/applications/components/edit-application/token-settings/__tests__/TokenUserAttributesSection.test.tsx (1)
446-465: Consider adding assertions for accordion state changes.The test clicks the accordion button twice but doesn't verify any state changes. Currently, it only confirms the button exists and can be clicked without throwing errors.
If testing MUI accordion expanded/collapsed state is challenging (as noted in line 300-302 comments), consider at minimum asserting that the default attributes (e.g., 'aud', 'exp') are visible/hidden after toggling, or add a comment explaining the test's limited scope.
frontend/apps/thunder-develop/src/features/applications/components/edit-application/integration-guides/__tests__/TechnologyGuide.test.tsx (3)
228-347: Duplicate tests with the "Code Block Language Mapping" describe block.The tests added in the "Code Blocks" describe block (lines 229-347) duplicate tests already present in the "Code Block Language Mapping" describe block (lines 661-783):
| Lines 229-251 | duplicates | Lines 662-684 |
| Lines 253-276 | duplicates | Lines 686-709 |
| Lines 278-299 | duplicates | Lines 711-733 |
| Lines 301-322 | duplicates | Lines 735-757 |
| Lines 324-347 | duplicates | Lines 759-782 |Consider removing one set of these duplicate tests to reduce maintenance overhead and test execution time.
229-251: Assertion does not verify the actual language mapping.The test claims to verify that
terminalmaps tobash, but the assertion only checks that a<pre>element exists. It doesn't verify the language prop passed toSyntaxHighlighter.To properly test the mapping, you could either:
- Mock
SyntaxHighlighterand assert thelanguageprop- Check for a class or attribute set by the highlighter that indicates the language
638-657: Near-duplicate of the preceding test.This test is nearly identical to "should handle fallback failure gracefully for code" (lines 623-637). Both tests:
- Reject the clipboard API
- Make
execCommandthrow- Click the copy button
The only difference is the use of
waitForand the assertion target. The comment says it tests "the catch handler in onClick" but doesn't actually verify the catch handler behavior (e.g., thatlogger.errorwas called).If the intent is to verify the logger error path, consider:
Proposed improvement to verify actual catch behavior
it('should trigger the catch handler in onClick when handleCopyCode rejects', async () => { + const loggerErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); // Make both clipboard API and fallback fail to ensure the promise rejects mockWriteText.mockRejectedValue(new Error('Clipboard API failed')); const mockExecCommand = vi.fn().mockImplementation(() => { throw new Error('execCommand failed'); }); document.execCommand = mockExecCommand; renderWithProviders(<TechnologyGuide guides={mockIntegrationGuides} templateId="react" />); const copyCodeButton = screen.getByTestId('copy-code-button-1'); fireEvent.click(copyCodeButton); - // Wait for the async operation to complete - the catch handler should log error await waitFor(() => { - expect(mockExecCommand).toHaveBeenCalledWith('copy'); + // Verify the error was logged (actual catch handler behavior) + expect(loggerErrorSpy).toHaveBeenCalled(); }); + + loggerErrorSpy.mockRestore(); });If
logger.errorfrom@thunder/loggeris used (per the relevant code snippet), spy on that instead.frontend/apps/thunder-develop/src/features/organization-units/components/edit-organization-unit/__tests__/EditGeneralSettings.test.tsx (1)
262-290: Strengthen assertions to actually validate the cleared-timeout behavior.Right now this test passes even if the earlier timeout is not cleared, because it only checks copy calls and total count. Add a mid-interval assertion so the “Copied” state doesn’t reset early.
Suggested test strengthening
await act(async () => { fireEvent.click(idCopyButton); }); expect(mockClipboard.writeText).toHaveBeenCalledWith('ou-123'); + expect(idCopyButton).toHaveAttribute('aria-label', 'Copied'); - // Advance timers to trigger the reset + // Ensure previous timeout doesn't reset state early + await act(async () => { + vi.advanceTimersByTime(1999); + }); + expect(idCopyButton).toHaveAttribute('aria-label', 'Copied'); + + // Advance timers to trigger the reset await act(async () => { - vi.advanceTimersByTime(2000); + vi.advanceTimersByTime(1); }); + expect(idCopyButton).toHaveAttribute('aria-label', 'Copy'); // Both copies should have been triggered expect(mockClipboard.writeText).toHaveBeenCalledTimes(2);frontend/apps/thunder-develop/src/features/organization-units/components/edit-organization-unit/__tests__/EditUsers.test.tsx (1)
193-197: Test doesn't verify the assertion claimed in its title.The test claims to verify that
organizationUnitIdis passed to the API hook, but it only checks that the UI renders. To properly verify the prop is passed, you should assert that the mock was called with the expected ID.💡 Suggested improvement
it('should pass organizationUnitId to the API hook', () => { renderWithProviders(<EditUsers organizationUnitId="different-ou" />); - expect(screen.getByText('Users')).toBeInTheDocument(); + expect(mockUseGetOrganizationUnitUsers).toHaveBeenCalled(); + expect(screen.getByText('Users')).toBeInTheDocument(); });Note: This requires the mock to track calls. Alternatively, rename the test to better reflect what it actually verifies.
frontend/apps/thunder-develop/src/features/organization-units/components/__tests__/OrganizationUnitDeleteDialog.test.tsx (1)
222-239: Potential duplicate test.This test (
should call onSuccess when provided on successful deletion) appears to duplicate the existing test at lines 106-121 (should call onClose and onSuccess on successful deletion). Both tests verify thatonSuccessandonCloseare called after successful deletion.Consider removing one of these tests or differentiating their purposes more clearly.
frontend/apps/thunder-develop/src/features/organization-units/pages/__tests__/OrganizationUnitEditPage.test.tsx (1)
1068-1074: Non-null assertion on querySelector result.Using the non-null assertion (
!) ondocument.querySelector('textarea')could cause test failures with unclear error messages if the element isn't found. Consider adding an explicit null check or assertion.💡 Suggested improvement
await waitFor(() => { const textbox = document.querySelector('textarea'); expect(textbox).toBeInTheDocument(); }); - const textbox = document.querySelector('textarea')!; + const textbox = document.querySelector('textarea'); + expect(textbox).not.toBeNull(); fireEvent.change(textbox, {target: {value: 'New description'}});
...plications/components/edit-application/integration-guides/__tests__/TechnologyGuide.test.tsx
Outdated
Show resolved
Hide resolved
.../apps/thunder-develop/src/features/applications/pages/__tests__/ApplicationEditPage.test.tsx
Outdated
Show resolved
Hide resolved
frontend/apps/thunder-develop/src/hooks/__tests__/useDataGridLocaleText.test.tsx
Show resolved
Hide resolved
DonOmalVindula
force-pushed
the
migrate-vitest-v4
branch
from
ca82712 to
334caea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@frontend/apps/thunder-develop/src/components/Sidebar/__tests__/MenuContent.test.tsx`:
- Around line 178-309: The tests currently call
vi.mocked(mockUseNavigation.default).mockReturnValue(...) which persists across
tests and can leak state; change those calls in the tests that override
useNavigation (the blocks setting currentPage: 'users' and currentPage: 'flows')
to use mockReturnValueOnce(...) instead, or add a beforeEach that calls
vi.clearAllMocks()/vi.resetAllMocks() to reset the mocked useNavigation between
tests so each test remains isolated (refer to mockUseNavigation.default and
mockReturnValue/mockReturnValueOnce in the test file).
🧹 Nitpick comments (5)
frontend/apps/thunder-develop/src/features/applications/components/edit-application/advanced-settings/__tests__/CertificateSection.test.tsx (1)
305-321: Duplicate test case.This test duplicates the behavior already covered by the existing test
'should prioritize editedApp certificate value over application'(lines 188-204). Both tests verify that when bothapplicationandeditedApphave certificate values, theeditedAppvalue is displayed.Consider removing this test to avoid redundancy in the test suite.
frontend/apps/thunder-develop/src/features/applications/components/create-application/__tests__/ConfigureExperience.test.tsx (2)
375-400: Duplicate tests: These are identical to tests in the "Rendering" block.The tests at lines 376-387 and 389-400 perform the exact same setup and assertions as tests at lines 53-63 and 65-75. Both only verify that a radio button is checked, which doesn't actually test "styling" as the test names suggest.
Consider removing these duplicates or, if the intent is to actually verify visual styling, add assertions that check for specific CSS classes or computed styles on the card elements.
402-442: Test names claim to verify styling but assertions only check text presence.Both tests (
should apply selected styles for user type cardsandshould apply non-selected styles for user type cards) only assert that text is in the document. They don't verify any actual styling differences between selected and non-selected states.To properly test styling, consider asserting on the card's border color or CSS classes that differ based on selection state:
💡 Suggested approach for testing selection styling
- it('should apply selected styles for user type cards', () => { + it('should apply selected styles for user type cards', () => { const mockUserTypes = [ {id: '1', name: 'Internal', ouId: 'INTERNAL', allowSelfRegistration: true}, {id: '2', name: 'External', ouId: 'EXTERNAL', allowSelfRegistration: false}, ]; render( <ConfigureExperience selectedApproach={ApplicationCreateFlowSignInApproach.INBUILT} onApproachChange={mockOnApproachChange} userTypes={mockUserTypes} selectedUserTypes={['Internal']} onUserTypesChange={vi.fn()} />, ); - // User type cards should be rendered with Internal showing as selected - expect(screen.getByText('Internal')).toBeInTheDocument(); - expect(screen.getByText('External')).toBeInTheDocument(); + // Verify Internal card has selected styling (e.g., primary border) + const internalCard = screen.getByText('Internal').closest('[class*="MuiCard"]'); + expect(internalCard).toHaveStyle({ borderColor: expect.stringMatching(/primary/) }); + // Or check for a specific class indicating selection });frontend/apps/thunder-develop/src/features/applications/components/edit-application/token-settings/__tests__/TokenUserAttributesSection.test.tsx (1)
459-478: Consider adding assertions for accordion state changes.The test clicks the accordion button but doesn't verify any observable behavior changes. While Material-UI controls the collapse mechanism, you could assert on
aria-expandedattribute changes or content visibility to make this test more meaningful.Suggested improvement
if (accordionButton) { + // Verify initial expanded state + expect(accordionButton).toHaveAttribute('aria-expanded', 'true'); + // Click to collapse await user.click(accordionButton); + expect(accordionButton).toHaveAttribute('aria-expanded', 'false'); + // Click to expand again await user.click(accordionButton); + expect(accordionButton).toHaveAttribute('aria-expanded', 'true'); }frontend/apps/thunder-develop/src/components/Navbar/__tests__/NavbarBreadcrumbs.test.tsx (1)
211-282: Avoid asserting on MUI internal class names in tests.At lines 211–282, the tests rely on
.MuiTypography-*,.MuiBreadcrumbs-*, and.MuiBox-rootclass selectors. While these are documented API classes stable for theming, they are still implementation details in the context of testing. MUI's official testing guide recommends writing tests using semantic queries (role, aria-label, text) or data-testid instead, to avoid brittleness when internal DOM structures shift.Prefer:
getByRole()with accessible namesgetByLabelText()for labeled elementsgetByText()for text contentgetByTestId()as a last resort for decorative/complex elements♻️ Example refactor for "renders both breadcrumb segments" test
- const typographyElements = container.querySelectorAll('.MuiTypography-root'); - expect(typographyElements.length).toBeGreaterThanOrEqual(2); + const breadcrumb = screen.getByRole('navigation'); + const items = breadcrumb.querySelectorAll('li'); + expect(items.length).toBeGreaterThanOrEqual(2);
frontend/apps/thunder-develop/src/components/Sidebar/__tests__/MenuContent.test.tsx
Outdated
Show resolved
Hide resolved
DonOmalVindula
force-pushed
the
migrate-vitest-v4
branch
3 times, most recently
from
6b82041 to
215f3d3
Compare
DonOmalVindula
force-pushed
the
migrate-vitest-v4
branch
10 times, most recently
from
78e5733 to
913b272
Compare
DonOmalVindula
force-pushed
the
migrate-vitest-v4
branch
from
913b272 to
bd39f2c
Compare
2 participants
Purpose
This pull request significantly expands and improves the test coverage for several UI components in the Thunder Develop frontend, focusing on the Header, Navbar, Sidebar, and related subcomponents. The changes add detailed tests for rendering, structure, icons, interactivity, and memoization, as well as refining test utilities and mock setups for more robust and isolated testing.
Header and Subcomponent Test Enhancements:
Headerfor icon rendering, layout structure, mobile navigation, tooltips, and correct rendering of subcomponents such asLanguageSwitcherandColorSchemeToggle. Tests also verify correct memoization and layout after re-render.Searchcomponent tests to cover outlined variant rendering, adornments, input sizing, form control structure, and preservation of content after re-render.LanguageSwitchertests with cases for selection logic, icon rendering, menu structure, secondary text display, empty language lists, and memoization. Also updated test utilities and mocks for more accurate simulation. [1] [2] [3] [4]Navbar and Breadcrumbs Test Improvements:
AppNavbarto verify rendering ofSideMenuMobile, initial drawer state, app bar styling, and content preservation after re-render.CustomIcontests to include gradient background styling.NavbarBreadcrumbstests to cover different navigation contexts, breadcrumb segment rendering, typography, separator icons, alignment, and memoization.Sidebar and Menu Test Isolation:
MenuContenttests to reset and restore theuseNavigationmock before each test, ensuring isolation and preventing state leakage between tests.Related Issues
@testing-library/*+ Migrate toVitest Browser Modefor browser based testing #1261Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
Tests
Refactor
Chores