test(admin/mcp): admin と server/mcp をカバレッジ目標 80% に到達させる (#1037)#1046
Conversation
…#1037) Expand unit tests for admin user management, API errors, AI model actions, and admin API client helpers. Add MCP tool handler contract tests and HTTP transport auth/error coverage.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds many unit tests across admin and server/mcp: admin API client, UI dialogs/hooks/actions, AI-model action tests, users page workflows; and mcp coverage tooling, HTTP authorization/proxy tests, tool helpers, and tool registration/handler tests. ChangesAdmin package test coverage
Server/MCP package test coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly expands test coverage across the admin frontend and the MCP server, adding unit and integration tests for API endpoints, dialogs, hooks, and tool handlers. The reviewer's feedback focuses on improving test robustness and maintainability. Specifically, they recommend refactoring the fragile Select mock in ErrorsContent.test.tsx to use unique keys based on a counter, wrapping fake timer tests in useApiErrors.test.ts with a try...finally block to ensure timers are always restored, and removing redundant helper tests in index.test.ts that are already covered elsewhere.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| import { ErrorsContent } from "./ErrorsContent"; | ||
| import type { ApiErrorRow } from "@/api/admin"; | ||
|
|
||
| const selectCallbacks: Map<string, (v: string) => void> = new Map(); |
There was a problem hiding this comment.
脆弱なテストモックの改善
selectCallbacks のキーに value を使用しているため、複数の Select が同じ値（例：__any__）を持つ場合にコールバックが上書きされる危険性があります。レンダリング順（1番目が status、2番目が severity）に基づいて一意のキーを割り当てるカウンタ変数を導入することを推奨します。
| const selectCallbacks: Map<string, (v: string) => void> = new Map(); | |
| let selectCount = 0; | |
| const selectCallbacks: Map<string, (v: string) => void> = new Map(); |
| Select: ({ | ||
| children, | ||
| value, | ||
| onValueChange, | ||
| }: { | ||
| children: React.ReactNode; | ||
| value?: string; | ||
| onValueChange?: (v: string) => void; | ||
| }) => { | ||
| if (onValueChange && value) { | ||
| selectCallbacks.set(value, onValueChange); | ||
| } | ||
| return <div data-value={value}>{children}</div>; | ||
| }, |
There was a problem hiding this comment.
脆弱なテストモックの改善
レンダリング順（1番目が status、2番目が severity）に基づいて一意のキーを割り当てることで、コールバックの衝突を防ぎます。
Select: ({
children,
value,
onValueChange,
}: {
children: React.ReactNode;
value?: string;
onValueChange?: (v: string) => void;
}) => {
if (onValueChange) {
const key = selectCount === 0 ? "status" : "severity";
selectCallbacks.set(key, onValueChange);
selectCount++;
}
return <div data-value={value}>{children}</div>;
},
| beforeEach(() => { | ||
| selectCallbacks.clear(); | ||
| }); |
There was a problem hiding this comment.
テストごとのカウンタのリセット
各テストの実行前に selectCount をリセットして、テスト間の干渉を防ぎます。
| beforeEach(() => { | |
| selectCallbacks.clear(); | |
| }); | |
| beforeEach(() => { | |
| selectCallbacks.clear(); | |
| selectCount = 0; | |
| }); |
| const statusCallback = selectCallbacks.get("__any__"); | ||
| expect(statusCallback).toBeDefined(); | ||
| React.act(() => { | ||
| statusCallback?.("open"); | ||
| }); | ||
| expect(onStatusFilterChange).toHaveBeenCalledWith("open"); |
There was a problem hiding this comment.
テストでの一意なキーの使用
衝突を避けるため、新しく導入した一意のキー status を使用してコールバックを取得します。
| const statusCallback = selectCallbacks.get("__any__"); | |
| expect(statusCallback).toBeDefined(); | |
| React.act(() => { | |
| statusCallback?.("open"); | |
| }); | |
| expect(onStatusFilterChange).toHaveBeenCalledWith("open"); | |
| const statusCallback = selectCallbacks.get("status"); | |
| expect(statusCallback).toBeDefined(); | |
| React.act(() => { | |
| statusCallback?.("open"); | |
| }); | |
| expect(onStatusFilterChange).toHaveBeenCalledWith("open"); |
| const severityCallback = selectCallbacks.get("__any__"); | ||
| expect(severityCallback).toBeDefined(); | ||
| React.act(() => { | ||
| severityCallback?.("high"); | ||
| }); | ||
| expect(onSeverityFilterChange).toHaveBeenCalledWith("high"); |
There was a problem hiding this comment.
テストでの一意なキーの使用
衝突を避けるため、新しく導入した一意のキー severity を使用してコールバックを取得します。
| const severityCallback = selectCallbacks.get("__any__"); | |
| expect(severityCallback).toBeDefined(); | |
| React.act(() => { | |
| severityCallback?.("high"); | |
| }); | |
| expect(onSeverityFilterChange).toHaveBeenCalledWith("high"); | |
| const severityCallback = selectCallbacks.get("severity"); | |
| expect(severityCallback).toBeDefined(); | |
| React.act(() => { | |
| severityCallback?.("high"); | |
| }); | |
| expect(onSeverityFilterChange).toHaveBeenCalledWith("high"); |
| it("polls via fallback interval when stream is disabled", async () => { | ||
| vi.useFakeTimers(); | ||
| const { unmount } = renderHook(() => useApiErrors({ intervalMs: 1000, enableStream: false })); | ||
| await act(async () => { | ||
| await Promise.resolve(); | ||
| }); | ||
| const callsBefore = vi.mocked(getApiErrors).mock.calls.length; | ||
|
|
||
| await act(async () => { | ||
| await vi.advanceTimersByTimeAsync(1000); | ||
| await Promise.resolve(); | ||
| }); | ||
|
|
||
| expect(vi.mocked(getApiErrors).mock.calls.length).toBeGreaterThan(callsBefore); | ||
| unmount(); | ||
| vi.useRealTimers(); | ||
| }); |
There was a problem hiding this comment.
テスト失敗時のタイマー復元の保証
アサーションが失敗した場合に vi.useRealTimers() が実行されず、他のテストに偽のタイマー状態が引き継がれてしまうのを防ぐため、try...finally ブロックで保護することを推奨します。
it("polls via fallback interval when stream is disabled", async () => {
vi.useFakeTimers();
try {
const { unmount } = renderHook(() => useApiErrors({ intervalMs: 1000, enableStream: false }));
await act(async () => {
await Promise.resolve();
});
const callsBefore = vi.mocked(getApiErrors).mock.calls.length;
await act(async () => {
await vi.advanceTimersByTimeAsync(1000);
await Promise.resolve();
});
expect(vi.mocked(getApiErrors).mock.calls.length).toBeGreaterThan(callsBefore);
unmount();
} finally {
vi.useRealTimers();
}
});| describe("jsonResult / textResult helpers", () => { | ||
| it("jsonResult serializes data as formatted JSON text content", () => { | ||
| const result = jsonResult({ ok: true, count: 2 }); | ||
| expect(result.isError).toBeUndefined(); | ||
| expect(result.content).toEqual([ | ||
| { type: "text", text: JSON.stringify({ ok: true, count: 2 }, null, 2) }, | ||
| ]); | ||
| }); | ||
|
|
||
| it("textResult wraps plain text in a single content item", () => { | ||
| const result = textResult("hello"); | ||
| expect(result).toEqual({ content: [{ type: "text", text: "hello" }] }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
重複テストコードの削除
jsonResult および textResult ヘルパー関数のテストは、すでに helpers.test.ts で詳細に検証されています。冗長なテストコードを削除してメンテナンス性を向上させます。
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1377930f88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await first; | ||
| await second; |
There was a problem hiding this comment.
In the admin CI path I checked, .github/workflows/ci.yml runs the admin production build, and admin/package.json defines that as tsc -b && vite build; because admin/tsconfig.json is strict and includes src, TypeScript cannot prove that first and second are assigned inside the act callback before these awaits, so this new test adds TS2454 errors and blocks the admin build/deploy. Initialize them with a definite value or use a non-null assertion after assignment.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/mcp/src/__tests__/http.test.ts (1)
83-91: ⚡ Quick winAssert “no MCP wiring” for all unauthorized paths.
The missing/malformed auth tests only check
401. Add side-effect assertions (HttpZediClient/createMcpServernot called) there too, so auth short-circuit behavior is fully specified and mutation-resistant.Proposed diff
it("POST /mcp without Authorization returns 401", async () => { const res = await mcpPost(); expect(res.status).toBe(401); + expect(mockHttpZediClient).not.toHaveBeenCalled(); + expect(mockCreateMcpServer).not.toHaveBeenCalled(); }); it("POST /mcp with malformed Bearer returns 401", async () => { const res = await mcpPost({ Authorization: "Basic xyz" }); expect(res.status).toBe(401); + expect(mockHttpZediClient).not.toHaveBeenCalled(); + expect(mockCreateMcpServer).not.toHaveBeenCalled(); });As per coding guidelines, “Prioritize Mutation test score as the first quality indicator,” so these assertions improve spec strength in unauthorized branches.
🤖 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 `@server/mcp/src/__tests__/http.test.ts` around lines 83 - 91, The two tests for missing/malformed auth should also assert that the MCP wiring never runs: after calling mcpPost() (and mcpPost({ Authorization: "Basic xyz" })), add assertions that the mocked HttpZediClient constructor (or factory) and createMcpServer were not invoked (e.g., expect(HttpZediClient).not.toHaveBeenCalled() and expect(createMcpServer).not.toHaveBeenCalled()), so unauthorized requests short-circuit without constructing the client or creating the server.Source: Coding guidelines
server/mcp/src/__tests__/tools/index.test.ts (1)
545-558: ⚡ Quick winRemove duplicate helper tests already covered in helpers.test.ts.
The tests for
jsonResultandtextResultat lines 545-558 duplicate the exact same unit tests already inhelpers.test.tslines 62-69. Both test files verify identical behavior (JSON formatting and text wrapping) with the same assertions. Sincehelpers.test.tsprovides canonical unit test coverage for these helpers, this describe block is redundant.🧹 Proposed fix to remove duplicate tests
-describe("jsonResult / textResult helpers", () => { - it("jsonResult serializes data as formatted JSON text content", () => { - const result = jsonResult({ ok: true, count: 2 }); - expect(result.isError).toBeUndefined(); - expect(result.content).toEqual([ - { type: "text", text: JSON.stringify({ ok: true, count: 2 }, null, 2) }, - ]); - }); - - it("textResult wraps plain text in a single content item", () => { - const result = textResult("hello"); - expect(result).toEqual({ content: [{ type: "text", text: "hello" }] }); - }); -});Note: After removing these tests, the imports
jsonResultandtextResultat line 20 become unused and can also be removed.🤖 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 `@server/mcp/src/__tests__/tools/index.test.ts` around lines 545 - 558, Remove the redundant describe block testing jsonResult and textResult from server/mcp/src/__tests__/tools/index.test.ts (the two it blocks starting with "jsonResult serializes..." and "textResult wraps...") because identical tests exist in helpers.test.ts; delete that entire describe/it block and then remove the now-unused imports jsonResult and textResult (originally imported near the top) to keep imports clean.
🤖 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 `@server/mcp/src/__tests__/http.test.ts`:
- Around line 83-91: The two tests for missing/malformed auth should also assert
that the MCP wiring never runs: after calling mcpPost() (and mcpPost({
Authorization: "Basic xyz" })), add assertions that the mocked HttpZediClient
constructor (or factory) and createMcpServer were not invoked (e.g.,
expect(HttpZediClient).not.toHaveBeenCalled() and
expect(createMcpServer).not.toHaveBeenCalled()), so unauthorized requests
short-circuit without constructing the client or creating the server.
In `@server/mcp/src/__tests__/tools/index.test.ts`:
- Around line 545-558: Remove the redundant describe block testing jsonResult
and textResult from server/mcp/src/__tests__/tools/index.test.ts (the two it
blocks starting with "jsonResult serializes..." and "textResult wraps...")
because identical tests exist in helpers.test.ts; delete that entire describe/it
block and then remove the now-unused imports jsonResult and textResult
(originally imported near the top) to keep imports clean.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f15df7e5-c37d-4afe-8249-74ac1997120f
⛔ Files ignored due to path filters (1)
server/mcp/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
admin/src/api/admin.test.tsadmin/src/pages/ai-models/useAiModelActions.test.tsadmin/src/pages/errors/ErrorDetailDialog.test.tsxadmin/src/pages/errors/ErrorsContent.test.tsxadmin/src/pages/errors/useApiErrors.test.tsadmin/src/pages/users/UsersContent.test.tsxadmin/src/pages/users/index.test.tsxserver/mcp/package.jsonserver/mcp/src/__tests__/http.test.tsserver/mcp/src/__tests__/tools/helpers.test.tsserver/mcp/src/__tests__/tools/index.test.ts
Fix MCP mock constructor types, complete NoteRow fixtures, admin test strict-mode issues, and ErrorsContent select mock collision.
2 participants
概要
Issue #1037 の受け入れ条件に沿い、
adminとserver/mcpのラインカバレッジを 80% 以上に引き上げるため、仕様駆動（/spec-test）の観点で単体テストを拡充しました。破壊的操作（BAN / 削除 / サスペンド）の確認→実行→失敗時通知、MCP ツールの入出力契約、HTTP 認可エラーを重点的に検証しています。変更点
admin/index.test.tsx,UsersContent.test.tsx): デバウンス検索、ロール変更 / サスペンド / 復活 / 削除の成功・失敗、確認ダイアログ分岐、バッジ・ページネーション境界useApiErrors.test.ts,ErrorsContent.test.tsx,ErrorDetailDialog.test.tsx): SSE マージ / フィルタ除外 / ポーリング / レース、フィルタ UI、詳細ダイアログの保存・AI 解析表示useAiModelActions.test.ts):handleSetSystemDefaultの楽観的更新・失敗ロールバック・早期 returnadmin.test.ts):getUsers,patchUserRole,suspendUser,unsuspendUser,getUserImpact,deleteUser,getAuditLogsserver/mcp/tools/index.test.ts): 全ツールの handler 単体テスト（クライアント呼び出し・JSON 応答・API 失敗時isError、スキーマバリデーション）http.test.ts): Bearer 認可成功 / 失敗、トランスポートエラー、server.closeの finallyhelpers.test.ts):jsonResult/textResult@vitest/coverage-v8を devDependency に追加（カバレッジ計測用）カバレッジ結果
adminserver/mcp計測コマンド:
変更の種類
テスト方法
cd admin && bunx vitest run --config vitest.config.ts→ 210 tests passcd server/mcp && bunx vitest run --config vitest.config.ts→ 90 tests pass--coverage付きで Lines ≥ 80% を確認チェックリスト
アサーション強度レビュー（Stryker 対象外ワークスペース）
admin — 総合評価: Adequate
useAiModelActionsuseApiErrorsindex.tsx(Users)UsersContentErrorsContent/ErrorDetailDialogadmin.ts(user/audit)軽微な follow-up（必須ではない）:
onSearchChangeをtoHaveBeenCalledWithで厳密化、systemDefault + toggle 相互作用テスト。server/mcp — 総合評価: Adequate（推定 mutation 70–75%）
expectedPayload未設定（戻り値 BlockStatement ミュータントが生存しうる）wrapToolHandlerの非 Error throw パス本 PR ではカバレッジ目標達成を優先。上記は follow-up issue 候補。
関連 Issue
Closes #1037
Summary by CodeRabbit