Frontend: integrate Vitest and React Testing Library architecture#615
Open
Xploit-Ghost wants to merge 5 commits into
Open
Frontend: integrate Vitest and React Testing Library architecture#615Xploit-Ghost wants to merge 5 commits into
Xploit-Ghost wants to merge 5 commits into
Conversation
Added instructions for running frontend tests using Vitest.
Contributor
|
Warning Review limit reached
More reviews will be available in 26 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements frontend unit testing infrastructure for the React components using Vitest and React Testing Library. It adds a new ChangesFrontend Unit Testing Suite
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thank you @Xploit-Ghost for your contribution. Please make sure your PR follows https://github.com/GitMetricsLab/github_tracker/blob/main/CONTRIBUTING.md#-pull-request-guidelines
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/components/Navbar.test.tsx (3)
35-42: ⚡ Quick winStrengthen the theme toggle assertion.
The test only verifies that at least one toggle button exists (
length > 0), but the comment states there should be two (desktop + mobile). Use a specific assertion.♻️ Proposed fix
it('renders the correct theme toggle icon based on context', () => { // Render with dark mode renderWithProviders(<Navbar />, 'dark'); const toggleButtons = screen.getAllByLabelText('Toggle Theme'); - // Check that the toggle buttons are present (one for desktop, one for mobile) - expect(toggleButtons.length).toBeGreaterThan(0); + // Check that there are exactly two toggle buttons (one for desktop, one for mobile) + expect(toggleButtons).toHaveLength(2); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Navbar.test.tsx` around lines 35 - 42, The test for Navbar's theme toggle uses a weak assertion (expect(toggleButtons.length).toBeGreaterThan(0)); update it to assert exactly two toggles are rendered (desktop + mobile) by replacing that check with expect(toggleButtons).toHaveLength(2). Locate the check in the test that calls renderWithProviders(<Navbar />, 'dark') and uses screen.getAllByLabelText('Toggle Theme'); ensure the assertion uses toHaveLength(2) and optionally add a second assertion that each button displays the expected icon/text for the 'dark' context if you want to validate icon correctness.
8-18: ⚡ Quick winReturn
mockToggleThemefrom the helper to enable interaction verification.The
mockToggleThemefunction is created but not returned, preventing tests from verifying that theme toggle interactions actually calltoggleTheme. Return it alongside the render result.♻️ Proposed enhancement
-const renderWithProviders = (ui: React.ReactElement, themeMode: 'light' | 'dark' = 'light') => { +const renderWithProviders = (ui: React.ReactElement, themeMode: 'light' | 'dark' = 'light') => { const mockToggleTheme = vi.fn(); - return render( + const renderResult = render( <ThemeContext.Provider value={{ mode: themeMode, toggleTheme: mockToggleTheme }}> <MemoryRouter> {ui} </MemoryRouter> </ThemeContext.Provider> ); + + return { ...renderResult, mockToggleTheme }; };Then tests can verify toggle behavior:
it('calls toggleTheme when theme toggle button is clicked', () => { const { mockToggleTheme } = renderWithProviders(<Navbar />); const toggleButton = screen.getAllByLabelText('Toggle Theme')[0]; fireEvent.click(toggleButton); expect(mockToggleTheme).toHaveBeenCalledTimes(1); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Navbar.test.tsx` around lines 8 - 18, The helper renderWithProviders creates mockToggleTheme but does not return it, preventing tests from asserting toggle calls; update renderWithProviders to return the render result together with mockToggleTheme (e.g., return an object containing the result of render(...) and the mockToggleTheme) so callers of renderWithProviders can destructure { mockToggleTheme } and assert calls to toggleTheme when interacting with the Navbar.
44-53: ⚡ Quick winUse a more robust assertion for mobile menu visibility.
Counting "Home" text occurrences is fragile and could break if the desktop navigation changes. Consider querying for a mobile-specific container, role, or data-testid instead.
♻️ Alternative approach
If the mobile menu has a specific container or role:
it('opens mobile menu when hamburger icon is clicked', () => { renderWithProviders(<Navbar />); const menuButton = screen.getByLabelText('Toggle Menu'); // Verify menu is initially not visible (adjust selector based on actual implementation) expect(screen.queryByRole('navigation', { name: /mobile/i })).not.toBeInTheDocument(); fireEvent.click(menuButton); // Verify mobile menu is now visible expect(screen.getByRole('navigation', { name: /mobile/i })).toBeInTheDocument(); });Or if the Navbar component accepts adding data-testid:
// In the component, add data-testid="mobile-menu" to the mobile menu container expect(screen.getByTestId('mobile-menu')).toBeVisible();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Navbar.test.tsx` around lines 44 - 53, The test in Navbar.test.tsx is fragile because it counts 'Home' occurrences; update the test for Navbar to assert the mobile menu visibility using a mobile-specific selector (e.g., a role or data-testid) instead of text counts: renderWithProviders(<Navbar />), assert the mobile nav container (e.g., role="navigation" with accessible name containing "mobile" or data-testid="mobile-menu") is initially not present/visible, fireEvent.click on the menu toggle (label 'Toggle Menu'), then assert that getByRole/getByTestId for the mobile menu container is now present and visible; modify the Navbar component to add a data-testid="mobile-menu" or an accessible name on the mobile navigation container if needed.package.json (1)
11-12: ⚡ Quick winClarify the distinction between
testandtest:clientscripts.Both scripts execute the same command (
vitest), creating ambiguity about which to use. Consider either:
- Updating the
testscript to run both frontend and backend tests (e.g.,"test": "npm run test:client && npm run test:backend"), OR- Removing the
testscript entirely iftest:clientandtest:backendare sufficient.♻️ Proposed fix (Option 1: run both test suites)
- "test": "vitest", + "test": "npm run test:client && npm run test:backend", "test:client": "vitest",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 11 - 12, The package.json currently has duplicate scripts ("test" and "test:client") both running vitest; update the "test" script to run both frontend and backend suites by chaining the existing "test:client" and a new/expected "test:backend" (e.g., make "test" invoke "npm run test:client" and "npm run test:backend"), and if "test:backend" doesn't exist add it to run the backend test runner; alternatively, if you prefer separate commands remove the redundant "test" script—adjust the "test", "test:client", and "test:backend" entries accordingly.src/components/ActivityFeed.test.tsx (1)
16-55: ⚡ Quick winAdd test coverage for fetch error scenarios.
The test suite covers loading, success, and empty results, but doesn't verify error handling when
fetchrejects (network failures, API errors, etc.). IfActivityFeedimplements error handling, it should be tested.🧪 Suggested error handling test
it('displays an error message when fetch fails', async () => { vi.mocked(global.fetch).mockRejectedValueOnce(new Error('Network error')); render(<ActivityFeed username="testuser" />); await waitFor(() => { expect(screen.queryByText('Loading...')).not.toBeInTheDocument(); }); // Adjust the expected error message based on ActivityFeed's actual error UI expect(screen.getByText(/error|failed/i)).toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ActivityFeed.test.tsx` around lines 16 - 55, Add a new test to ActivityFeed.test.tsx that asserts the component shows an error UI when fetch rejects: mock global.fetch to reject (e.g., mockRejectedValueOnce(new Error('Network error'))), render <ActivityFeed username="testuser" />, wait for the loading indicator to disappear with waitFor, and then assert that the error message element (match by text pattern like /error|failed/i or whatever ActivityFeed renders on errors) is present; place it alongside the existing tests so the suite covers the fetch failure path for the ActivityFeed component.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/ActivityFeed.test.tsx`:
- Line 22: The tests currently cast global.fetch to any (e.g., (global.fetch as
any).mockResolvedValueOnce(...)) and return a minimal object lacking standard
Response fields; replace these with a properly typed mock by using
vi.mocked(global.fetch) (or cast to MockedFunction) and mockResolvedValueOnce
with a full Response-like object containing ok, status, statusText and json:
async () => <payload>; update each occurrence referenced (the mocks around
ActivityFeed.test.tsx lines where fetch is mocked) to return this typed mock so
ESLint/type checks pass and components relying on response.ok/status behave
correctly.
- Line 5: The test replaces global.fetch with vi.fn() but never restores it,
causing leaks; capture the original fetch (e.g., const originalFetch =
global.fetch) before mocking and add an afterEach or afterAll hook in
ActivityFeed.test.tsx that restores global.fetch = originalFetch and restores
mocks (vi.restoreAllMocks() or similar) so other tests aren't affected.
---
Nitpick comments:
In `@package.json`:
- Around line 11-12: The package.json currently has duplicate scripts ("test"
and "test:client") both running vitest; update the "test" script to run both
frontend and backend suites by chaining the existing "test:client" and a
new/expected "test:backend" (e.g., make "test" invoke "npm run test:client" and
"npm run test:backend"), and if "test:backend" doesn't exist add it to run the
backend test runner; alternatively, if you prefer separate commands remove the
redundant "test" script—adjust the "test", "test:client", and "test:backend"
entries accordingly.
In `@src/components/ActivityFeed.test.tsx`:
- Around line 16-55: Add a new test to ActivityFeed.test.tsx that asserts the
component shows an error UI when fetch rejects: mock global.fetch to reject
(e.g., mockRejectedValueOnce(new Error('Network error'))), render <ActivityFeed
username="testuser" />, wait for the loading indicator to disappear with
waitFor, and then assert that the error message element (match by text pattern
like /error|failed/i or whatever ActivityFeed renders on errors) is present;
place it alongside the existing tests so the suite covers the fetch failure path
for the ActivityFeed component.
In `@src/components/Navbar.test.tsx`:
- Around line 35-42: The test for Navbar's theme toggle uses a weak assertion
(expect(toggleButtons.length).toBeGreaterThan(0)); update it to assert exactly
two toggles are rendered (desktop + mobile) by replacing that check with
expect(toggleButtons).toHaveLength(2). Locate the check in the test that calls
renderWithProviders(<Navbar />, 'dark') and uses
screen.getAllByLabelText('Toggle Theme'); ensure the assertion uses
toHaveLength(2) and optionally add a second assertion that each button displays
the expected icon/text for the 'dark' context if you want to validate icon
correctness.
- Around line 8-18: The helper renderWithProviders creates mockToggleTheme but
does not return it, preventing tests from asserting toggle calls; update
renderWithProviders to return the render result together with mockToggleTheme
(e.g., return an object containing the result of render(...) and the
mockToggleTheme) so callers of renderWithProviders can destructure {
mockToggleTheme } and assert calls to toggleTheme when interacting with the
Navbar.
- Around line 44-53: The test in Navbar.test.tsx is fragile because it counts
'Home' occurrences; update the test for Navbar to assert the mobile menu
visibility using a mobile-specific selector (e.g., a role or data-testid)
instead of text counts: renderWithProviders(<Navbar />), assert the mobile nav
container (e.g., role="navigation" with accessible name containing "mobile" or
data-testid="mobile-menu") is initially not present/visible, fireEvent.click on
the menu toggle (label 'Toggle Menu'), then assert that getByRole/getByTestId
for the mobile menu container is now present and visible; modify the Navbar
component to add a data-testid="mobile-menu" or an accessible name on the mobile
navigation container if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4eeeb1ba-a4fc-4b54-beac-5731e39a1ce4
📒 Files selected for processing (4)
README.mdpackage.jsonsrc/components/ActivityFeed.test.tsxsrc/components/Navbar.test.tsx
Refactor fetch mocking in ActivityFeed tests to use a helper function for creating mock responses and ensure original fetch is restored after tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces the foundational testing suite for the React/Vite frontend using Vitest and React Testing Library, addressing the frontend UI reliability requirements outlined in #613.
Changes Made
test:clientscript topackage.jsonfor unified frontend test execution.Navbarcomponent (handlingThemeContextandMemoryRouterwrapping) and theActivityFeedcomponent (mocking the globalfetchAPI for asynchronous state testing).README.mdwith the new frontend testing commands to parallel the existing backend documentation.Related Issue
Resolves #613
Summary by CodeRabbit
Documentation
Tests
Chores
npm run test:clientscript to run frontend tests.