Test for location input on /settings page. #1204
Conversation
|
@petercr is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
WalkthroughThe changes in this pull request enhance the end-to-end test suite for the authenticated settings page in the application. Modifications include the addition of new tests for the username and location input fields, which verify their visibility, validation, and persistence of values. The tests ensure that the functionalities align with the expected behaviors outlined in the linked issue. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip You can disable the changed files summary in the walkthrough.Disable the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
e2e/settings.spec.ts (2)
43-43: Improve test descriptionThe current test name "location input is visible" doesn't fully describe all the test cases within. Consider a more comprehensive description.
-test('location input is visible', async ({page}) => { +test('location input field validation and persistence', async ({page}) => {
42-58: Add accessibility testsAs mentioned in the PR objectives, accessibility testing is required. Consider adding tests to verify:
- Keyboard navigation
- ARIA labels and roles
- Focus management
Example test to add:
test('location input meets accessibility requirements', async ({page}) => { await page.goto('http://localhost:3000/settings'); const locationInput = page.locator('#location'); // Test keyboard navigation await page.keyboard.press('Tab'); await expect(locationInput).toBeFocused(); // Test ARIA labels await expect(locationInput).toHaveAttribute('aria-label', 'Location'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
e2e/settings.spec.ts(2 hunks)
🔇 Additional comments (2)
e2e/settings.spec.ts (2)
1-1: LGTM!
The import statement correctly includes both test and expect from Playwright.
51-52: Review autocomplete attribute value
The autocomplete value 'country-name' might not be appropriate for a location field that accepts cities or general locations. Consider using 'address-level1' or 'address-level2' depending on the expected input.
e2e/settings.spec.ts
Outdated
| await inputField.fill('45&p^x#@!96%*()'); | ||
| await page.locator('button[type="submit"]').click(); | ||
| const errorMessage = page.locator('p:text-is("Username can only contain alphanumerics and dashes.")') | ||
| await expect(errorMessage).toBeVisible(); | ||
| await expect(errorMessage).toHaveText('Username can only contain alphanumerics and dashes.'); |
There was a problem hiding this comment.
Simplify error message assertion
The error message assertion is duplicated. Also, consider testing additional validation rules like minimum/maximum length constraints.
-const errorMessage = page.locator('p:text-is("Username can only contain alphanumerics and dashes.")')
-await expect(errorMessage).toBeVisible();
-await expect(errorMessage).toHaveText('Username can only contain alphanumerics and dashes.');
+const errorMessage = page.getByText('Username can only contain alphanumerics and dashes.');
+await expect(errorMessage).toBeVisible();
+// Add tests for length constraints
+await inputField.fill('a'); // Too short
+await page.locator('button[type="submit"]').click();
+await expect(page.getByText('Username must be at least 3 characters')).toBeVisible();
There was a problem hiding this comment.
@petercr this seems like good points, could you add this changes? :) thanks!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@CarolinaCobo I think I already did this on PR #1203
That PR handled the username input and testing.
But let me know if you want me to add anything there 👌
This one #1204 is focusing on the #location input.
I have tested for max length and its error, but there is no minimum length for location currently.
CarolinaCobo
had a problem deploying
to
production
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
e2e/settings.spec.ts (1)
63-69: Add comprehensive validation tests for location input.The current validation only tests the maximum length. Consider adding more validation cases for better coverage.
Add these additional test cases:
// Add validation tests for location + // Test empty state + await page.locator('input[id="location"]').fill(""); + await page.locator('button[type="submit"]').click(); + await expect(page.getByText("Location is required")).toBeVisible(); + + // Test trimming + await page.locator('input[id="location"]').fill(" New York "); + await page.locator('button[type="submit"]').click(); + await expect(page.locator('input[id="location"]')).toHaveValue("New York"); + + // Test special characters + await page.locator('input[id="location"]').fill("New York!@#$%"); + await page.locator('button[type="submit"]').click(); + await expect(page.getByText("Location contains invalid characters")).toBeVisible(); + // Test maximum length await page.locator('input[id="location"]').fill("a".repeat(101));
e2e/settings.spec.ts
Outdated
| await page.goto("http://localhost:3000/settings"); | ||
|
|
||
| // Test to see if input is visible | ||
| await page.locator('input[id="location"]').isVisible(); |
There was a problem hiding this comment.
Fix incorrect usage of isVisible() method.
The isVisible() method returns a Promise that needs to be properly handled with an assertion.
Replace with:
-await page.locator('input[id="location"]').isVisible();
+await expect(page.locator('input[id="location"]')).toBeVisible();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await page.locator('input[id="location"]').isVisible(); | |
| await expect(page.locator('input[id="location"]')).toBeVisible(); |
e2e/settings.spec.ts
Outdated
| // Test to see if change in location persists after submit and page reload | ||
| await page.fill('input[id="location"]', "A fun place to visit."); | ||
| await page.locator('button[type="submit"]').click(); | ||
| await expect(page.locator('input[id="location"]')).toHaveValue( | ||
| "A fun place to visit.", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Add missing page reload in persistence test.
The test comment mentions verifying persistence after page reload, but the actual reload step is missing.
Add the reload step:
await page.fill('input[id="location"]', "A fun place to visit.");
await page.locator('button[type="submit"]').click();
await expect(page.locator('input[id="location"]')).toHaveValue(
"A fun place to visit.",
);
+ // Verify persistence after reload
+ await page.reload();
+ await expect(page.locator('input[id="location"]')).toHaveValue(
+ "A fun place to visit.",
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Test to see if change in location persists after submit and page reload | |
| await page.fill('input[id="location"]', "A fun place to visit."); | |
| await page.locator('button[type="submit"]').click(); | |
| await expect(page.locator('input[id="location"]')).toHaveValue( | |
| "A fun place to visit.", | |
| ); | |
| }); | |
| // Test to see if change in location persists after submit and page reload | |
| await page.fill('input[id="location"]', "A fun place to visit."); | |
| await page.locator('button[type="submit"]').click(); | |
| await expect(page.locator('input[id="location"]')).toHaveValue( | |
| "A fun place to visit.", | |
| ); | |
| // Verify persistence after reload | |
| await page.reload(); | |
| await expect(page.locator('input[id="location"]')).toHaveValue( | |
| "A fun place to visit.", | |
| ); | |
| }); |
✨ Codu Pull Request 💻
Fixes #1168
Part 5
Pull Request details
Added UI tests for the
#locationinput on the settings page:Any Breaking changes
None
Associated Screenshots
None
What gif best describes this PR or how it makes you feel
This is me trying to finish all my PRs last minute! 😂