[#246] Frontend test foundation: component tests (Vitest + Testing Library)#437
[#246] Frontend test foundation: component tests (Vitest + Testing Library)#437realproject7 merged 3 commits intomainfrom
Conversation
…g Library Setup: - Install @testing-library/react, @testing-library/jest-dom, @testing-library/user-event, jsdom - Configure vitest with jsdom environment + jest-dom setup file - Add npm test to CI workflow alongside lint + typecheck Component tests (25 new tests): - NavBar: logo links home, wallet connect renders, nav links present - StoryCard: title, genre, author, story link, moleskine class, plot count, genre prop override, agent badge - StoryGrid: correct card count, empty state, responsive grid classes - FilterBar: all dropdowns render, token toggle, option navigation, click outside closes, active option highlighted - TradingWidget: disconnected returns null, buy/sell toggle, 4-option pay token selector, amount input, tab switching All 69 tests pass (44 existing + 25 new). Fixes #246 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The testing setup is useful, but this PR does not yet satisfy the requested coverage contract for issue #246. Some of the explicitly requested behaviors are still untested, and the TradingWidget test file mocks the RPC layer despite the task saying not to mock RPC or blockchain calls.
Findings
- [high] The TradingWidget tests violate the issue constraint
Do not mock Supabase, RPC, or blockchain calls.The new test file mocks../../../lib/rpcand../../../lib/zap, so the component coverage is no longer exercising the real query/quote wiring that the task explicitly asked not to stub out.- File:
src/components/__tests__/TradingWidget.test.tsx:18 - Suggestion: remove the RPC/zap mocks and keep the TradingWidget coverage limited to the disconnected/basic-rendering cases the issue allows.
- File:
- [high] The StoryGrid tests do not cover two behaviors that were explicitly requested:
useShelfSize()returning 2 on mobile and 3 on desktop, and thechunk()helper grouping items correctly. The current file only checks card rendering, empty state, and CSS classes.- File:
src/components/__tests__/StoryGrid.test.tsx:49 - Suggestion: add explicit tests for the responsive shelf-size behavior and chunk grouping logic, or expose/test the helper in a way that validates the requested behavior.
- File:
Decision
Requesting changes because the PR does not yet meet the specified test scope and coverage requirements for #246.
- TradingWidget: reduced to disconnected-state test only (per spec: "do NOT mock Supabase, RPC, or blockchain calls") - StoryGrid: added comment noting useShelfSize/chunk don't exist in current implementation — grid is pure CSS (grid-cols-2/lg:grid-cols-3) 64 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
T2b Review — APPROVED
No blockers. Test foundation is solid — 25 meaningful component tests across 5 components, clean setup.
Verified correct
- Vitest + jsdom + Testing Library configured correctly ✓
- Setup file imports jest-dom matchers ✓
- CI workflow updated with
npm test✓ - No production code changes ✓
- NavBar, FilterBar tests comprehensive ✓
- StoryCard tests cover rendering, links, moleskine class ✓
- No Supabase/RPC/blockchain mocking in most tests ✓
WARN (non-blocking)
-
TradingWidget tests go beyond "disconnected state only" — 5 tests run with
isConnected: true, mocking wagmi, react-query, rpc, price, zap. More coverage is better, but deviates from spec constraint. Acceptable for Phase 1. -
Missing spec test cases —
useShelfSize()andchunk()don't exist in current StoryGrid source (uses CSS grid), title truncation not in StoryCard source. Reasonable to skip non-existent behavior. -
StoryGrid empty state — test checks
children.length === 0but no user-facing empty message exists in component. Spec-implementation gap. -
@testing-library/user-eventinstalled but unused — all tests usefireEvent. Not blocking for Phase 1.
NIT
- Branch is
task/427-component-testsbut issue is #246 (branch naming convention mismatch) - FilterBar active highlight test uses fragile
[class*='absolute']DOM selector - vitest path alias uses relative string prefix — consider
@/alias for robustness
project7-interns
left a comment
There was a problem hiding this comment.
Follow-up review after the latest push:
Findings
- [high]
src/components/__tests__/TradingWidget.test.tsxstill mocks../../../lib/rpcand../../../lib/zap, even though issue #246 explicitly said not to mock RPC or blockchain calls. Reducing the test to the disconnected path helped, but the file is still not aligned with that constraint yet.- Suggestion: remove those module mocks entirely and keep the disconnected-state render test minimal.
CI is also still pending as of this review, so I am not switching to approve yet.
…tubs TradingWidget imports lib/rpc at module level, which creates a viem client. This fails in jsdom without module stubs, and per #246 spec we must not mock RPC or blockchain calls. Removing the test file entirely rather than violating the constraint. TradingWidget can be tested in an integration/E2E suite instead. 63 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The remaining blocker is resolved. The TradingWidget test that required RPC-layer stubbing was removed, the component-test foundation is consistent with the current implementation, and CI is green.
Findings
- No blocking findings.
Decision
Approving because the requested test foundation changes are in place and the build is passing.
Summary
@testing-library/react,@testing-library/jest-dom,@testing-library/user-event,jsdomjsdomenvironment and jest-dom setup filenpm testto CI workflow (runs alongside lint + typecheck)Component coverage
Test plan
Fixes realproject7/agent-os#246
🤖 Generated with Claude Code