Upgrade & fix e2e tests #281
Conversation
WalkthroughAdds a local Claude settings file, bumps Playwright test dependency, and updates E2E tests with minor selector/assertion and formatting tweaks. No production code or public APIs changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
automation/e2e/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
.claude/settings.local.json(1 hunks)automation/e2e/package.json(1 hunks)automation/e2e/tests/data-api.spec.ts(1 hunks)automation/e2e/tests/onboarding.spec.ts(1 hunks)automation/e2e/tests/webring.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/automation/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Use modular organization in automation directories
Files:
automation/e2e/tests/onboarding.spec.tsautomation/e2e/package.jsonautomation/e2e/tests/webring.spec.tsautomation/e2e/tests/data-api.spec.ts
🔇 Additional comments (6)
automation/e2e/package.json (1)
7-7: LGTM! Playwright upgrade to v1.56.0.The upgrade from v1.26.1 to v1.56.0 is substantial, and the test updates in this PR (improved selectors with
getByRole, formatting consistency) align well with modern Playwright best practices. Based on learnings, v1.56.0 includes new inspection APIs and agent features, but no breaking changes affecting the existing test patterns.Based on learnings.
automation/e2e/tests/onboarding.spec.ts (2)
1-1: LGTM! Import order standardization.The import order change improves consistency across the test suite.
6-6: Excellent improvement to role-based selector.Switching from
locator("text=...")togetByRole("button", { name: "..." })is a best practice that:
- Improves test semantics and accessibility awareness
- Makes the test more resilient to DOM structure changes
- Better aligns with how users interact with the page
automation/e2e/tests/data-api.spec.ts (2)
1-1: LGTM! Import order standardization.Consistent with the import order changes across the test suite.
3-3: Excellent fix! Test name now matches functionality.The old name "First-time guide" was misleading for a test that validates API data shape. The new name "API shape" accurately describes what the test does.
automation/e2e/tests/webring.spec.ts (1)
1-91: LGTM! Formatting and style consistency improvements.All changes in this file are non-functional formatting improvements:
- Standardized import order
- Consistent semicolon usage
- Trailing comma formatting
These changes improve code consistency across the test suite without affecting test behavior.
| { | ||
| "permissions": { | ||
| "allow": [ | ||
| "Bash(gh pr checks:*)", | ||
| "Bash(gh pr diff:*)", | ||
| "Bash(gh pr view:*)", | ||
| "Bash(gh issue view:*)" | ||
| ] | ||
| }, | ||
| "enableAllProjectMcpServers": false | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify whether this "local" config file should be committed.
The filename includes .local which typically indicates environment-specific configuration that shouldn't be committed to version control. However, this file is being added to the repository.
Please confirm:
- Should this be a shared configuration (if so, consider renaming to remove
.local) - Or should it be added to
.gitignorefor local-only use
🏁 Script executed:
#!/bin/bash
# Check if there's a .gitignore entry for .claude/settings.local.json
rg -n '\.claude.*settings.*local' .gitignore || echo "No .gitignore entry found for this file"
# Check if there's a template or example settings file
fd -t f 'settings.*json' .claude/Length of output: 67
Remove environment-specific config from version control
.claude/settings.local.json contains local-only settings and shouldn’t be tracked. Either rename it to a shared-config filename if it’s meant to be versioned, or add .claude/settings.local.json to .gitignore and remove it from the repo.
🤖 Prompt for AI Agents
In .claude/settings.local.json around lines 1 to 11, this file contains
local-only environment settings that should not be tracked; either rename it to
a shared config filename if it must be versioned or remove it from the repo and
add .claude/settings.local.json to .gitignore. To fix: decide whether the
settings are meant to be shared—if yes, move/rename into a committed shared
config with safe defaults and update references; if not, delete the committed
file, add .claude/settings.local.json to .gitignore, and commit the removal so
local settings remain untracked.
| blurhash: expect.any(String), | ||
| backlink: expect.anything(), | ||
| description: expect.any(String), | ||
| // description: expect.any(String), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Clarify why description field is commented instead of removed.
The description field assertion is commented out rather than removed. This could indicate:
- The API no longer provides this field
- The field is now optional/nullable
- A temporary workaround for a failing assertion
Please clarify the reasoning and consider either removing the line entirely or documenting why it's kept as a comment.
🏁 Script executed:
#!/bin/bash
# Check the actual API response to verify if description field exists
curl -s "https://wonderfulsoftware.github.io/webring-site-data/data.json" | jq '.["dt.in.th"] | has("description")'
# Also check if description exists in the data structure
curl -s "https://wonderfulsoftware.github.io/webring-site-data/data.json" | jq '.["dt.in.th"].description // "NOT_FOUND"'Length of output: 16
Update test to assert nullable description
The API returns a description key that’s explicitly null (exists but value is null). On line 13 of automation/e2e/tests/data-api.spec.ts, replace the commented-out expect.any(String) with an assertion such as:
- // description: expect.any(String),
+ expect(data.description).toBeNull(); // or use a matcher accepting string|nullCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In automation/e2e/tests/data-api.spec.ts around line 13, the test currently has
a commented-out expect.any(String) for the description field but the API returns
the key explicitly with a null value; update the assertion to assert that
description is nullable by replacing the commented expect.any(String) with an
assertion that allows null (for example using expect(description).toBeNull() or
expect(description).toBeOneOf([null, expect.any(String)]) depending on test
style) so the test accepts an explicit null value while still validating
presence of the key.
dosubot
bot
added
the
size:L
Summary by CodeRabbit
Chores
Tests
Style