Write tests for log in - #190
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a comprehensive Vitest + React Testing Library suite for the LoginPage that mocks authActions.login and react-router's useNavigate, covering static UI, input handling, loading state, success/error flows, form validation, and error message lifecycle. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
client/src/pages/LoginPage/LoginPage.test.tsx (6)
1-20: Mocks are fine; consider explicit factory forauthActionsfor clarityRight now
vi.mock('@/actions/auth')relies on Vitest’s auto-mocking behavior. Defining the mock shape explicitly makes the tests more robust and self-documenting, and avoids surprises if the real module changes:-// Mock dependencies -vi.mock('@/actions/auth'); +// Mock dependencies +vi.mock('@/actions/auth', () => ({ + authActions: { + login: vi.fn(), + }, +}));This also guarantees
authActions.loginis always avi.fn()without depending on module implementation details.
58-67: Minor misleading comment in password typing testThe test itself correctly verifies password field typing, but the inline comment on Line 62 refers to disabling inputs while loading, which doesn’t match this test’s behavior and may confuse future readers.
You can safely drop or update that comment:
- // disable form inputs while loading
69-94: Loading-state test works; consider avoiding real timers in the mockThis test correctly asserts that inputs and the button are disabled and that the loading label appears while the login promise is pending. One small improvement is to avoid the
setTimeout-based implementation in the mock, which introduces a real-time delay and can create state updates after the test finishes.You could instead return a never-resolving promise to keep the component in loading state without delayed updates:
- vi.mocked(authActions.login).mockImplementation(() => - new Promise(resolve => setTimeout(() => resolve({ - success: true, - data: { token: 'fake-token' } as any - }), 100)) - ); + vi.mocked(authActions.login).mockImplementation( + () => new Promise(() => {}) as Promise<{ success: true }> + );This keeps
isLoadingtrue for the duration of the test without side-effectful resolution.
96-114: Test name mentions navigation but only asserts success message + login callThe test currently verifies that the success message appears and
authActions.loginis called with the right credentials, but the description says “and navigates on successful login”.Either:
- Rename the test to match what it actually asserts, or
- Also assert navigation (and possibly reuse
mockNavigate) here, if you prefer to couple success and navigation checks in one place.Given you already have a dedicated navigation test later, renaming is likely the simpler option.
152-160: Empty-fields test only checksrequiredattributes; consider asserting no submitThe test name implies that form submission is prevented, but the assertions only verify the
requiredattributes. To tighten this test and better reflect its description, you could simulate a submit and assert thatauthActions.loginis not called:- it('prevents form submission when fields are empty', () => { - renderLoginPage(); - - const emailInput = screen.getByLabelText('Email') as HTMLInputElement; - const passwordInput = screen.getByLabelText('Password') as HTMLInputElement; - - expect(emailInput).toBeRequired(); - expect(passwordInput).toBeRequired(); - }); + it('prevents form submission when fields are empty', async () => { + const user = userEvent.setup(); + renderLoginPage(); + + const submitButton = screen.getByRole('button', { name: 'Login' }); + await user.click(submitButton); + + expect(authActions.login).not.toHaveBeenCalled(); + });You can keep a separate test for
requiredattributes if you still want that coverage explicitly.
193-215: Navigation test works; consider fake timers to avoid real 1s delayThe test correctly verifies that a successful login eventually calls
mockNavigatewithROUTES.LINEUP_CONSTRUCTOR. Since the component uses a 1-secondsetTimeout, this test currently incurs real-time delay and relies on a 2-secondwaitFortimeout.You can make this faster and more deterministic using fake timers, e.g.:
- it('navigates to team builder after successful login', async () => { - const user = userEvent.setup(); + it('navigates to team builder after successful login', async () => { + vi.useFakeTimers(); + const user = userEvent.setup(); … - await waitFor(() => { - expect(screen.getByText('Login successful!')).toBeInTheDocument(); - }); - - // Wait for setTimeout navigation - await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith(ROUTES.LINEUP_CONSTRUCTOR); - }, { timeout: 2000 }); + await waitFor(() => { + expect(screen.getByText('Login successful!')).toBeInTheDocument(); + }); + + // Fast‑forward the navigation timeout + vi.advanceTimersByTime(1000); + expect(mockNavigate).toHaveBeenCalledWith(ROUTES.LINEUP_CONSTRUCTOR); + vi.useRealTimers();Please double‑check exact fake‑timer APIs (
advanceTimersByTimevs async variants) against your Vitest version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/pages/LoginPage/LoginPage.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/src/pages/LoginPage/LoginPage.test.tsx (3)
client/src/pages/LoginPage/LoginPage.tsx (1)
LoginPage(17-102)client/src/config/constants.ts (1)
ROUTES(9-16)client/src/actions/auth.ts (1)
authActions(3-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (7)
client/src/pages/LoginPage/LoginPage.test.tsx (7)
21-32: Test suite setup and helper renderer look goodThe
beforeEachwithvi.clearAllMocks()plus therenderLoginPagehelper that wraps withBrowserRouterkeeps the tests isolated and avoids boilerplate in each case. No changes needed here.
34-46: Solid coverage of static login UIThis test does a good job asserting key headings, labels, call-to-action text, and the signup link target (
ROUTES.SIGNUP), which will catch most accidental UI regressions in the login shell.
48-56: Email input typing behavior test is clear and sufficientVerifying the email field value after
user.typeis a straightforward, component-level sanity check and aligns with how the field is labeled. Looks good as-is.
116-132: Failed-login error flow test is solidThe mock result and assertion of
'Invalid credentials'match the expected behavior for a failed login. This is a good, minimal coverage of the negative path; no changes needed.
134-150: Generic error behavior for undefined error is well coveredMocking
success: falsewithout anerrorfield and asserting'An error occurred'matches the described fallback behavior. This nicely protects against unexpected backend error shapes.
162-184: Error-clearing behavior between submissions is well testedChaining
mockResolvedValueOncefor two different error messages and asserting that the first error disappears while the second appears ensures the component resets error state correctly on resubmission. This is a nice coverage of subtle UX behavior.
186-191: Autocomplete attributes test is concise and valuableAsserting
autocomplete="email"and"current-password"on the respective fields is a good accessibility/UX check and guards against accidental attribute regression. No changes needed.
|
@copilot review |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.