feat: added the validation test#1303
Conversation
|
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:
WalkthroughAdds React Testing Library suites validating per-word 30-character name limits, multi-space handling, and recovery for AvatarSelectionStep and AccountSettingsCard; also reformats a single-image SQL query and normalizes HTTPException/whitespace in image routes without behavioral changes. ChangesName Validation Test Coverage
DB and routes formatting normalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 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 |
|
|
1 similar 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: 2
🤖 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 `@frontend/src/pages/__tests__/nameValidation.test.tsx`:
- Around line 60-95: The tests in AccountSettingsCard are asserting the Save
Changes button state without guaranteeing avatar selection, so disabled results
may be due to avatar gating rather than name validation; update the setup or
individual tests to ensure the avatar requirement is satisfied (e.g.,
programmatically select an avatar in setup or render AccountSettingsCard with a
prop/mock indicating an avatar is already chosen) before typing
VALID_30/INVALID_31/ERROR_MSG flows, and in the "recovery" test also assert that
the Save Changes button becomes enabled after correcting the input (use
screen.getByRole('button', { name: /save changes/i }) to check
enabled/disabled). Ensure tests still exercise multi-space and boundary inputs
while avatar selection is fixed so failures reflect name validation only.
- Around line 15-54: The tests assert Next button disabled/enabled but never
provide a selected avatar, causing false positives; update the setup in the
AvatarSelectionStep tests (or the individual tests '31-character word...',
'30-character word...', and 'recovery - valid input after invalid clears error')
to ensure selectedAvatar is set before checking the Next button: either render
AvatarSelectionStep with a valid selectedAvatar prop or simulate selecting an
avatar element (e.g., click the avatar option) after render, then perform the
name typing and assert the Next button state via getByRole('button', { name:
/next/i }); also update the recovery test to re-select an avatar if needed and
assert the Next button becomes enabled after entering a valid name.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd9db29c-b9e4-4e0f-baa5-bc1ee7ae27bb
📒 Files selected for processing (1)
frontend/src/pages/__tests__/nameValidation.test.tsx
…selection and use fireEvent
There was a problem hiding this comment.
🧹 Nitpick comments (3)
frontend/src/pages/__tests__/nameValidation.test.tsx (3)
27-55: 💤 Low valueConsider using
.toBeEnabled()instead of.not.toBeDisabled()for clarity.Lines 31 and 54 assert
expect(...).not.toBeDisabled(), which is semantically less clear thanexpect(...).toBeEnabled(). The positive assertion is easier to read and more directly expresses intent.♻️ Clearer assertion style
test('30-character word is valid - no error shown', () => { const { input } = setup(); fireEvent.change(input, { target: { value: VALID_30 } }); expect(screen.queryByText(ERROR_MSG)).not.toBeInTheDocument(); - expect(screen.getByRole('button', { name: /next/i })).not.toBeDisabled(); + expect(screen.getByRole('button', { name: /next/i })).toBeEnabled(); }); test('recovery - valid input after invalid clears error and re-enables Next', () => { const { input } = setup(); fireEvent.change(input, { target: { value: INVALID_31 } }); expect(screen.getByText(ERROR_MSG)).toBeInTheDocument(); expect(screen.getByRole('button', { name: /next/i })).toBeDisabled(); fireEvent.change(input, { target: { value: 'John' } }); expect(screen.queryByText(ERROR_MSG)).not.toBeInTheDocument(); - expect(screen.getByRole('button', { name: /next/i })).not.toBeDisabled(); + expect(screen.getByRole('button', { name: /next/i })).toBeEnabled(); });🤖 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 `@frontend/src/pages/__tests__/nameValidation.test.tsx` around lines 27 - 55, In the tests "30-character word is valid - no error shown" and "recovery - valid input after invalid clears error and re-enables Next" replace the negative assertion style expect(screen.getByRole('button', { name: /next/i })).not.toBeDisabled() with the clearer positive assertion expect(screen.getByRole('button', { name: /next/i })).toBeEnabled(); update these two assertions in the file's tests to improve readability while keeping the same semantics.
68-104: 💤 Low valueSimplify multi-line button queries for conciseness.
Lines 72–74, 81–83, 96–98, and 101–103 spread button queries across three lines. Extracting the button query to a local constant or using a single line improves readability.
Also consider using
.toBeEnabled()instead of.not.toBeDisabled()on lines 74 and 103 for semantic clarity (same rationale as AvatarSelectionStep tests).♻️ Simplify button assertions
Example for the first test:
test('30 character word is valid - no error shown', () => { const { input } = setup(); fireEvent.change(input, { target: { value: VALID_30 } }); expect(screen.queryByText(ERROR_MSG)).not.toBeInTheDocument(); - expect( - screen.getByRole('button', { name: /save changes/i }), - ).not.toBeDisabled(); + const saveButton = screen.getByRole('button', { name: /save changes/i }); + expect(saveButton).toBeEnabled(); });Apply similar changes to the other tests at lines 81–83, 96–98, and 101–103.
🤖 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 `@frontend/src/pages/__tests__/nameValidation.test.tsx` around lines 68 - 104, The button queries in the tests ("30 character word is valid - no error shown", "31 character word shows error...", "recovery - valid input after invalid clears error and re-enables Save") are split across multiple lines; refactor each to capture the Save Changes button once into a local constant (e.g., const saveButton = screen.getByRole('button', { name: /save changes/i })) and reuse that constant for assertions instead of repeating the multi-line call, and switch assertions that use .not.toBeDisabled() to the clearer .toBeEnabled() (keep ERROR_MSG, setup(), and the existing input changes intact).
1-9: ⚡ Quick winPrefer
userEventoverfireEventfor more realistic user interactions.React Testing Library best practices recommend using
userEvent(from@testing-library/user-event) instead offireEventbecause it more closely simulates real user behavior—for example,userEvent.clickfires mousedown, focus, mouseup, and click events in sequence, whereasfireEvent.clickdispatches only a click event. This improves test reliability and catches more edge cases.As per coding guidelines, "Ensure that test code is automated, comprehensive, and follows testing best practices."
♻️ Refactor to use userEvent
-import { render, screen, fireEvent } from '`@/test-utils`'; +import { render, screen } from '`@/test-utils`'; +import userEvent from '`@testing-library/user-event`'; import { AvatarSelectionStep } from '`@/components/OnboardingSteps/AvatarSelectionStep`'; import AccountSettingsCard from '`@/pages/SettingsPage/components/AccountSettingsCard`';Then update test setup and assertions to use
await user.click(...)andawait user.type(...)orawait user.clear()+await user.type(...)with async test functions. Example for AvatarSelectionStep setup:describe('Name validation - AvatarSelectionStep', () => { - const setup = () => { + const setup = async () => { + const user = userEvent.setup(); render( <AvatarSelectionStep stepIndex={0} totalSteps={4} currentStepDisplayIndex={0} />, ); - fireEvent.click(screen.getAllByAltText('Avatar')[0]); + await user.click(screen.getAllByAltText('Avatar')[0]); const input = screen.getByPlaceholderText('Enter your name'); - return { input }; + return { user, input }; };And update individual tests to use
await user.type(input, ...)orawait user.clear(input)followed byawait user.type(input, ...).🤖 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 `@frontend/src/pages/__tests__/nameValidation.test.tsx` around lines 1 - 9, Replace fireEvent usage with userEvent: import userEvent from '`@testing-library/user-event`' and create a user instance via userEvent.setup(); change affected tests (those interacting with AvatarSelectionStep, AccountSettingsCard or any test using fireEvent) to be async and replace fireEvent.click/type/clear with await user.click(...), await user.type(...), or await user.clear(...) + await user.type(...); also remove the old fireEvent import and ensure assertions run after awaiting the user actions so DOM updates settle.
🤖 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.
Nitpick comments:
In `@frontend/src/pages/__tests__/nameValidation.test.tsx`:
- Around line 27-55: In the tests "30-character word is valid - no error shown"
and "recovery - valid input after invalid clears error and re-enables Next"
replace the negative assertion style expect(screen.getByRole('button', { name:
/next/i })).not.toBeDisabled() with the clearer positive assertion
expect(screen.getByRole('button', { name: /next/i })).toBeEnabled(); update
these two assertions in the file's tests to improve readability while keeping
the same semantics.
- Around line 68-104: The button queries in the tests ("30 character word is
valid - no error shown", "31 character word shows error...", "recovery - valid
input after invalid clears error and re-enables Save") are split across multiple
lines; refactor each to capture the Save Changes button once into a local
constant (e.g., const saveButton = screen.getByRole('button', { name: /save
changes/i })) and reuse that constant for assertions instead of repeating the
multi-line call, and switch assertions that use .not.toBeDisabled() to the
clearer .toBeEnabled() (keep ERROR_MSG, setup(), and the existing input changes
intact).
- Around line 1-9: Replace fireEvent usage with userEvent: import userEvent from
'`@testing-library/user-event`' and create a user instance via userEvent.setup();
change affected tests (those interacting with AvatarSelectionStep,
AccountSettingsCard or any test using fireEvent) to be async and replace
fireEvent.click/type/clear with await user.click(...), await user.type(...), or
await user.clear(...) + await user.type(...); also remove the old fireEvent
import and ensure assertions run after awaiting the user actions so DOM updates
settle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cba1cbb4-a564-4da3-beec-b3947eba034b
📒 Files selected for processing (3)
backend/app/database/images.pybackend/app/routes/images.pyfrontend/src/pages/__tests__/nameValidation.test.tsx
✅ Files skipped from review due to trivial changes (2)
- backend/app/database/images.py
- backend/app/routes/images.py
There was a problem hiding this comment.
Why make this a separate file in pages/__tests__? Why not add these tests within the SettingsPage.test.tsx file?
I'd recommend you either add this as part of SettingsPage.test.tsx or better yet, add them as component tests rather than keeping them in pages/__tests__ director
There was a problem hiding this comment.
Why make this a separate file in
pages/__tests__? Why not add these tests within theSettingsPage.test.tsxfile? I'd recommend you either add this as part ofSettingsPage.test.tsxor better yet, add them as component tests rather than keeping them inpages/__tests__director
@coderabbitai, I don't think my original concern came across clearly after checking commit 6f6973d. Could you rephrase and expand on the review comment for the contributor, to better explain what was asked for?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find existing test files to understand the structure
fd --type f --extension tsx --extension ts 'test' frontend/src
echo "---"
# Check where AccountSettingsCard lives
fd 'AccountSettingsCard' frontend/src
echo "---"
# Check where AvatarSelectionStep lives
fd 'AvatarSelectionStep' frontend/src
echo "---"
# Check SettingsPage.test.tsx content
fd 'SettingsPage.test.tsx' frontend/src --exec cat {}Length of output: 5408
@rohan-pandeyy, here's an expanded version of the concern for the contributor:
Test file placement and structure
The new file nameValidation.test.tsx has two issues worth addressing: where it lives and how it is organized.
1. Wrong directory
pages/__tests__/ is used by this project for broad, page-level sanity checks (see PageSanity.test.tsx and allPages.test.tsx). The two components under test here are:
| Component | Source location |
|---|---|
AvatarSelectionStep |
src/components/OnboardingSteps/AvatarSelectionStep.tsx |
AccountSettingsCard |
src/pages/SettingsPage/components/AccountSettingsCard.tsx |
Neither is a "page" — they are components. All other component-level tests in this project live in src/components/__tests__/ (e.g. SettingsPage.test.tsx, Sidebar.test.tsx, InfoDialog.test.tsx). Placing these tests in pages/__tests__/ breaks that established convention.
2. Two unrelated components in one file
Mixing tests for AvatarSelectionStep and AccountSettingsCard in a single file named after a behavior (nameValidation) makes the test suite harder to navigate. The standard pattern here is one file per component.
Recommended approach (two options, pick one):
-
Option A — component test files (preferred):
src/components/__tests__/AvatarSelectionStep.test.tsx→ holds theAvatarSelectionStepvalidation testssrc/components/__tests__/AccountSettingsCard.test.tsx→ holds theAccountSettingsCardvalidation tests
-
Option B — merge into the existing settings test:
- Keep
AvatarSelectionSteptests in a newAvatarSelectionStep.test.tsxundercomponents/__tests__/ - Move the
AccountSettingsCardname-validation suite into the existingsrc/components/__tests__/SettingsPage.test.tsx, which already renders the fullSettingspage and can reachAccountSettingsCard
- Keep
Option A is cleaner because each file stays focused on one component and is easy to locate by name. Option B is acceptable if you'd prefer to avoid a new file for AccountSettingsCard.
Either way, the current nameValidation.test.tsx in pages/__tests__/ should be removed and replaced by the correctly placed files.
… branch main to current branch
…t/name-validation-per-word
2 participants
Addressed Issues:
Fixes #1288
Screenshots/Recordings:
Screen Shot of Passes test case:
Additional Notes:
This PR adds the unit tests that were deferred from #1286. Tests cover the per-word 30-character name validation logic in both
AvatarSelectionStepandAccountSettingsCardcomponents.The following test cases are covered for each component:
AI Usage Disclosure:
Check one of the checkboxes below:
I have used the following AI models and tools: Claude
Checklist
Summary by CodeRabbit
Tests
Style