test: add MCP config and provider lifecycle test coverage - #31
Merged
Conversation
Add 29 tests across 3 files covering previously untested MCP config resolution, snapshot persistence, translation functions, and provider session lifecycle (recovery, cleanup, isolation). - mcpTranslation.test.ts (11 tests): TOML/JSON generation, merge, sanitization - McpConfig.test.ts (14 tests): config resolution, snapshots, version hashing - lifecycle.integration.test.ts (4 tests): resume cursor recovery, MCP snapshot survival after adapter death, stopSession cleanup, concurrent session isolation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThree new test suites added to verify MCP configuration service behavior, provider session lifecycle management, and translation utilities. Tests cover adapter recovery, snapshot persistence, configuration resolution, teardown correctness, session isolation, and transport normalization. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
size:XL
vouch:trusted
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/server/src/provider/Layers/McpConfig.test.ts (1)
19-24: Consider adding temp directory cleanup for CI environments.While the OS eventually cleans up temp directories, long-running CI systems can accumulate many temp directories. Consider using a test hook to clean up.
Optional: Add cleanup hook
import { afterEach, beforeEach } from "@effect/vitest"; let tempDirs: string[] = []; function makeTempContext() { const cwd = nodeFs.mkdtempSync(path.join(os.tmpdir(), "mcp-cwd-")); const baseDir = nodeFs.mkdtempSync(path.join(os.tmpdir(), "mcp-base-")); const stateDir = path.join(baseDir, "userdata"); tempDirs.push(cwd, baseDir); return { cwd, baseDir, stateDir }; } afterEach(() => { for (const dir of tempDirs) { nodeFs.rmSync(dir, { recursive: true, force: true }); } tempDirs = []; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/McpConfig.test.ts` around lines 19 - 24, Add temp-dir tracking and a cleanup hook so CI doesn't accumulate temp folders: in the test file, import afterEach from "@effect/vitest", introduce a tempDirs: string[] array, update makeTempContext to push cwd and baseDir into tempDirs, and add an afterEach() hook that iterates tempDirs and removes each with nodeFs.rmSync(dir, { recursive: true, force: true }) then clears tempDirs; reference symbols: makeTempContext, tempDirs, afterEach, nodeFs.rmSync.apps/server/integration/lifecycle.integration.test.ts (1)
75-93: Consider adding a timeout to prevent indefinite blocking if event count is incorrect.If the harness produces fewer events than expected,
Queue.takewill block indefinitely. While this is acceptable since tests control event emission, adding a timeout improves debuggability when tests fail.Optional: Add timeout for better test failure messages
return yield* Effect.forEach( Array.from({ length: count }, () => undefined), - () => Queue.take(queue), + () => Queue.take(queue).pipe( + Effect.timeout("5 seconds"), + Effect.catchTag("TimeoutException", () => + Effect.fail(new Error(`Timed out waiting for event ${count}`)) + ) + ), { discard: false }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/integration/lifecycle.integration.test.ts` around lines 75 - 93, The helper collectEventsDuring can block forever when Queue.take waits for missing events; update the implementation so each Queue.take is guarded by a timeout (e.g., use Effect.timeoutFail or Effect.raceWith) and fail with a clear error including the expected count and which take timed out; locate the Array.from(...).forEach -> Queue.take(...) call and replace the plain Queue.take with a timed variant, ensuring the test receives a deterministic failure instead of hanging while keeping the same return shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/server/integration/lifecycle.integration.test.ts`:
- Around line 75-93: The helper collectEventsDuring can block forever when
Queue.take waits for missing events; update the implementation so each
Queue.take is guarded by a timeout (e.g., use Effect.timeoutFail or
Effect.raceWith) and fail with a clear error including the expected count and
which take timed out; locate the Array.from(...).forEach -> Queue.take(...) call
and replace the plain Queue.take with a timed variant, ensuring the test
receives a deterministic failure instead of hanging while keeping the same
return shape.
In `@apps/server/src/provider/Layers/McpConfig.test.ts`:
- Around line 19-24: Add temp-dir tracking and a cleanup hook so CI doesn't
accumulate temp folders: in the test file, import afterEach from
"@effect/vitest", introduce a tempDirs: string[] array, update makeTempContext
to push cwd and baseDir into tempDirs, and add an afterEach() hook that iterates
tempDirs and removes each with nodeFs.rmSync(dir, { recursive: true, force: true
}) then clears tempDirs; reference symbols: makeTempContext, tempDirs,
afterEach, nodeFs.rmSync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15f99200-7469-4b6b-aa79-3c53271995c9
📒 Files selected for processing (3)
apps/server/integration/lifecycle.integration.test.tsapps/server/src/provider/Layers/McpConfig.test.tsapps/server/src/provider/mcpTranslation.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mcpTranslation.test.ts(11 tests): TOML/JSON generation, merge-with-marker, server name sanitizationMcpConfig.test.ts(14 tests): config resolution (project, global, override, format normalization), snapshot persistence, version hashinglifecycle.integration.test.ts(4 tests): resume cursor recovery after adapter death, MCP snapshot survival, stopSession cleanup, concurrent session isolationTest plan
bunx vitest run)oxfmt --checkcleanoxlint0 warnings, 0 errorstsc --noEmitclean🤖 Generated with Claude Code
Summary by CodeRabbit