feat(testing-lib) Implementing a testing suite for MCP and Evals - #69
Conversation
- Created package directory structure - Added package.json with dependencies and peer dependencies - Configured TypeScript, tsup, and Vitest - Added README with package overview Completed tasks: T001-T006
- Created shared types and interfaces (types.ts) - Implemented error types (TestingError, ConnectionError, TimeoutError, etc.) - Added debug logger using debug package convention - Created debug module index - Set up main package index with type exports Completed tasks: T007-T011
- Implemented TestClient wrapper around MCP SDK - Added startTestServer for App instances and external processes - Implemented ServerStartupError with diagnostic details - Created server module index exports - Added unit and contract tests (stubs for now, to be expanded) Completed tasks: T012-T019
- Implemented expectToolResult standalone matcher with all assertions - Added toMatchObject, toMatchSchema, toHaveNoError, toHaveError, toContainText - Implemented defineTestSuite for test case definition - Implemented runTestSuite for executing test suites - Created behavior and eval module indexes - Added comprehensive unit, contract, and integration tests Completed tasks: T020-T031
- Implemented framework-agnostic core matchers - Added setupVitestMatchers and setupJestMatchers adapters - Added TypeScript type augmentations for both frameworks - Created matchers module index - Added comprehensive unit and integration tests Completed tasks: T032-T039
- Implemented generators wrapper around fast-check with lazy loading - Added generators.fromSchema for Zod schema generation - Implemented forAllInputs property test runner - Added PropertyFailureError with shrunk input details - Created property module index - Added unit and integration tests with graceful handling of missing dependencies Completed tasks: T040-T046
- Implemented createMockHost wrapping MockAdapter from @mcp-apps-kit/ui - Added emitToolResult, setTheme, emitToolCancelled methods - Added getToolCallHistory and clearHistory for assertions - Implemented createTestEnvironment coordinating server and client - Implemented TestEnvironmentBuilder with fluent API - Created ui module index - Added unit tests with graceful handling of missing dependencies Completed tasks: T047-T054
- Implemented LLMProvider interface - Added OpenAI and Anthropic providers with lazy loading - Implemented createLLMEvaluator factory - Added env var validation for API keys - Implemented built-in criteria (accuracy, relevance, safety, completeness) - Added evaluateWithPrompt for custom evaluation - Created llm module index - Added unit tests with graceful handling of missing dependencies Completed tasks: T055-T063
- Updated main index.ts with all exports - Verified subpath exports for /vitest and /jest in package.json - Wrote comprehensive README with examples - Fixed build errors (exported extractResultData, fixed type imports) - Build verified and working - Package already included in monorepo workspace (packages/*) Note: T069 (quickstart validation) and T070 (full test suite) require runtime dependencies and can be validated during integration testing. Completed tasks: T064-T068
- Fixed import paths in contract/integration tests (wrong relative depth) - Fixed zod-fast-check version (^0.10.1 instead of ^1.0.0) - Improved error handling in test-server test to prevent unhandled errors - All 62 tests now passing across 14 test files
- Created comprehensive test suite for minimal example - Tests for v1 and v2 greet tools - Versioning tests - Integration tests - Fixed TestClient to use client.request() with CallToolResultSchema - Updated to extract structuredContent from MCP SDK results Note: Some tests still failing - need to verify structuredContent extraction
- Fixed TestClient to properly use client.request() with CallToolResultSchema to access structuredContent from MCP SDK responses - Fixed type issues for lazy-loaded modules (fast-check, zod-fast-check, mock) - Added @types/debug for proper TypeScript support - Fixed type import path for MCP SDK Client - Fixed vitest adapter type augmentation - Simplified minimal example tests to be more robust - All 83 tests passing (62 in testing package + 21 in minimal example)
- Add comprehensive advanced-features.test.ts covering: - defineTestSuite / runTestSuite (declarative test definitions) - Suite hooks (beforeEach/afterEach) - Suite skip flag - Property-based testing with built-in generators - All assertion methods (toHaveNoError, toContainText, toMatchSchema, toMatchObject) - Client options (history tracking, listTools, timeout) - Remove zod-fast-check dependency (incompatible with Zod v4) - Remove generators.fromSchema() function - Add fast-check as devDependency for property testing - All 98 tests passing (62 in testing package + 36 in minimal example)
- Add `port` and `version` options to TestEnvironmentOptions - Update createTestEnvironment to support versioned apps (e.g., /v1/mcp) - Add TestEnvironmentBuilder.withPort() and .withVersion() methods - Update advanced-features.test.ts to use createTestEnvironment - Add tests for createTestEnvironment and TestEnvironmentBuilder - All 103 tests passing (62 in testing package + 41 in minimal example)
- Implement standalone MockHost that works with or without @mcp-apps-kit/ui - Add MockHost methods: setTheme, getTheme, simulateToolCall - Add event handlers: onToolResult, onTeardown, onToolCancelled - Track tool call history with timestamps - Add comprehensive tests for all MockHost functionality - All 112 tests passing (62 in testing package + 50 in minimal example)
- Add UI component tests for GreetingWidgetV1 and GreetingWidgetV2 - Test rendering, user interactions, tool calls, and error handling - Mock @mcp-apps-kit/ui-react hooks and @mcp-apps-kit/ui exports - Add @testing-library/react, @testing-library/jest-dom, jsdom dependencies - Configure jsdom environment via @vitest-environment directive - All 64 tests passing in minimal example
- Add table of contents and structured sections - Document all major features with code examples - Add Test Environment section (createTestEnvironment, builder, manual setup) - Add Assertions & Matchers section with all available matchers - Add Test Suites section with hooks and skip flags - Add Property-Based Testing section with generators - Add UI Widget Testing section with @testing-library/react - Add Mock Host Environment section - Add LLM Evaluation section with criteria - Add Framework Integration section (Vitest/Jest) - Add complete API Reference tables - Follow style from @mcp-apps-kit/core README
- Add peerDependenciesMeta to mark fast-check, openai, anthropic, vitest, and jest as optional - Update README install section with clearer table format - Clarify that optional deps won't be auto-installed
- Move fast-check, openai, @anthropic-ai/sdk to dependencies - Users no longer need to install these separately - Keep vitest/jest as optional peers (user chooses test framework) - Simplify README install section
- Change validation condition to ensure the response text includes the input name and is not an error. - Remove unused Anthropic client initialization code to streamline the provider implementation.
- Fix property test predicate in advanced-features.test.ts to properly check for errors and parse JSON response - Change integration.test.ts to use truly sequential calls instead of Promise.all - Move versioning.test.ts to tests/integration/ directory with updated imports - Various testing library improvements
- Add TypeScript files from the testing package to ESLint configuration for linting. - Update test files to improve formatting and consistency, including removing unnecessary whitespace and ensuring proper indentation. - Refactor test assertions for better readability and maintainability.
- Introduce optional stop method in the App interface to allow graceful server shutdown. - Update startTestServerFromApp function to track if app.start() was used and call app.stop() if available during server stop. - Ensure proper handling of dynamic and fixed port scenarios for server shutdown.
- Update test files to utilize structuredContent for typed data assertions, improving reliability and readability. - Refactor property tests to filter out whitespace-only strings and ensure proper input validation. - Introduce lazy loading for fast-check generators, allowing for better performance and compatibility with ESM. - Improve error handling in property tests to provide detailed failure information, enhancing debugging capabilities. - Update README documentation to reflect changes in testing utilities and usage examples.
- Mark @anthropic-ai/sdk, fast-check, and openai as optional peer dependencies in package.json. - Revise README to clarify optional dependencies and their installation instructions. - Introduce new resource and prompt matchers for improved testing capabilities. - Implement lazy loading for fast-check and LLM evaluation modules to optimize performance. - Expand test client functionality with methods for listing and retrieving prompts.
- Add checks for empty or missing content in Anthropic responses to prevent runtime errors. - Update error messages for clarity regarding unexpected response types from Anthropic. - Implement timeout cleanup in startTestServerFromApp to prevent memory leaks. - Refactor lazy-loader to allow retrying of loading on failure and improve handling of concurrent calls.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a new testing workspace package Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Env as TestEnvironmentBuilder
participant Server as Test Server
participant Client as Test Client
participant App as MCP App
Test->>Env: build() / createTestEnvironment(opts)
Env->>Server: startTestServer(app or external)
Server->>App: app.start({port, transport:'http'}) / spawn cmd
Server-->>Env: TestServer{url,mcpUrl,stop}
Env->>Client: createTestClient(mcpUrl, options)
Client-->>Env: TestClient{callTool, listTools, history, disconnect}
Env-->>Test: {server, client, cleanup}
Test->>Client: callTool('greet', {name:'Alice'})
Client->>Server: HTTP POST /mcp (CallTool)
Server->>App: route to tool implementation
App-->>Server: ToolResult
Server-->>Client: ToolResult
Client-->>Test: ToolResult (with structuredContent)
Test->>Test: expectToolResult(result).toMatchSchema(...)
Test->>Env: cleanup()
Env->>Client: disconnect()
Env->>Server: stop()
sequenceDiagram
participant Suite as Test Runner
participant Client as Test Client
participant Tool as App/tool
participant Matchers as expectToolResult
Suite->>Suite: iterate TestCases (skip/only)
Suite->>Client: callTool(toolName, input)
Client->>Tool: invoke tool
Tool-->>Client: return ToolResult
Suite->>Matchers: expectToolResult(result)
Matchers-->>Suite: pass / throw (assert)
Suite->>Suite: record case result (time/status)
Suite->>Suite: run afterEach()
Suite-->>Test Runner: aggregated TestSuiteResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Comment |
…versions - Change vitest specifier from ^4.0.0 to ^3 || ^4 to allow compatibility with both major versions.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| */ | ||
|
|
||
| import { describe, it, expect, vi, beforeEach } from "vitest"; | ||
| import { render, screen, waitFor, fireEvent } from "@testing-library/react"; |
| * using mock clients to simulate MCP tool calls. | ||
| */ | ||
|
|
||
| import { describe, it, expect, vi, beforeEach } from "vitest"; |
|
@cursor review |
Code Review - Testing Library PROverviewThis is an impressive and comprehensive addition to the mcp-apps-kit ecosystem! The testing library adds ~9500 lines of well-structured code that provides a complete testing solution for MCP applications. The implementation quality is very high overall. ✅ Strengths1. Excellent Architecture & Design
2. Outstanding Documentation
3. Developer Experience
4. Security Considerations
5. Test Coverage
|
🔍 Issues & RecommendationsHigh Priority1. Strict TypeScript Violations
|
🎯 Adherence to CLAUDE.md Guidelines
🔒 Security AssessmentOverall: GOOD ✅
One consideration: External server startup (test-server.ts:151) passes through all env vars - generally fine for testing but be aware sensitive vars are passed to child processes. 📊 Performance Considerations
Recommendation: Add parallel test execution guidance to README. |
✅ Final VerdictAPPROVE with minor changes ⭐ This is high-quality work that significantly enhances the mcp-apps-kit ecosystem. The architecture is solid, documentation is excellent, and the API design is intuitive. Required Changes:
Recommended Changes:
Nice-to-Have:
Excellent work! This testing library will be a valuable addition to the project. The code quality is high and the developer experience is well thought out. 🎉 Files reviewed: 68 changed files with focus on:
Low Priority Items:
Suggested Future Additions (not blockers):
|
Code Review: Testing Library ImplementationThis is an excellent PR that adds comprehensive testing capabilities to the MCP AppsKit. Overall score: 9.5/10 Key Strengths
Issues Found1. Retry Logic Incomplete (Medium Priority)Location: packages/testing/src/server/test-client.ts:37 Function is named callToolWithRetry but never retries on failure. Either implement the retry logic using the retries option or rename the function. 2. Timeout Memory Leak Risk (Minor)Location: packages/testing/src/server/test-client.ts:47-63 Timeout may not be cleared if promise rejects for reasons other than timeout. Wrap in try/finally to ensure cleanup. 3. Port Collision Risk (Minor)Location: packages/testing/src/server/test-server.ts:40-61 Race condition between finding available port and using it in app.start(). RecommendationsBefore Merge (High Priority):
Post Merge (Medium Priority): ConclusionThis is a production-ready, high-quality addition demonstrating excellent software engineering practices. The few issues identified are minor and don't block merging. Recommendation: APPROVE with minor fixes Great work! This testing library will significantly improve the developer experience for MCP application testing. Reviewed by: Claude Code | 2026-01-07 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In @examples/minimal/tests/greet-v1.test.ts:
- Line 17: Replace the hardcoded await new Promise(setTimeout...) by waiting for
an actual readiness signal: modify the test to either call a startTestServer
variant that resolves only when the server is listening (adjust or implement
startTestServer to resolve after server.listen callback), or poll a readiness
endpoint (e.g., GET /health) until it returns success with a timeout, or add
retry logic around the first client call (with exponential backoff and overall
timeout); update the test in greet-v1.test.ts to use startTestServer (or the new
startTestServerReady) or the polling helper instead of the 100ms sleep so the
test proceeds only once the server is truly ready.
In @examples/minimal/tests/greet-v2.test.ts:
- Around line 14-31: The test uses an arbitrary delay after calling
startTestServer — remove the setTimeout and make startTestServer guarantee
readiness before resolving (update the startTestServer implementation to resolve
only after the server's listen/ready callback completes), then keep the test
using startTestServer(...) and createTestClient(...) without the delay; if you
prefer not to change startTestServer, implement a small waitForServer(url) retry
helper and call await waitForServer(`http://localhost:${testPort}/v2/mcp`) after
startTestServer instead of using setTimeout.
In @examples/minimal/tests/integration.test.ts:
- Around line 1-85: This test file (the one containing describe("Minimal Example
Integration") and importing app) must be relocated into the project's
tests/integration test folder and renamed to follow conventions (e.g.,
minimal-example.test.ts or integration.test.ts); move the file contents into
that new test directory, update any relative imports if they break (the import
of app and testing helpers), and ensure the test runner configuration includes
the tests/integration folder so the moved test is discovered.
In @examples/minimal/tests/integration/versioning.test.ts:
- Line 17: The cast "as unknown" on the app passed to startTestServer defeats
type safety; remove the unnecessary cast and ensure app is typed to match
startTestServer's parameter (App | ExternalServerOptions). Locate the call to
startTestServer (symbol: startTestServer) and the app variable (symbol: app),
then either declare/annotate app as App or adapt the argument to the expected
union (e.g., pass an ExternalServerOptions object or wrap app appropriately) so
the type error is resolved without using "as unknown" before assigning to
mainServer.
In @examples/minimal/tests/ui-widget.test.tsx:
- Line 11: The import list includes an unused symbol "fireEvent" which should be
removed to clean up the test file; update the import statement that currently
imports render, screen, waitFor, fireEvent from "@testing-library/react" to drop
"fireEvent" so only the used imports remain (e.g., render, screen, waitFor).
In @packages/testing/package.json:
- Around line 63-91: Update the overly restrictive peerDependencies in
package.json: change the "openai" constraint from ^4.0.0 to ^6.0.0, "fast-check"
from ^3.0.0 to ^4.0.0, and "jest" from ^29.0.0 to ^30.0.0 while leaving "zod"
(^4.0.0) and "vitest" (^3 || ^4) as-is; ensure these edits are made under the
"peerDependencies" section so consumers can install current major versions.
In @packages/testing/src/adapters/jest.ts:
- Around line 7-16: Replace the dynamic type import for ZodType with the
standard direct type import used elsewhere: change the line declaring `type
ZodType = import("zod").ZodType;` to a direct type import `import type { ZodType
} from "zod";`, ensuring the `ZodType` symbol is imported in the same style as
other matcher/type files to maintain consistency.
In @packages/testing/src/eval/behavior/runner.ts:
- Around line 61-79: The passed++ is incremented before suite.afterEach runs
inside the finally, so if suite.afterEach throws the test is incorrectly counted
as passed; move the passed++ increment to after the finally completes (i.e.,
after running/awaiting suite.afterEach) or wrap the suite.afterEach call in its
own try-catch and only increment passed++ when no errors occur; adjust
references around runTestCase, the try/catch/finally block, the suite.afterEach
invocation, and the passed/failed counters accordingly so passed++ occurs only
after afterEach succeeds.
In @packages/testing/src/eval/llm/evaluator.ts:
- Around line 38-40: The switch default currently throws a generic Error for
unsupported providers (throw new Error(`Unsupported provider:
${config.provider}`)); replace this with the project's custom error type (e.g.,
import and throw ConfigurationError or create/throw UnsupportedProviderError) so
the code uses the consistent error taxonomy; update the import list to include
the chosen error class and throw new ConfigurationError(`Unsupported provider:
${config.provider}`) (or new UnsupportedProviderError(...)) in place of the
generic Error, keeping the message intact to preserve context.
In @packages/testing/src/eval/llm/providers/anthropic.ts:
- Around line 179-183: The inner variable named result in the
parsed.criteria.find callback shadows the outer result parameter of evaluate(),
so rename the inner variable to a non-conflicting name (e.g., parsedCriterion or
parsedItem) inside the options.criteria.map block to avoid shadowing; update
references in the callback and downstream use in the creation of
CriterionResult[] so the logic and types (CriterionResult[],
parsed.criteria.find, options.criteria.map) remain identical but without name
collision.
In @packages/testing/src/utils/lazy-loader.ts:
- Around line 138-147: The pendingPromise created in the lazy loader (the block
that sets pendingPromise = createClient(apiKey).then(...)) lacks rejection
handling so a failed createClient leaves pendingPromise set and subsequent calls
reuse the rejected promise; fix by attaching a .catch handler to that promise
which clears pendingPromise (and also resets cachedClient and cachedApiKey if
appropriate) before rethrowing the error so state is clean for retries; update
the promise chain around createClient, pendingPromise, cachedClient, and
cachedApiKey accordingly.
In @packages/testing/tests/integration/behavior-testing.test.ts:
- Line 8: The import line currently imports beforeEach from vitest but that
symbol is never used and is shadowed by local constants named beforeEach in this
test file; remove beforeEach from the named imports in the vitest import
statement (i.e., change `import { describe, it, expect, vi, beforeEach } from
"vitest";` to exclude beforeEach) so the code no longer imports an unused symbol
while leaving the other imports intact.
In @packages/testing/tests/integration/property-testing.test.ts:
- Around line 13-35: The test currently swallows the absence of fast-check by
returning inside a try-catch; instead detect availability of fast-check up front
and use Vitest's conditional skip (e.g., test.skip or conditionally call it) so
the test is explicitly skipped when missing; specifically, replace the try-catch
around generators.string and forAllInputs with a pre-check (attempt to
require/import fast-check or check that generators and forAllInputs are
present), and if unavailable call the test skip mechanism before executing the
assertions, and apply the same change to the second test that also wraps
fast-check usage (the block using generators.string and forAllInputs).
In @packages/testing/tests/unit/eval/evaluator.test.ts:
- Around line 1-11: The test file must be moved to mirror the source's nested
eval/llm structure under the unit tests (place it in the tests/unit location
matching the src eval llm hierarchy) and then update the module imports so they
resolve from the new relative location (ensure imports referencing
createLLMEvaluator, criteria and ConfigurationError point to the correct
relative paths after the move); keep the test logic unchanged, only relocate the
file and fix the import paths.
🧹 Nitpick comments (27)
examples/minimal/tests/ui-widget.test.tsx (2)
1-251: Reorganize test file to follow subdirectory structure.Per coding guidelines, test files should be organized under
tests/withunit/,integration/, andcontract/subdirectories. This UI widget test file should be moved to an appropriate subdirectory, likelytests/integration/ui-widget.test.tsxsince it tests React components with mocked dependencies.Based on learnings, tests should mirror the source structure within these subdirectories.
176-251: Consider adding comprehensive test coverage for V2 to match V1.The GreetingWidgetV2 test suite has fewer test cases than V1. Consider adding tests for:
- Error handling on greet failure (similar to V1 lines 140-156)
- Cancel button functionality (similar to V1 lines 125-138)
- Unwrapped result format support (similar to V1 lines 76-86)
- Theme application (similar to V1 lines 166-173)
These scenarios would ensure both widgets have equivalent test coverage for shared functionality.
examples/minimal/tests/advanced-features.test.ts (2)
316-337: Property test predicate uses type assertion.The type assertion on line 332 (
result.structuredContent as { message: string } | undefined) is acceptable given the external API, but consider using a type guard or Zod schema validation for stronger type safety in property tests.💡 Optional: Use schema validation for type-safe access
+import { z } from "zod"; + +const greetResultSchema = z.object({ message: z.string() }); + await forAllInputs( generators.string({ minLength: 1, maxLength: 20 }), async (name: string) => { if (name.trim().length === 0) { return true; } const result = await env.client.callTool("greet", { name }); if (result.isError) { return false; } - const data = result.structuredContent as { message: string } | undefined; - return typeof data?.message === "string" && data.message.includes(name); + const parsed = greetResultSchema.safeParse(result.structuredContent); + return parsed.success && parsed.data.message.includes(name); }, { numRuns: 5 } );
391-448: Consider server readiness checks instead of fixed delays.The
setTimeout(resolve, 100)pattern (lines 394, 420, 435) for waiting on server startup can be flaky under load. While acceptable for examples, consider using a readiness check or thecreateTestEnvironmenthelper which handles this internally.packages/testing/tests/unit/utils/lazy-loader.test.ts (1)
103-168: Good test coverage, consider adding failure scenario test.The tests cover the happy paths well. Consider adding a test for when
createClientrejects to verify error handling behavior (especially relevant given the issue identified in the implementation).💡 Optional: Add test for client creation failure
it("should allow retry after client creation fails", async () => { const mockClient = { id: "client1" }; const createClient = vi .fn() .mockRejectedValueOnce(new Error("Network error")) .mockResolvedValueOnce(mockClient); const factory = createCachedClientFactory(createClient); // First call fails await expect(factory.get("api-key-123")).rejects.toThrow("Network error"); // Second call should retry and succeed const result = await factory.get("api-key-123"); expect(result).toBe(mockClient); expect(createClient).toHaveBeenCalledTimes(2); });packages/testing/tests/unit/ui/test-environment.test.ts (1)
22-28: Consider verifying the fluent API actually applies configuration.The test confirms the fluent API returns a builder instance, but doesn't verify that
withServerUrlandwithClientOptionsactually store the configuration. While this is acceptable for a unit test focused on API structure, consider adding assertions or integration tests that verify the built environment reflects the configured values.💡 Enhanced test example
it("should support fluent API", () => { const builder = new TestEnvironmentBuilder() .withServerUrl("http://localhost:3000") .withClientOptions({ timeout: 5000 }); expect(builder).toBeDefined(); + // Could verify internal state or add integration test + // that builds and checks the resulting environment });packages/testing/tests/unit/server/test-client.test.ts (1)
31-53: Strengthen option tests to verify actual TestClient behavior.The current option tests only verify that plain objects can have properties assigned, but they never pass these options to
createTestClientor verify that the options affect behavior. This means bugs where options are silently ignored would not be caught.Consider either:
- Moving these to integration tests where you can verify the options actually work with a test server, or
- Enhancing these tests to verify the options are at least accepted by
createTestClient(even if the full behavior can't be tested without a server)Example: Verify options are accepted
describe("options", () => { - it("should accept trackHistory option", async () => { - // We can't fully test this without a server, but we can verify the option is accepted - const options = { trackHistory: true }; - expect(options.trackHistory).toBe(true); - }); + it("should accept trackHistory option", async () => { + // Verify the function accepts the option without throwing + await expect( + createTestClient("http://localhost:59998/mcp", { trackHistory: true }) + ).rejects.toThrow(ConnectionError); // Expected to fail connection, but option should be accepted + }); - it("should accept timeout option", async () => { - const options = { timeout: 5000 }; - expect(options.timeout).toBe(5000); - }); + it("should accept timeout option", async () => { + await expect( + createTestClient("http://localhost:59998/mcp", { timeout: 5000 }) + ).rejects.toThrow(ConnectionError); + }); - it("should accept retries option", async () => { - const options = { retries: 3 }; - expect(options.retries).toBe(3); - }); + it("should accept retries option", async () => { + await expect( + createTestClient("http://localhost:59998/mcp", { retries: 3 }) + ).rejects.toThrow(ConnectionError); + }); it("should have default timeout of 30000ms", () => { - // Verify the default from types - const defaultOptions = { timeout: 30000 }; - expect(defaultOptions.timeout).toBe(30000); + // This can only be properly verified with a real server in integration tests + // For now, we document the expected default + expect(30000).toBe(30000); }); });packages/testing/tests/contract/test-server.test.ts (1)
20-28: Consider removing empty placeholder tests or converting them to TODOs.These placeholder tests currently don't validate anything and will always pass. Consider either:
- Implementing them as part of this PR (if integration tests are ready)
- Removing them and tracking the work elsewhere (e.g., GitHub issue)
- Using
it.todo()to mark them as pending work♻️ Suggested refactor using Vitest's todo feature
- it("createTestClient should return a TestClient", async () => { - // This test requires a running server - // Will be implemented with integration tests - }); + it.todo("createTestClient should return a TestClient when connected to a running server"); - it("startTestServer should return a TestServer", async () => { - // This test requires @mcp-apps-kit/core or external server - // Will be implemented with integration tests - }); + it.todo("startTestServer should return a TestServer when starting an app or external process");packages/testing/tests/unit/eval/generators.test.ts (1)
11-19: Consider using dynamic import for consistency.The current implementation uses
require()to detect fast-check availability, which is inconsistent with the ESM pattern used throughout the rest of the codebase (e.g.,await import("zod")in other test files).♻️ Refactor to use dynamic import pattern
// Detect whether fast-check is available let isFastCheckAvailable = false; try { - // eslint-disable-next-line @typescript-eslint/no-require-imports - require("fast-check"); + await import("fast-check"); isFastCheckAvailable = true; } catch { isFastCheckAvailable = false; }Note: This would require wrapping the detection in an async IIFE or moving it into a
beforeAllhook:let isFastCheckAvailable = false; beforeAll(async () => { try { await import("fast-check"); isFastCheckAvailable = true; } catch { isFastCheckAvailable = false; } });packages/testing/tests/unit/eval/evaluator.test.ts (1)
13-31: Consider using Vitest lifecycle hooks for environment isolation.The test directly manipulates
process.env.OPENAI_API_KEYwithin a try-finally block. While the current approach works, using Vitest'sbeforeEach/afterEachhooks would provide better test isolation and make the intent clearer.♻️ Optional refactor using lifecycle hooks
describe("createLLMEvaluator", () => { + let originalKey: string | undefined; + + beforeEach(() => { + originalKey = process.env.OPENAI_API_KEY; + }); + + afterEach(() => { + if (originalKey) { + process.env.OPENAI_API_KEY = originalKey; + } else { + delete process.env.OPENAI_API_KEY; + } + }); + it("should throw ConfigurationError if API key is not set", () => { - // Temporarily remove API key - const originalKey = process.env.OPENAI_API_KEY; delete process.env.OPENAI_API_KEY; - - try { - expect(() => { - createLLMEvaluator({ - provider: "openai", - model: "gpt-4o-mini", - }); - }).toThrow(ConfigurationError); - } finally { - // Restore API key - if (originalKey) { - process.env.OPENAI_API_KEY = originalKey; - } - } + + expect(() => { + createLLMEvaluator({ + provider: "openai", + model: "gpt-4o-mini", + }); + }).toThrow(ConfigurationError); });packages/testing/tests/unit/ui/mock-host.test.ts (1)
8-53: Extract repetitive try-catch pattern into a test helper.All three tests use identical try-catch blocks to handle missing
@mcp-apps-kit/uidependency. This repetition reduces maintainability and clarity. Consider extracting this pattern into a helper function or using Vitest's conditional test features.♻️ Proposed refactor using a helper function
+/** + * Helper to run tests that depend on @mcp-apps-kit/ui + * Skips gracefully if the dependency is not available + */ +function testWithUI(name: string, fn: () => void | Promise<void>) { + it(name, async () => { + try { + await fn(); + } catch (error) { + if (error instanceof Error && error.message.includes("@mcp-apps-kit/ui")) { + // Test skipped - dependency not available + return; + } + throw error; + } + }); +} + describe("createMockHost", () => { - it("should create a mock host with default options", () => { - // This test requires @mcp-apps-kit/ui to be installed - try { - const host = createMockHost(); - expect(host).toBeDefined(); - expect(typeof host.emitToolResult).toBe("function"); - expect(typeof host.getToolCallHistory).toBe("function"); - } catch (error) { - // Skip test if @mcp-apps-kit/ui is not available - if (error instanceof Error && error.message.includes("@mcp-apps-kit/ui")) { - // Test skipped - dependency not available - return; - } - throw error; - } + testWithUI("should create a mock host with default options", () => { + const host = createMockHost(); + expect(host).toBeDefined(); + expect(typeof host.emitToolResult).toBe("function"); + expect(typeof host.getToolCallHistory).toBe("function"); }); - it("should track tool call history", () => { - // This test requires @mcp-apps-kit/ui to be installed - try { - const host = createMockHost(); - const history = host.getToolCallHistory(); - expect(Array.isArray(history)).toBe(true); - } catch (error) { - if (error instanceof Error && error.message.includes("@mcp-apps-kit/ui")) { - return; - } - throw error; - } + testWithUI("should track tool call history", () => { + const host = createMockHost(); + const history = host.getToolCallHistory(); + expect(Array.isArray(history)).toBe(true); }); - it("should clear history", () => { - // This test requires @mcp-apps-kit/ui to be installed - try { - const host = createMockHost(); - host.clearHistory(); - expect(host.getToolCallHistory()).toHaveLength(0); - } catch (error) { - if (error instanceof Error && error.message.includes("@mcp-apps-kit/ui")) { - return; - } - throw error; - } + testWithUI("should clear history", () => { + const host = createMockHost(); + host.clearHistory(); + expect(host.getToolCallHistory()).toHaveLength(0); }); });packages/testing/src/eval/llm/evaluator.ts (1)
29-29: Use theLLMProviderinterface type directly instead ofReturnType.The type annotation
ReturnType<typeof createOpenAIProvider>is indirect and assumes both providers return compatible types. Since bothcreateOpenAIProviderandcreateAnthropicProvidershould implement the sameLLMProviderinterface, use that type explicitly for clarity and to ensure consistency if provider signatures diverge in the future.♻️ Proposed refactor
- let provider: ReturnType<typeof createOpenAIProvider>; + let provider: LLMProvider;You'll need to import the
LLMProvidertype:-import type { LLMEvaluator, LLMEvaluatorConfig } from "../../types"; +import type { LLMEvaluator, LLMEvaluatorConfig, LLMProvider } from "../../types";packages/testing/tests/integration/vitest-integration.test.ts (1)
29-38: Consider static import for Zod.The dynamic import of Zod on line 30 is unusual since Zod is listed as a dependency in the library context. Unless there's a specific reason for lazy-loading (e.g., testing optional dependency behavior), a static import at the top of the file would be more conventional and clearer.
♻️ Proposed refactor (if no lazy-loading needed)
import { describe, it, expect, beforeAll } from "vitest"; import { setupVitestMatchers } from "../../src/adapters/vitest"; import type { ToolResult } from "../../src/types"; +import { z } from "zod"; // ... rest of file ... it("should work with toMatchToolSchema", async () => { - const { z } = await import("zod"); const result: ToolResult = { content: [{ type: "text", text: '{"message":"Hello"}' }], isError: false, }; const schema = z.object({ message: z.string() }); expect(result).toMatchToolSchema(schema); });examples/minimal/tests/greet-v2.test.ts (1)
38-47: ValidatestructuredContentwith a schema instead of type assertion.The type assertion
as { message: string; fullName: string }on line 44 bypasses TypeScript's type safety. Since you're already using Zod in this file (line 64-68), validate the structure with a schema to ensure the data actually conforms to expectations at runtime.♻️ Proposed refactor
Define the schema once at the module level:
+const GreetResponseSchema = z.object({ + message: z.string(), + fullName: z.string(), + timestamp: z.string(), +}); + describe("Greet Tool V2", () => {Then use it to validate instead of asserting:
it("should greet a user by name only", async () => { const result = await env.client.callTool("greet", { name: "Alice" }); expectToolResult(result).toHaveNoError(); - // Use structuredContent for typed data assertions - const data = result.structuredContent as { message: string; fullName: string }; + // Validate and parse structuredContent + const data = GreetResponseSchema.parse(result.structuredContent); expect(data.message).toContain("Alice"); expect(data.fullName).toBe("Alice"); });Apply the same pattern to lines 58, 85, and 91.
examples/minimal/tests/integration.test.ts (2)
16-16: Remove artificial delay from test setup.The 100ms timeout appears unnecessary and slows down test execution. The test client has built-in retry and timeout mechanisms (configured on line 20-21) that should handle connection timing without explicit delays.
⚡ Proposed fix to remove unnecessary delay
const server = await startTestServer(app, { port: testPort }); - await new Promise((resolve) => setTimeout(resolve, 100)); const client = await createTestClient(`http://localhost:${testPort}/v1/mcp`, {
80-80: Avoid type assertion that bypasses type safety.The
asassertion silences TypeScript's validation of required fields. Consider using a test helper that creates invalid inputs explicitly, or useexpect().rejectspattern without the assertion.🔒 Proposed fix using proper error testing
it("should handle errors gracefully", async () => { - // Test with invalid input (missing required field) - try { - await env.client.callTool("greet", {} as { name: string }); - } catch (error) { - expect(error).toBeDefined(); - } + // Test with invalid input (missing required field) + await expect( + env.client.callTool("greet", {}) + ).rejects.toBeDefined(); });packages/testing/src/matchers/resource.ts (1)
156-184: Consider clarifying null/undefined equivalence behavior.Lines 159-161 treat
nullandundefinedas equivalent in matching. While this may be intentional for flexibility, it could mask bugs where the distinction betweennullandundefinedmatters (e.g., explicit null in JSON vs. missing field). Consider documenting this behavior or making it stricter if the distinction is important for your use cases.packages/testing/src/eval/llm/providers/anthropic.ts (1)
194-196: Consider making the overall pass threshold configurable.The overall pass threshold is hardcoded to 0.7, while individual criterion thresholds are configurable via
criterion.threshold. This inconsistency may surprise users who expect to control both thresholds. Consider adding anoverallThresholdoption toEvalOptions.packages/testing/tests/unit/server/test-server.test.ts (1)
129-144: Test doesn't verify actual behavior.This test only validates the options object structure but never calls
startTestServerwith these options. It doesn't actually test that environment variables are passed through to the external command. Consider either:
- Adding a real integration test that verifies env var passthrough
- Removing this test if it's not providing value
- At minimum, documenting why this limited validation is sufficient
packages/testing/src/server/test-client.ts (1)
72-82: Local type shadows imported ContentBlock type.Line 73 defines a local
ContentBlocktype that shadows theContentBlocktype imported from../types(via theToolResulttype). While this works because TypeScript resolves to the local scope, it's confusing to have two differentContentBlocktypes. Consider renaming the local type or using the imported type directly.♻️ Proposed fix
// Build content blocks from the result - type ContentBlock = { type: string; text?: string; data?: string; mimeType?: string }; - const contentBlocks = (result.content ?? []).map((block: ContentBlock) => { + type RawContentBlock = { type: string; text?: string; data?: string; mimeType?: string }; + const contentBlocks = (result.content ?? []).map((block: RawContentBlock) => { if (block.type === "text") { return { type: "text" as const, text: block.text }; }packages/testing/src/eval/behavior/matchers.ts (1)
189-192: Consider documenting the null/undefined equivalence behavior.The function treats
nullandundefinedas equivalent (both match each other). This is reasonable for a flexible "partial match" semantic, but could surprise users expecting strict equality. Consider adding a JSDoc note about this behavior.packages/testing/src/server/test-server.ts (1)
212-228: Potential double-resolve instop()method.If the process exits normally after
SIGTERMbut before the 5-second timeout, both theexitlistener (line 220) and the timeout callback (line 225) may callresolve(). While callingresolve()twice on a Promise is harmless (subsequent calls are ignored), this could be cleaner.♻️ Suggested improvement
async stop(): Promise<void> { serverLogger("Stopping external server"); return new Promise((resolve) => { if (childProcess.killed) { resolve(); return; } + let resolved = false; childProcess.once("exit", () => { + if (!resolved) { + resolved = true; resolve(); + } }); childProcess.kill("SIGTERM"); setTimeout(() => { - if (!childProcess.killed) childProcess.kill("SIGKILL"); - resolve(); + if (!resolved) { + resolved = true; + if (!childProcess.killed) childProcess.kill("SIGKILL"); + resolve(); + } }, 5000); }); },packages/testing/src/ui/test-environment.ts (2)
12-16: Consider extracting the shared App interface.This
Appinterface is duplicated fromtest-server.ts(same fields minusstop). Consider extracting it to a shared location to avoid drift.
50-51: Arbitrary delay may cause flaky tests.The 100ms sleep after
startTestServeris fragile. SincestartTestServeralready handles startup completion via its timeout mechanism, this additional delay shouldn't be necessary. If it is needed, consider increasing the default timeout instartTestServeror using a more robust readiness check.♻️ Consider removing the arbitrary delay
server = await startTestServer(options.app as App, { port }); - - // Wait a bit for server to be ready - await new Promise((resolve) => setTimeout(resolve, 100)); } else if (options.serverUrl) {packages/testing/src/eval/llm/providers/openai.ts (1)
85-88: Consider validating the parsed JSON structure.The response is cast to a specific shape without runtime validation. If the LLM returns valid JSON with an unexpected structure (e.g., missing
criteriaoroverallfields), this will throw cryptic errors at lines 93-94 or 106.♻️ Add defensive checks or Zod validation
// Parse response - const parsed = JSON.parse(content) as { - criteria: Array<{ name: string; score: number; explanation: string }>; - overall: { score: number }; - }; + const parsed = JSON.parse(content) as Record<string, unknown>; + + if (!parsed.criteria || !Array.isArray(parsed.criteria)) { + throw new Error("Invalid response structure: missing criteria array"); + } + if (!parsed.overall || typeof (parsed.overall as Record<string, unknown>).score !== "number") { + throw new Error("Invalid response structure: missing overall.score"); + } + + const typedParsed = parsed as { + criteria: Array<{ name: string; score: number; explanation: string }>; + overall: { score: number }; + };packages/testing/src/adapters/vitest.ts (1)
15-16: Consider using standardimport typesyntax.The inline
type ZodType = import("zod").ZodTypeworks but is unconventional. A regularimport type { ZodType } from "zod"at the top of the file would be clearer.♻️ Use standard import type
import type { ToolResult } from "../types"; +import type { ZodType } from "zod"; -// ZodType is the modern replacement for ZodSchema in Zod v4+ -type ZodType = import("zod").ZodType;packages/testing/src/types.ts (1)
17-22: Consider using a discriminated union for ContentBlock.The current interface allows combinations that may be invalid (e.g.,
type: "text"withdataandmimeTypebut notext). A discriminated union would provide stronger type safety.♻️ Alternative with discriminated union
export type ContentBlock = | { type: "text"; text: string } | { type: "image"; data: string; mimeType: string } | { type: "resource"; data?: string; mimeType?: string };This is optional and depends on whether the MCP SDK's actual types support this level of strictness.
| beforeAll(async () => { | ||
| const testPort = 3001; | ||
| const server = await startTestServer(app, { port: testPort }); | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); |
There was a problem hiding this comment.
Replace hardcoded sleep with proper readiness check.
The hardcoded 100ms sleep after starting the test server is a test smell that can lead to flaky tests. If the server takes longer to start (e.g., under load or on slower CI machines), tests may fail intermittently.
Recommended approaches
Consider one of these alternatives:
- Add a readiness endpoint to the server and poll it
- Have
startTestServerreturn only when the server is ready to accept connections - Add retry logic to the first client operation with appropriate timeout
beforeAll(async () => {
const testPort = 3001;
const server = await startTestServer(app, { port: testPort });
- await new Promise((resolve) => setTimeout(resolve, 100));
+ // startTestServer should already ensure server is ready,
+ // or implement a readiness check here
const client = await createTestClient(`http://localhost:${testPort}/v1/mcp`, {
trackHistory: true,
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @examples/minimal/tests/greet-v1.test.ts at line 17, Replace the hardcoded
await new Promise(setTimeout...) by waiting for an actual readiness signal:
modify the test to either call a startTestServer variant that resolves only when
the server is listening (adjust or implement startTestServer to resolve after
server.listen callback), or poll a readiness endpoint (e.g., GET /health) until
it returns success with a timeout, or add retry logic around the first client
call (with exponential backoff and overall timeout); update the test in
greet-v1.test.ts to use startTestServer (or the new startTestServerReady) or the
polling helper instead of the 100ms sleep so the test proceeds only once the
server is truly ready.
| beforeAll(async () => { | ||
| const testPort = 3002; | ||
| const server = await startTestServer(app, { port: testPort }); | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
|
|
||
| const client = await createTestClient(`http://localhost:${testPort}/v2/mcp`, { | ||
| trackHistory: true, | ||
| }); | ||
|
|
||
| env = { | ||
| server, | ||
| client, | ||
| async cleanup() { | ||
| await client.disconnect(); | ||
| await server.stop(); | ||
| }, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Remove the arbitrary delay after server startup.
The setTimeout on line 17 suggests the server may not be fully ready immediately after startTestServer resolves. If startTestServer doesn't wait for the server to be ready, this should be fixed in the test server implementation rather than worked around with an arbitrary delay. Arbitrary delays make tests slower and can still cause flakiness if the delay is insufficient under load.
🔧 Recommended approach
If startTestServer doesn't guarantee server readiness, verify in the test server implementation (packages/testing/src/server/test-server.ts) that it waits for the server to start listening before resolving. Then remove the setTimeout:
beforeAll(async () => {
const testPort = 3002;
const server = await startTestServer(app, { port: testPort });
- await new Promise((resolve) => setTimeout(resolve, 100));
const client = await createTestClient(`http://localhost:${testPort}/v2/mcp`, {
trackHistory: true,
});Alternatively, if there's a genuine need to wait, add a readiness check:
const server = await startTestServer(app, { port: testPort });
// Wait for server to be ready by attempting connection with retry
await waitForServer(`http://localhost:${testPort}/v2/mcp`);🤖 Prompt for AI Agents
In @examples/minimal/tests/greet-v2.test.ts around lines 14 - 31, The test uses
an arbitrary delay after calling startTestServer — remove the setTimeout and
make startTestServer guarantee readiness before resolving (update the
startTestServer implementation to resolve only after the server's listen/ready
callback completes), then keep the test using startTestServer(...) and
createTestClient(...) without the delay; if you prefer not to change
startTestServer, implement a small waitForServer(url) retry helper and call
await waitForServer(`http://localhost:${testPort}/v2/mcp`) after startTestServer
instead of using setTimeout.
| /** | ||
| * Integration tests for minimal example | ||
| */ | ||
|
|
||
| import { describe, it, expect, beforeAll, afterAll } from "vitest"; | ||
| import { expectToolResult, startTestServer, createTestClient } from "@mcp-apps-kit/testing"; | ||
| import type { TestEnvironment } from "@mcp-apps-kit/testing"; | ||
| import { app } from "../src/index.js"; | ||
|
|
||
| describe("Minimal Example Integration", () => { | ||
| let env: TestEnvironment; | ||
|
|
||
| beforeAll(async () => { | ||
| const testPort = 3004; | ||
| const server = await startTestServer(app, { port: testPort }); | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
|
|
||
| const client = await createTestClient(`http://localhost:${testPort}/v1/mcp`, { | ||
| trackHistory: true, | ||
| timeout: 10000, | ||
| }); | ||
|
|
||
| env = { | ||
| server, | ||
| client, | ||
| async cleanup() { | ||
| await client.disconnect(); | ||
| await server.stop(); | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| await env.cleanup(); | ||
| }); | ||
|
|
||
| it("should start server and connect client", () => { | ||
| expect(env.server).toBeDefined(); | ||
| expect(env.client).toBeDefined(); | ||
| expect(env.server.url).toBeTruthy(); | ||
| }); | ||
|
|
||
| it("should list available tools", async () => { | ||
| const tools = await env.client.listTools(); | ||
| expect(tools.length).toBeGreaterThan(0); | ||
| expect(tools.some((t) => t.name === "greet")).toBe(true); | ||
| }); | ||
|
|
||
| it("should track call history when enabled", async () => { | ||
| env.client.clearHistory(); | ||
|
|
||
| await env.client.callTool("greet", { name: "HistoryTest" }); | ||
|
|
||
| const history = env.client.getCallHistory(); | ||
| expect(history.length).toBe(1); | ||
| expect(history[0]?.name).toBe("greet"); | ||
| expect(history[0]?.args).toEqual({ name: "HistoryTest" }); | ||
| }); | ||
|
|
||
| it("should handle multiple sequential calls", async () => { | ||
| env.client.clearHistory(); | ||
|
|
||
| const result1 = await env.client.callTool("greet", { name: "First" }); | ||
| const result2 = await env.client.callTool("greet", { name: "Second" }); | ||
| const result3 = await env.client.callTool("greet", { name: "Third" }); | ||
|
|
||
| const results = [result1, result2, result3]; | ||
| expect(results).toHaveLength(3); | ||
| results.forEach((result) => { | ||
| expectToolResult(result).toHaveNoError(); | ||
| }); | ||
|
|
||
| const history = env.client.getCallHistory(); | ||
| expect(history.length).toBe(3); | ||
| }); | ||
|
|
||
| it("should handle errors gracefully", async () => { | ||
| // Test with invalid input (missing required field) | ||
| try { | ||
| await env.client.callTool("greet", {} as { name: string }); | ||
| } catch (error) { | ||
| expect(error).toBeDefined(); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Relocate test to follow project structure guidelines.
Based on coding guidelines, test files should mirror source structure with subdirectories for unit/, integration/, and contract/ tests. This integration test should be moved to tests/integration/integration.test.ts or tests/integration/minimal-example.test.ts.
As per coding guidelines, test files in tests/ should be organized in subdirectories.
🤖 Prompt for AI Agents
In @examples/minimal/tests/integration.test.ts around lines 1 - 85, This test
file (the one containing describe("Minimal Example Integration") and importing
app) must be relocated into the project's tests/integration test folder and
renamed to follow conventions (e.g., minimal-example.test.ts or
integration.test.ts); move the file contents into that new test directory,
update any relative imports if they break (the import of app and testing
helpers), and ensure the test runner configuration includes the
tests/integration folder so the moved test is discovered.
|
|
||
| beforeAll(async () => { | ||
| const testPort = 3003; | ||
| mainServer = await startTestServer(app as unknown, { port: testPort }); |
There was a problem hiding this comment.
Investigate the as unknown type cast.
The cast to unknown defeats type safety. Since startTestServer accepts App | ExternalServerOptions according to its signature, and app should be of type App, this cast appears unnecessary. If there's a type mismatch, it should be resolved properly rather than worked around with a cast to unknown.
♻️ Suggested fix
-mainServer = await startTestServer(app as unknown, { port: testPort });
+mainServer = await startTestServer(app, { port: testPort });If this causes a type error, the underlying type mismatch should be resolved rather than hidden with a cast.
📝 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.
| mainServer = await startTestServer(app as unknown, { port: testPort }); | |
| mainServer = await startTestServer(app, { port: testPort }); |
🤖 Prompt for AI Agents
In @examples/minimal/tests/integration/versioning.test.ts at line 17, The cast
"as unknown" on the app passed to startTestServer defeats type safety; remove
the unnecessary cast and ensure app is typed to match startTestServer's
parameter (App | ExternalServerOptions). Locate the call to startTestServer
(symbol: startTestServer) and the app variable (symbol: app), then either
declare/annotate app as App or adapt the argument to the expected union (e.g.,
pass an ExternalServerOptions object or wrap app appropriately) so the type
error is resolved without using "as unknown" before assigning to mainServer.
| */ | ||
|
|
||
| import { describe, it, expect, vi, beforeEach } from "vitest"; | ||
| import { render, screen, waitFor, fireEvent } from "@testing-library/react"; |
There was a problem hiding this comment.
Remove unused import fireEvent.
The fireEvent import is not used anywhere in this test file. All user interactions are handled via userEvent instead.
🧹 Proposed fix
-import { render, screen, waitFor, fireEvent } from "@testing-library/react";
+import { render, screen, waitFor } from "@testing-library/react";📝 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.
| import { render, screen, waitFor, fireEvent } from "@testing-library/react"; | |
| import { render, screen, waitFor } from "@testing-library/react"; |
🤖 Prompt for AI Agents
In @examples/minimal/tests/ui-widget.test.tsx at line 11, The import list
includes an unused symbol "fireEvent" which should be removed to clean up the
test file; update the import statement that currently imports render, screen,
waitFor, fireEvent from "@testing-library/react" to drop "fireEvent" so only the
used imports remain (e.g., render, screen, waitFor).
| const criteriaResults: CriterionResult[] = options.criteria.map((criterion) => { | ||
| const result = parsed.criteria.find((c) => c.name === criterion.name); | ||
| if (!result) { | ||
| throw new Error(`Missing evaluation result for criterion: ${criterion.name}`); | ||
| } |
There was a problem hiding this comment.
Variable shadowing: result shadows outer parameter.
The variable result in the find callback shadows the outer result parameter passed to evaluate(). This is confusing and could lead to bugs if the wrong variable is referenced. Consider renaming the inner variable.
🐛 Proposed fix
const criteriaResults: CriterionResult[] = options.criteria.map((criterion) => {
- const result = parsed.criteria.find((c) => c.name === criterion.name);
- if (!result) {
+ const criterionMatch = parsed.criteria.find((c) => c.name === criterion.name);
+ if (!criterionMatch) {
throw new Error(`Missing evaluation result for criterion: ${criterion.name}`);
}
const threshold = criterion.threshold ?? 0.7;
return {
name: criterion.name,
- score: result.score,
- pass: result.score >= threshold,
- explanation: result.explanation,
+ score: criterionMatch.score,
+ pass: criterionMatch.score >= threshold,
+ explanation: criterionMatch.explanation,
};
});📝 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.
| const criteriaResults: CriterionResult[] = options.criteria.map((criterion) => { | |
| const result = parsed.criteria.find((c) => c.name === criterion.name); | |
| if (!result) { | |
| throw new Error(`Missing evaluation result for criterion: ${criterion.name}`); | |
| } | |
| const criteriaResults: CriterionResult[] = options.criteria.map((criterion) => { | |
| const criterionMatch = parsed.criteria.find((c) => c.name === criterion.name); | |
| if (!criterionMatch) { | |
| throw new Error(`Missing evaluation result for criterion: ${criterion.name}`); | |
| } | |
| const threshold = criterion.threshold ?? 0.7; | |
| return { | |
| name: criterion.name, | |
| score: criterionMatch.score, | |
| pass: criterionMatch.score >= threshold, | |
| explanation: criterionMatch.explanation, | |
| }; | |
| }); |
🤖 Prompt for AI Agents
In @packages/testing/src/eval/llm/providers/anthropic.ts around lines 179 - 183,
The inner variable named result in the parsed.criteria.find callback shadows the
outer result parameter of evaluate(), so rename the inner variable to a
non-conflicting name (e.g., parsedCriterion or parsedItem) inside the
options.criteria.map block to avoid shadowing; update references in the callback
and downstream use in the creation of CriterionResult[] so the logic and types
(CriterionResult[], parsed.criteria.find, options.criteria.map) remain identical
but without name collision.
| // Start creating client and cache the promise | ||
| pendingPromise = createClient(apiKey).then((client) => { | ||
| cachedClient = client; | ||
| cachedApiKey = apiKey; | ||
| pendingPromise = null; | ||
| return client; | ||
| }); | ||
|
|
||
| return pendingPromise; | ||
| }, |
There was a problem hiding this comment.
Missing error handling for failed client creation.
If createClient rejects, pendingPromise is never cleared, causing subsequent calls with the same API key to return the rejected promise. Add error handling to clear state on failure.
🐛 Proposed fix
// Start creating client and cache the promise
- pendingPromise = createClient(apiKey).then((client) => {
- cachedClient = client;
- cachedApiKey = apiKey;
- pendingPromise = null;
- return client;
- });
+ pendingPromise = createClient(apiKey)
+ .then((client) => {
+ cachedClient = client;
+ cachedApiKey = apiKey;
+ pendingPromise = null;
+ return client;
+ })
+ .catch((error) => {
+ // Clear state on failure to allow retry
+ pendingPromise = null;
+ cachedApiKey = null;
+ throw error;
+ });📝 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.
| // Start creating client and cache the promise | |
| pendingPromise = createClient(apiKey).then((client) => { | |
| cachedClient = client; | |
| cachedApiKey = apiKey; | |
| pendingPromise = null; | |
| return client; | |
| }); | |
| return pendingPromise; | |
| }, | |
| // Start creating client and cache the promise | |
| pendingPromise = createClient(apiKey) | |
| .then((client) => { | |
| cachedClient = client; | |
| cachedApiKey = apiKey; | |
| pendingPromise = null; | |
| return client; | |
| }) | |
| .catch((error) => { | |
| // Clear state on failure to allow retry | |
| pendingPromise = null; | |
| cachedApiKey = null; | |
| throw error; | |
| }); | |
| return pendingPromise; | |
| }, |
🤖 Prompt for AI Agents
In @packages/testing/src/utils/lazy-loader.ts around lines 138 - 147, The
pendingPromise created in the lazy loader (the block that sets pendingPromise =
createClient(apiKey).then(...)) lacks rejection handling so a failed
createClient leaves pendingPromise set and subsequent calls reuse the rejected
promise; fix by attaching a .catch handler to that promise which clears
pendingPromise (and also resets cachedClient and cachedApiKey if appropriate)
before rethrowing the error so state is clean for retries; update the promise
chain around createClient, pendingPromise, cachedClient, and cachedApiKey
accordingly.
| * using mock clients to simulate MCP tool calls. | ||
| */ | ||
|
|
||
| import { describe, it, expect, vi, beforeEach } from "vitest"; |
There was a problem hiding this comment.
Remove unused import beforeEach.
The beforeEach import from vitest is never used. The local constants named beforeEach defined on lines 33 and 231 shadow this import. Remove it from the import statement.
♻️ Proposed fix
-import { describe, it, expect, vi, beforeEach } from "vitest";
+import { describe, it, expect, vi } from "vitest";📝 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.
| import { describe, it, expect, vi, beforeEach } from "vitest"; | |
| import { describe, it, expect, vi } from "vitest"; |
🤖 Prompt for AI Agents
In @packages/testing/tests/integration/behavior-testing.test.ts at line 8, The
import line currently imports beforeEach from vitest but that symbol is never
used and is shadowed by local constants named beforeEach in this test file;
remove beforeEach from the named imports in the vitest import statement (i.e.,
change `import { describe, it, expect, vi, beforeEach } from "vitest";` to
exclude beforeEach) so the code no longer imports an unused symbol while leaving
the other imports intact.
| it("should run property tests with generators", async () => { | ||
| // This test requires fast-check to be installed | ||
| // Will be skipped or fail gracefully if not available | ||
| try { | ||
| const stringGen = generators.string({ minLength: 1, maxLength: 10 }); | ||
|
|
||
| await forAllInputs( | ||
| stringGen, | ||
| (input) => { | ||
| // Property: string length should be within bounds | ||
| return input.length >= 1 && input.length <= 10; | ||
| }, | ||
| { numRuns: 10 } | ||
| ); | ||
| } catch (error) { | ||
| // If fast-check is not available, skip the test | ||
| if (error instanceof Error && error.message.includes("fast-check")) { | ||
| // Test skipped - fast-check not available | ||
| return; | ||
| } | ||
| throw error; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Replace silent test skipping with explicit skip mechanisms.
The try-catch pattern that silently returns when fast-check is unavailable (lines 28-32) causes the test to pass without actually running any assertions. This masks whether fast-check is properly installed and could hide real failures. Use Vitest's conditional skip mechanisms instead.
♻️ Proposed refactor
+import { vi } from "vitest";
+
+// Check if fast-check is available
+let hasFastCheck = false;
+try {
+ await import("fast-check");
+ hasFastCheck = true;
+} catch {
+ // fast-check not available
+}
+
describe("Property Testing Integration", () => {
- it("should run property tests with generators", async () => {
- // This test requires fast-check to be installed
- // Will be skipped or fail gracefully if not available
- try {
- const stringGen = generators.string({ minLength: 1, maxLength: 10 });
+ it.skipIf(!hasFastCheck)("should run property tests with generators", async () => {
+ const stringGen = generators.string({ minLength: 1, maxLength: 10 });
- await forAllInputs(
- stringGen,
- (input) => {
- // Property: string length should be within bounds
- return input.length >= 1 && input.length <= 10;
- },
- { numRuns: 10 }
- );
- } catch (error) {
- // If fast-check is not available, skip the test
- if (error instanceof Error && error.message.includes("fast-check")) {
- // Test skipped - fast-check not available
- return;
- }
- throw error;
- }
+ await forAllInputs(
+ stringGen,
+ (input) => {
+ // Property: string length should be within bounds
+ return input.length >= 1 && input.length <= 10;
+ },
+ { numRuns: 10 }
+ );
});Apply the same pattern to the second test at lines 37-60.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @packages/testing/tests/integration/property-testing.test.ts around lines 13
- 35, The test currently swallows the absence of fast-check by returning inside
a try-catch; instead detect availability of fast-check up front and use Vitest's
conditional skip (e.g., test.skip or conditionally call it) so the test is
explicitly skipped when missing; specifically, replace the try-catch around
generators.string and forAllInputs with a pre-check (attempt to require/import
fast-check or check that generators and forAllInputs are present), and if
unavailable call the test skip mechanism before executing the assertions, and
apply the same change to the second test that also wraps fast-check usage (the
block using generators.string and forAllInputs).
| /** | ||
| * Unit tests for LLM evaluator | ||
| * | ||
| * Note: These tests require OpenAI or Anthropic SDKs to be installed. | ||
| * They will be skipped if the dependencies are not available. | ||
| */ | ||
|
|
||
| import { describe, it, expect } from "vitest"; | ||
| import { createLLMEvaluator, criteria } from "../../../src/eval/llm"; | ||
| import { ConfigurationError } from "../../../src/errors"; | ||
|
|
There was a problem hiding this comment.
Test file structure should mirror source directory.
The test file is located at tests/unit/eval/evaluator.test.ts but the source file is at src/eval/llm/evaluator.ts. According to project conventions, the test should be at tests/unit/eval/llm/evaluator.test.ts to mirror the source structure.
Based on learnings, test files should mirror the source structure with unit/, integration/, and contract/ subdirectories.
📁 Suggested file relocation
Move this test file to:
packages/testing/tests/unit/eval/llm/evaluator.test.ts
And update the import paths accordingly:
-import { createLLMEvaluator, criteria } from "../../../src/eval/llm";
-import { ConfigurationError } from "../../../src/errors";
+import { createLLMEvaluator, criteria } from "../../../../src/eval/llm";
+import { ConfigurationError } from "../../../../src/errors";📝 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.
| /** | |
| * Unit tests for LLM evaluator | |
| * | |
| * Note: These tests require OpenAI or Anthropic SDKs to be installed. | |
| * They will be skipped if the dependencies are not available. | |
| */ | |
| import { describe, it, expect } from "vitest"; | |
| import { createLLMEvaluator, criteria } from "../../../src/eval/llm"; | |
| import { ConfigurationError } from "../../../src/errors"; | |
| /** | |
| * Unit tests for LLM evaluator | |
| * | |
| * Note: These tests require OpenAI or Anthropic SDKs to be installed. | |
| * They will be skipped if the dependencies are not available. | |
| */ | |
| import { describe, it, expect } from "vitest"; | |
| import { createLLMEvaluator, criteria } from "../../../../src/eval/llm"; | |
| import { ConfigurationError } from "../../../../src/errors"; | |
🤖 Prompt for AI Agents
In @packages/testing/tests/unit/eval/evaluator.test.ts around lines 1 - 11, The
test file must be moved to mirror the source's nested eval/llm structure under
the unit tests (place it in the tests/unit location matching the src eval llm
hierarchy) and then update the module imports so they resolve from the new
relative location (ensure imports referencing createLLMEvaluator, criteria and
ConfigurationError point to the correct relative paths after the move); keep the
test logic unchanged, only relocate the file and fix the import paths.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| // Build evaluation result | ||
| const criteriaResults: CriterionResult[] = options.criteria.map((criterion) => { | ||
| const result = parsed.criteria.find((c) => c.name === criterion.name); |
There was a problem hiding this comment.
OpenAI provider lacks JSON structure validation before use
Medium Severity
The OpenAI provider parses JSON and casts it directly to the expected type without runtime validation. If the LLM returns valid JSON with an unexpected structure (e.g., {"message": "I cannot evaluate"}), parsed.criteria will be undefined, and calling .find() on it will throw a confusing TypeError: Cannot read properties of undefined (reading 'find'). The Anthropic provider in the same codebase has extensive validation (checking Array.isArray(parsedObj.criteria), validating each criterion's properties) that provides clear error messages. The same validation pattern is missing from the OpenAI provider.
| cachedApiKey = apiKey; | ||
| pendingPromise = null; | ||
| return client; | ||
| }); |
There was a problem hiding this comment.
Cached client factory never clears rejected promises
Low Severity
In createCachedClientFactory, the pendingPromise is only set to null in the .then() success callback. If createClient rejects (e.g., due to a transient network issue during module import), the rejected promise remains cached, and all subsequent calls with the same API key will return the same rejected promise instead of retrying. This is inconsistent with createLazyLoader in the same file, which correctly clears state.loadPromise = null in the catch block to allow retry attempts.
| } | ||
|
|
||
| // Create test client | ||
| client = await createTestClient(mcpUrl, options.clientOptions); |
There was a problem hiding this comment.
Server not stopped if client creation fails
Medium Severity
In createTestEnvironment, if createTestClient at line 74 throws an error (e.g., connection refused, network timeout), the server started at line 48 is never stopped. The function throws the error without calling server.stop(), leaving an orphaned server process running. This causes resource leaks, port conflicts in subsequent tests, and potential test flakiness. The server creation and client creation need to be wrapped in try-catch with cleanup on failure.
…documentation - Add OpenAI and dotenv as dependencies in the minimal example. - Implement LLM evaluation tests using OpenAI, including setup instructions in the README. - Update test setup to load environment variables from a .env file. - Enhance testing documentation with detailed instructions for running LLM evaluation tests. - Introduce new test files for LLM evaluation and improve overall test structure.
…ns and error handling - Introduce comprehensive evaluation tests for the MCP framework, covering basic tool evaluation, multi-turn conversations, batch evaluation, and multi-criteria judging. - Implement error handling tests to validate LLM responses under error injection scenarios. - Update README documentation to include new features and usage examples for multi-turn conversations and batch evaluations. - Refactor test setup to support token usage tracking and improved result reporting. - Enhance the testing framework with new utility functions for session management and batch processing.
- Enhance the README with instructions for setting up the OpenAI API key and running LLM-based evaluation tests. - Refactor test files for improved readability by consolidating code and removing unnecessary whitespace. - Update various test cases to ensure consistent formatting and maintainability. - Improve error handling in test scenarios to provide clearer feedback on failures.
- Introduce utility functions for parsing and formatting costs in dollars, improving cost handling in batch evaluations and session management. - Update error injection state management to allow resetting, enhancing testing scenarios for retry logic. - Refactor cost aggregation logic to ensure consistent dollar normalization across evaluations. - Improve documentation for error injection methods to clarify usage and deprecate older patterns.
- Upgrade @anthropic-ai/sdk from version 0.20.9 to 0.71.2 with updated specifier to '>=0.30.0'. - Introduce json-schema-to-ts@3.1.1 and ts-algebra@2.0.0 as new dependencies. - Remove deprecated web-streams-polyfill@3.3.3 and update its version to 4.0.0-beta.3. - Add optional peer dependency for zod with version 4.2.1.
| * Calculate delay for a retry attempt | ||
| */ | ||
| function calculateDelay(attempt: number, config: Required<Omit<RetryConfig, "retryOn">>): number { | ||
| let delay = config.delay; |
Comprehensive Code Review: Testing Library ImplementationOverviewThis is an excellent and comprehensive PR that introduces a full-featured testing library for MCP applications. The implementation is well-architected, thoroughly documented, and follows best practices. The scope is ambitious (86 files, 13,815 additions) but well-executed. 🎯 Strengths1. Architecture & Design ✨
2. Documentation 📚
3. Testing Strategy 🧪
4. Error Handling 🛡️
🔍 Areas for Improvement1. Security Considerations 🔒API Key HandlingThe library reads API keys from environment variables which is correct, but consider adding guidance: packages/testing/src/eval/mcp/providers/index.ts:35-37 return process.env.OPENAI_API_KEY;
return process.env.ANTHROPIC_API_KEY;Recommendation: Add a security note in the README warning users:
Process Spawningpackages/testing/src/server/test-server.ts:150-153 const childProcess = spawn(command, args, {
env: { ...process.env, ...env },
stdio: ["ignore", "pipe", "pipe"],
});This is generally safe but document that users should validate 2. Potential Bugs 🐛Timeout Cleanup Edge Casepackages/testing/src/server/test-server.ts:89-100 The timeout is cleared after const timeoutPromise = new Promise<never>((_, reject) => {
timeoutId = setTimeout(() => {
reject(new ServerStartupError(...));
}, timeout);
});Recommendation: Use a try {
await Promise.race([startPromise, timeoutPromise]);
} finally {
if (timeoutId !== undefined) {
clearTimeout(timeoutId);
}
}Process Kill Safetypackages/testing/src/server/test-server.ts:195-204 The async stop(): Promise<void> {
if (childProcess && !childProcess.killed) {
childProcess.kill();
}
}Recommendation: Add a timeout and force kill if needed: async stop(): Promise<void> {
if (childProcess && !childProcess.killed) {
childProcess.kill('SIGTERM');
// Wait for graceful shutdown
await new Promise((resolve) => {
const timeout = setTimeout(() => {
childProcess.kill('SIGKILL');
resolve(undefined);
}, 5000);
childProcess.once('exit', () => {
clearTimeout(timeout);
resolve(undefined);
});
});
}
}3. Performance Considerations ⚡Rate Limiting ImplementationThe retry/rate limiting logic is good (packages/testing/src/eval/mcp/retry.ts:317), but consider: Recommendation:
Test Server Port Findingpackages/testing/src/server/test-server.ts:40-61 The server.listen(0, () => {
const port = addr.port;
server.close((err) => {
resolve(port);
});
});Between closing and starting the actual server, another process could grab the port. Recommendation: Document this known limitation or consider:
4. Code Quality 🎨Consistency in Error HandlingSome places use packages/testing/src/server/test-server.ts:56 reject(new Error("Failed to detect available port"));Recommendation: Use Type Castingpackages/testing/src/server/test-client.ts:86 const structuredContent = (result as { structuredContent?: unknown }).structuredContent;Recommendation: Define a proper interface for the MCP result type to avoid casting. 5. Testing Coverage 🧪The testing library itself has good test coverage, but consider: Missing test scenarios:
Recommendation: Add integration tests that specifically test:
📝 Minor Issues1. TypeScript
2. Documentation Gaps
3. Dependenciespackages/testing/package.json:70 "fast-check": "^3.0.0"Consider updating to "fast-check": "^4.5.3"🎯 RecommendationsHigh Priority
Medium Priority
Low Priority
🌟 Exemplary Practices
📊 Summary
✅ ConclusionThis PR is ready to merge with the recommended fixes for the timeout cleanup and process termination issues. The testing library is well-designed, thoroughly documented, and will be a valuable addition to the MCP AppsKit ecosystem. The scope is large but justified—this provides comprehensive testing capabilities that the project needs. Great work! 🎉 Recommended Action:
|
Code Review: Testing Library ImplementationThis is an excellent and comprehensive testing library addition! The implementation is well-structured, thoroughly tested, and follows best practices. Here's my detailed review: ✅ Strengths1. Architecture & Design
2. TypeScript Quality
3. Testing
4. Developer Experience
5. Feature Completeness
🔍 Areas for Consideration1. Console Logging (Low Priority)Found 60+
Recommendation: Consider using the debug logger pattern consistently instead of 2. Environment Variables (Medium Priority)API keys are accessed from environment variables in multiple places:
Current implementation is GOOD:
Minor suggestion: Consider adding a runtime check or warning when eval tests run without keys to make it more obvious why tests are skipped. 3. Peer Dependencies (Good as-is)The package correctly marks all optional dependencies as peer dependencies:
This is the correct approach - no changes needed. 4. Test Guard Pattern (Excellent)The minimal example properly guards server startup during tests: // Only start server if not in test environment
if (process.env.NODE_ENV !== "test" && !process.env.VITEST) {
app.start({ port });
}This is the right pattern for testable applications. 🔒 SecurityNo security concerns found:
📊 Metrics
🎯 RecommendationsMust Address (None)All code quality issues are acceptable for this PR. Should Consider (Optional)
Nice to Have
✨ VerdictLGTM - Excellent work! 🎉 This is a production-ready testing library that demonstrates:
The code follows the project's CLAUDE.md guidelines and maintains high quality standards. The minor suggestions above are purely optional improvements that don't block merging. Recommendation: ✅ Approve and Merge Great job on this substantial contribution! The testing library will be a valuable addition to the MCP AppsKit ecosystem. |
- Eliminate the deprecated resetErrorInjectionState function from error-injection.ts, encouraging the use of wrappedClient.reset() instead. - Update index.ts to remove the export of resetErrorInjectionState, streamlining the error injection API.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @examples/minimal/README.md:
- Around line 88-91: The fenced code block containing
"OPENAI_API_KEY=sk-your-actual-api-key" is missing a language identifier; update
the opening triple-backtick for that block to include a language (e.g., "bash")
so it matches the other block and satisfies markdown linting/consistency.
In @packages/testing/src/eval/llm/providers/openai.ts:
- Around line 84-109: The code blindly casts JSON.parse(content) to the expected
shape and then assumes parsed.criteria and parsed.overall exist; add runtime
validation after parsing to guard against malformed responses by checking parsed
is an object, parsed.criteria is an array, each parsed.criteria item has name
(string), score (number) and explanation (string), and parsed.overall exists
with a numeric score before using them in the loop that builds CriteriaResults
and before computing overallScore; if any check fails, throw a clear Error that
includes the raw content (or a snippet) and a message like "Invalid LLM
response: missing or malformed 'criteria'/'overall'". Ensure you reference and
validate parsed (the variable from JSON.parse), parsed.criteria, parsed.overall,
and the loop over options.criteria/CriteriaResults to avoid TypeError at
runtime.
In @packages/testing/src/eval/mcp/providers/anthropic.ts:
- Around line 70-71: Remove the unused eslint-disable directive preceding the
declaration of the content array: delete the line "// eslint-disable-next-line
@typescript-eslint/no-explicit-any" and leave the declaration "const content:
any[] = [];" as-is (or replace the any type with a more specific type if
desired) so the file no longer contains the unnecessary eslint-disable comment.
In @packages/testing/src/eval/mcp/session.ts:
- Around line 117-119: The end() method calls this.reset() which will break if
end is extracted or used as a callback; change its implementation so it doesn't
rely on dynamic this: either convert end into a bound arrow property (e.g., end
= () => { this.reset(); }) or move reset into a closure/local function and call
that directly from end; update the declaration of end() in the same object/class
where reset() is defined to use one of these approaches so calling end without
the original this still invokes reset.
In @packages/testing/src/eval/reporter/reporter.ts:
- Around line 32-42: The ESLint no-console warning comes from the console calls
in the defaultOutput object; suppress it by disabling the rule only for this
block (e.g., add a block-level suppression like /* eslint-disable no-console */
immediately before the defaultOutput definition and /* eslint-enable no-console
*/ immediately after) so the EvalReporterOutput defaultOutput (log/warn/error)
can intentionally use console without triggering pipeline warnings.
In @packages/testing/src/eval/reporter/types.ts:
- Around line 54-70: The EvalReportSummary type is used as the return type for
EvalReporter methods printSummary() and getSummary() but isn't exported from the
package public API; export it from the eval/reporter exports in your public
index (add EvalReportSummary to the exported symbols alongside EvalReporter) so
consumers can import it, and optionally convert the declaration from "export
interface EvalReportSummary" to "export type EvalReportSummary = { ... }" to
mark it as a type-only export per TypeScript guidelines.
🧹 Nitpick comments (11)
packages/testing/README.md (1)
724-730: Add language specifier to fenced code block.The example output block at line 724 is missing a language specifier. For console output, use
textorconsoleto satisfy linting and improve syntax highlighting.📝 Suggested fix
-``` +```text [MCP EVAL] Please greet Alice Tools: ✓ greet({"name":"Alice"}) Response: Hello, Alice! Duration: 1406ms Judge: [PASS] (100%) - The agent successfully greeted Alice with a friendly message.</details> </blockquote></details> <details> <summary>packages/testing/src/server/test-client.ts (1)</summary><blockquote> `73-82`: **Consider extracting inline type to improve maintainability.** The local `ContentBlock` type definition shadows the similarly-named interface from `../types`. While this inline definition serves its purpose for SDK response mapping, consider adding a brief comment explaining why this differs from the shared `ContentBlock` type to prevent confusion during maintenance. </blockquote></details> <details> <summary>packages/testing/src/eval/mcp/providers/openai.ts (1)</summary><blockquote> `165-171`: **Silent failure in `safeParseJSON` may hide tool argument parsing issues.** When tool arguments fail to parse, returning an empty object silently could mask issues with malformed tool calls from the LLM. Consider logging the parse failure for debugging purposes. <details> <summary>🔧 Suggested improvement</summary> ```diff +import { llmLogger } from "../../../debug"; + function safeParseJSON(str: string): Record<string, unknown> { try { return JSON.parse(str) as Record<string, unknown>; - } catch { + } catch (error) { + llmLogger("Failed to parse tool arguments: %s, error: %O", str, error); return {}; } }examples/minimal/tests/eval.test.ts (1)
394-399: Skip block may be redundant ifdescribeEvalalready handles provider key checks.The
describeEvalwrapper likely already skips tests when no provider keys are available. This explicit skip block creates a duplicate test entry that could be confusing in test output.🔧 Consider removing redundant skip block
If
describeEvalinternally checkshasAnyProviderKey()and skips appropriately, this block can be removed. Otherwise, consider consolidating the skip logic in one place.packages/testing/src/eval/llm/providers/anthropic.ts (1)
86-176: Consider extracting JSON validation to a separate function.The runtime validation logic (lines 103-163) is comprehensive but makes the
evaluatemethod quite long. Extracting this to a helper function would improve readability.🔧 Suggested extraction
interface ParsedEvalResponse { criteria: Array<{ name: string; score: number; explanation: string }>; overall: { score: number }; } function validateParsedResponse(rawParsed: unknown, textPreview: string): ParsedEvalResponse { if (typeof rawParsed !== "object" || rawParsed === null) { throw new Error(`Expected parsed response to be an object, got ${typeof rawParsed}. Response preview: ${textPreview}`); } // ... rest of validation return rawParsed as ParsedEvalResponse; }packages/testing/src/eval/mcp/providers/index.ts (1)
53-62: Unreachable default case returns same value as explicit case.The
defaultbranch on line 59-60 is unreachable becauseProviderTypeis exhaustively handled. While it provides a safety net, consider using TypeScript's exhaustive check pattern instead:♻️ Suggested improvement
export function getDefaultModel(provider: ProviderType): string { switch (provider) { case "openai": return "gpt-4o-mini"; case "anthropic": return "claude-3-haiku-20240307"; - default: - return "gpt-4o-mini"; + default: { + const _exhaustive: never = provider; + return _exhaustive; + } } }packages/testing/src/eval/mcp/providers/anthropic.ts (1)
193-198: Hardcodedmax_tokens: 512may be insufficient for complex JSON responses.
createJSONCompletionuses a fixed 512 token limit whilecreateCompletionuses the configurablemaxTokensfrom config. Multi-criteria judge responses with detailed explanations could be truncated.♻️ Suggested fix
const response: any = await anthropic.messages.create({ model, - max_tokens: 512, + max_tokens: Math.min(maxTokens, 1024), // Cap for JSON responses system: jsonSystemPrompt, messages: anthropicMessages, });packages/testing/src/eval/mcp/reporter.ts (2)
268-273: Fragile private field access via bracket notation.Accessing
defaultReporter?.["verbose"]relies on implementation details. If the field is renamed or the class structure changes, this will silently break.♻️ Suggested improvement
+let currentVerboseSetting: boolean | null = null; + export function getReporter(verbose: boolean = true): MCPEvalReporter { - if (defaultReporter?.["verbose"] !== verbose) { + if (currentVerboseSetting !== verbose) { defaultReporter = new MCPEvalReporter(verbose); + currentVerboseSetting = verbose; } return defaultReporter; }
353-357: Duplicate cost parsing logic.The cost string parsing (
replace(/[$¢]/g, "")thenparseFloat) appears in bothprintSummary(line 355-356) andgetSummary(line 463-464). The module already importsparseCostToDollarsfrom./cost-utils—consider using it here for consistency.♻️ Suggested fix in printSummary
+import { parseCostToDollars } from "./cost-utils"; + if (result.usage.estimatedCost) { hasCost = true; - const costStr = result.usage.estimatedCost.replace(/[$¢]/g, ""); - totalCost += parseFloat(costStr) || 0; + totalCost += parseCostToDollars(result.usage.estimatedCost); }Also applies to: 462-465
packages/testing/src/eval/mcp/batch.ts (2)
165-170: Duplicatesleeputility function.This
sleephelper is identical to the one inretry.ts. Consider extracting to a shared utility module to avoid duplication.
200-200: Minor:failuresarray built redundantly.The
failuresarray is populated during sequential execution (line 307) but then rebuilt fromresults.filter()(line 353) for the final result. The in-loop population is only used forstopOnFailurelogging. This is fine but the final rebuild makes the in-loop tracking partially redundant.Also applies to: 307-307, 353-354
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
examples/minimal/.env.exampleexamples/minimal/README.mdexamples/minimal/package.jsonexamples/minimal/tests/eval.test.tsexamples/minimal/tests/setup.tspackages/testing/README.mdpackages/testing/package.jsonpackages/testing/src/eval/index.tspackages/testing/src/eval/llm/providers/anthropic.tspackages/testing/src/eval/llm/providers/openai.tspackages/testing/src/eval/mcp/batch.tspackages/testing/src/eval/mcp/cost-utils.tspackages/testing/src/eval/mcp/error-injection.tspackages/testing/src/eval/mcp/evaluator.tspackages/testing/src/eval/mcp/index.tspackages/testing/src/eval/mcp/providers/anthropic.tspackages/testing/src/eval/mcp/providers/index.tspackages/testing/src/eval/mcp/providers/openai.tspackages/testing/src/eval/mcp/providers/types.tspackages/testing/src/eval/mcp/reporter.tspackages/testing/src/eval/mcp/retry.tspackages/testing/src/eval/mcp/session.tspackages/testing/src/eval/reporter/index.tspackages/testing/src/eval/reporter/reporter.tspackages/testing/src/eval/reporter/types.tspackages/testing/src/index.tspackages/testing/src/server/test-client.tspackages/testing/src/types.ts
✅ Files skipped from review due to trivial changes (1)
- packages/testing/src/eval/mcp/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/minimal/package.json
- examples/minimal/tests/setup.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript with no
anytypes - useunknownand narrow instead
Files:
packages/testing/src/eval/mcp/cost-utils.tspackages/testing/src/eval/mcp/providers/types.tspackages/testing/src/eval/mcp/providers/openai.tspackages/testing/src/eval/reporter/types.tspackages/testing/src/eval/mcp/providers/index.tspackages/testing/src/eval/mcp/providers/anthropic.tspackages/testing/src/eval/index.tspackages/testing/src/eval/reporter/reporter.tspackages/testing/src/eval/reporter/index.tspackages/testing/src/eval/mcp/session.tspackages/testing/src/server/test-client.tspackages/testing/src/eval/mcp/batch.tsexamples/minimal/tests/eval.test.tspackages/testing/src/eval/mcp/error-injection.tspackages/testing/src/eval/llm/providers/openai.tspackages/testing/src/eval/mcp/retry.tspackages/testing/src/eval/mcp/evaluator.tspackages/testing/src/types.tspackages/testing/src/index.tspackages/testing/src/eval/llm/providers/anthropic.tspackages/testing/src/eval/mcp/reporter.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Remove unused variables or prefix with underscore (
_)
Files:
packages/testing/src/eval/mcp/cost-utils.tspackages/testing/src/eval/mcp/providers/types.tspackages/testing/src/eval/mcp/providers/openai.tspackages/testing/src/eval/reporter/types.tspackages/testing/src/eval/mcp/providers/index.tspackages/testing/src/eval/mcp/providers/anthropic.tspackages/testing/src/eval/index.tspackages/testing/src/eval/reporter/reporter.tspackages/testing/src/eval/reporter/index.tspackages/testing/src/eval/mcp/session.tspackages/testing/src/server/test-client.tspackages/testing/src/eval/mcp/batch.tsexamples/minimal/tests/eval.test.tspackages/testing/src/eval/mcp/error-injection.tspackages/testing/src/eval/llm/providers/openai.tspackages/testing/src/eval/mcp/retry.tspackages/testing/src/eval/mcp/evaluator.tspackages/testing/src/types.tspackages/testing/src/index.tspackages/testing/src/eval/llm/providers/anthropic.tspackages/testing/src/eval/mcp/reporter.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
export typefor type-only exports
Files:
packages/testing/src/eval/mcp/cost-utils.tspackages/testing/src/eval/mcp/providers/types.tspackages/testing/src/eval/mcp/providers/openai.tspackages/testing/src/eval/reporter/types.tspackages/testing/src/eval/mcp/providers/index.tspackages/testing/src/eval/mcp/providers/anthropic.tspackages/testing/src/eval/index.tspackages/testing/src/eval/reporter/reporter.tspackages/testing/src/eval/reporter/index.tspackages/testing/src/eval/mcp/session.tspackages/testing/src/server/test-client.tspackages/testing/src/eval/mcp/batch.tsexamples/minimal/tests/eval.test.tspackages/testing/src/eval/mcp/error-injection.tspackages/testing/src/eval/llm/providers/openai.tspackages/testing/src/eval/mcp/retry.tspackages/testing/src/eval/mcp/evaluator.tspackages/testing/src/types.tspackages/testing/src/index.tspackages/testing/src/eval/llm/providers/anthropic.tspackages/testing/src/eval/mcp/reporter.ts
**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Export public API only in
index.tsfiles
Files:
packages/testing/src/eval/mcp/providers/index.tspackages/testing/src/eval/index.tspackages/testing/src/eval/reporter/index.tspackages/testing/src/index.ts
**/tests/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Mirror test file structure to source: tests in
tests/directory withunit/,integration/, andcontract/subdirectories
Files:
examples/minimal/tests/eval.test.ts
{packages/core,examples}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
{packages/core,examples}/**/*.ts: Always usedefineToolanddefineUIfor type inference when defining tools and UI components
Use Koa-style async/await middleware pattern withawait next()to chain middleware execution
UseAppErrorandErrorCodefrom@mcp-apps-kit/corefor error handling
Implement plugins using thePlugininterface with hooks:onInit,onStart,onShutdown,beforeToolCall,afterToolCall,onToolError
Useapp.events.on()andapp.events.once()for event subscription with event types likeapp:init,tool:call,app:start
Use Zod schemas withdefineToolfor input/output validation
Colocate UI definitions near tool definitions usingdefineUIwithhtmlproperty pointing to compiled UI assets
Files:
examples/minimal/tests/eval.test.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to **/tests/**/*.{test,spec}.{ts,tsx} : Mirror test file structure to source: tests in `tests/` directory with `unit/`, `integration/`, and `contract/` subdirectories
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Use `AppError` and `ErrorCode` from `mcp-apps-kit/core` for error handling
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Always use `defineTool` and `defineUI` for type inference when defining tools and UI components
Applied to files:
packages/testing/src/eval/mcp/providers/types.tspackages/testing/src/eval/reporter/types.tsexamples/minimal/README.mdpackages/testing/src/types.tspackages/testing/src/index.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to **/*.ts : Use `export type` for type-only exports
Applied to files:
packages/testing/src/eval/reporter/types.tspackages/testing/src/types.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to **/tests/**/*.{test,spec}.{ts,tsx} : Mirror test file structure to source: tests in `tests/` directory with `unit/`, `integration/`, and `contract/` subdirectories
Applied to files:
packages/testing/package.jsonexamples/minimal/README.mdpackages/testing/src/types.tspackages/testing/src/index.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Zod version must be ^4.0.0
Applied to files:
packages/testing/package.json
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Use `AppError` and `ErrorCode` from `mcp-apps-kit/core` for error handling
Applied to files:
packages/testing/package.jsonpackages/testing/src/eval/mcp/error-injection.tspackages/testing/src/index.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to **/index.ts : Export public API only in `index.ts` files
Applied to files:
packages/testing/src/eval/reporter/index.tspackages/testing/src/index.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Run full checks before PR: `pnpm build && pnpm test && pnpm lint && pnpm typecheck`
Applied to files:
examples/minimal/README.md
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Colocate UI definitions near tool definitions using `defineUI` with `html` property pointing to compiled UI assets
Applied to files:
examples/minimal/README.mdpackages/testing/src/types.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Use Zod schemas with `defineTool` for input/output validation
Applied to files:
packages/testing/src/types.tspackages/testing/src/index.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Implement plugins using the `Plugin` interface with hooks: `onInit`, `onStart`, `onShutdown`, `beforeToolCall`, `afterToolCall`, `onToolError`
Applied to files:
packages/testing/src/types.ts
🧬 Code graph analysis (8)
packages/testing/src/eval/mcp/providers/types.ts (3)
packages/testing/src/eval/mcp/index.ts (6)
ProviderTool(80-80)ProviderToolCall(82-82)ProviderMessage(79-79)ProviderResponse(81-81)ProviderConfig(78-78)LLMProvider(77-77)packages/testing/src/eval/mcp/providers/index.ts (6)
ProviderTool(16-16)ProviderToolCall(18-18)ProviderMessage(15-15)ProviderResponse(17-17)ProviderConfig(14-14)LLMProvider(13-13)packages/ui-react-builder/src/vite-plugin.ts (1)
config(502-514)
packages/testing/src/eval/reporter/types.ts (1)
packages/testing/src/types.ts (2)
EvaluationResult(336-346)EvalOptions(302-311)
packages/testing/src/eval/mcp/providers/anthropic.ts (3)
packages/testing/src/eval/mcp/providers/index.ts (7)
createAnthropicProvider(22-22)ProviderConfig(14-14)LLMProvider(13-13)ProviderMessage(15-15)ProviderTool(16-16)ProviderResponse(17-17)ProviderToolCall(18-18)packages/testing/src/eval/mcp/providers/types.ts (6)
ProviderConfig(52-56)LLMProvider(61-72)ProviderMessage(28-33)ProviderTool(10-14)ProviderResponse(38-47)ProviderToolCall(19-23)packages/testing/src/index.ts (1)
llmLogger(123-123)
packages/testing/src/eval/reporter/reporter.ts (2)
packages/testing/src/eval/reporter/types.ts (3)
EvalReporterOutput(26-30)EvalReportEntry(35-52)EvalReporterOptions(10-21)packages/testing/src/types.ts (3)
CriterionResult(316-325)EvaluationResult(336-346)EvalOptions(302-311)
packages/testing/src/eval/mcp/error-injection.ts (3)
packages/testing/src/eval/mcp/index.ts (5)
ToolErrorConfig(51-51)ErrorInjectionConfig(52-52)ErrorInjectionState(53-53)wrapWithErrorInjection(49-49)resetErrorInjectionState(50-50)packages/testing/src/index.ts (4)
ToolErrorConfig(94-94)ErrorInjectionConfig(95-95)TestClient(27-27)ToolResult(24-24)packages/testing/src/types.ts (2)
TestClient(77-109)ToolResult(27-38)
packages/testing/src/eval/llm/providers/openai.ts (5)
packages/testing/src/utils/lazy-loader.ts (2)
createLazyLoader(47-85)createCachedClientFactory(109-158)packages/testing/src/eval/llm/providers/index.ts (1)
LLMProvider(12-22)packages/testing/src/index.ts (6)
llmLogger(123-123)ConfigurationError(110-110)EvalOptions(47-47)EvaluationResult(50-50)CriteriaResults(49-49)CustomEvalOptions(51-51)packages/testing/src/errors.ts (1)
ConfigurationError(175-187)packages/testing/src/types.ts (4)
EvalOptions(302-311)EvaluationResult(336-346)CriteriaResults(331-331)CustomEvalOptions(373-378)
packages/testing/src/eval/mcp/evaluator.ts (9)
packages/testing/src/eval/mcp/index.ts (21)
ToolCallRecord(22-22)JudgeCriterion(24-24)JudgeOptions(25-25)JudgeResult(23-23)TokenUsage(27-27)MCPEvalResult(21-21)ProviderMessage(79-79)MCPEvalConfig(18-18)ProviderType(76-76)RetryConfig(44-44)ToolErrorConfig(51-51)MCPEvaluator(17-17)MCPSession(38-38)BatchEvalCase(60-60)MCPEval(16-16)detectProvider(75-75)getProviderApiKey(72-72)getDefaultModel(74-74)wrapWithErrorInjection(49-49)ProviderTool(80-80)createSession(38-38)packages/testing/src/eval/index.ts (3)
ToolCallRecord(19-19)MCPEvalResult(19-19)MCPEvalConfig(19-19)packages/testing/src/eval/mcp/providers/index.ts (6)
ProviderMessage(15-15)ProviderType(27-27)detectProvider(101-109)getProviderApiKey(32-41)getDefaultModel(53-62)ProviderTool(16-16)packages/testing/src/eval/mcp/providers/types.ts (2)
ProviderMessage(28-33)ProviderTool(10-14)packages/testing/src/eval/mcp/retry.ts (1)
RetryConfig(12-25)packages/testing/src/eval/mcp/error-injection.ts (2)
ToolErrorConfig(12-21)wrapWithErrorInjection(72-144)packages/testing/src/eval/mcp/session.ts (2)
MCPSession(14-32)createSession(57-121)packages/testing/src/eval/mcp/batch.ts (1)
BatchEvalCase(21-42)packages/testing/src/types.ts (1)
TestClient(77-109)
packages/testing/src/eval/llm/providers/anthropic.ts (5)
packages/testing/src/utils/lazy-loader.ts (2)
createLazyLoader(47-85)createCachedClientFactory(109-158)packages/testing/src/eval/llm/providers/index.ts (1)
LLMProvider(12-22)packages/testing/src/index.ts (6)
llmLogger(123-123)ConfigurationError(110-110)EvalOptions(47-47)EvaluationResult(50-50)CriteriaResults(49-49)CustomEvalOptions(51-51)packages/testing/src/errors.ts (1)
ConfigurationError(175-187)packages/testing/src/types.ts (4)
EvalOptions(302-311)EvaluationResult(336-346)CriteriaResults(331-331)CustomEvalOptions(373-378)
🪛 GitHub Actions: PR Check
packages/testing/src/eval/mcp/providers/anthropic.ts
[warning] 70-70: Unused eslint-disable directive (no problems were reported from '@typescript-eslint/no-explicit-any')
packages/testing/src/eval/reporter/reporter.ts
[warning] 34-40: Unexpected console statement (no-console)
packages/testing/src/eval/mcp/evaluator.ts
[warning] 774-774: Unexpected console statement (no-console).
packages/testing/src/eval/mcp/reporter.ts
[warning] 62-101: Multiple unexpected console statements detected by eslint (no-console)
🪛 markdownlint-cli2 (0.18.1)
examples/minimal/README.md
89-89: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
packages/testing/README.md
724-724: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (43)
examples/minimal/.env.example (1)
1-3: Environment template looks good.Clear placeholder format with helpful guidance comments. The example value won't accidentally work as a real API key.
examples/minimal/README.md (2)
69-99: Testing section is clear and well-structured.The new Testing section properly documents test execution and LLM evaluation setup with helpful environment configuration steps and a note about auto-skipping tests when the API key is missing. This makes it easy for contributors to get started.
100-122: Project structure accurately reflects new testing setup.The updated structure properly documents the new
tests/directory andvitest.config.tsconfiguration, making it clear to users where test files are located and how the testing framework is configured.packages/testing/package.json (3)
1-62: LGTM! Well-structured package configuration.The package metadata, dual ESM/CJS exports, and build configuration follow modern Node.js best practices. The three entry points (root, vitest, jest) are properly configured with types and dual module formats.
63-67: Dependencies and configuration are correct.The core dependencies are appropriate:
- Zod ^4.0.0 matches the required version per project learnings
- MCP SDK and debug are necessary for the testing utilities
- Node >=20 and sideEffects: false are good choices for a modern package
Also applies to: 92-98
68-74: Outdated peer dependency version constraints.The version constraints for
openai,fast-check, andjestare behind current major versions. This issue was already identified in a previous review.Likely an incorrect or invalid review comment.
packages/testing/README.md (1)
1-941: LGTM! Comprehensive and well-structured documentation.The README provides excellent coverage of the testing library's features including test environment setup, assertions/matchers, test suites, property-based testing, UI widget testing, LLM evaluation, and framework integration. The examples are clear and the API reference tables are helpful.
packages/testing/src/server/test-client.ts (2)
37-122: LGTM! Well-implemented retry and timeout handling.The
callToolWithRetryfunction correctly:
- Clears the timeout timer on both success and error paths to prevent leaks
- Only retries on
TimeoutError, preserving other error semantics- Tracks call history with duration and timestamp metadata
- Uses defensive copying when returning history
124-205: LGTM! Clean client interface implementation.The returned client object properly exposes all required methods with appropriate type handling for SDK responses. The
disconnectmethod correctly closes the transport.packages/testing/src/eval/llm/providers/openai.ts (2)
41-50: LGTM! Proper API key validation and error handling.The provider correctly validates the API key at creation time and throws a descriptive
ConfigurationErrorif missing.
151-188: LGTM! Well-structured evaluation prompt builder.The
buildEvaluationPromptfunction cleanly constructs the evaluation prompt with context, criteria, and expected JSON response format.packages/testing/src/types.ts (1)
1-549: LGTM! Well-organized and comprehensive type definitions.The types file provides excellent coverage of all testing utilities with:
- Clear section organization using comment headers
- Proper JSDoc documentation for all interfaces
- No use of
anytypes (usesunknownappropriately where needed)- Consistent naming conventions
The use of
unknownforappinTestEnvironmentOptions(line 166) is a good choice to avoid circular dependencies with@mcp-apps-kit/core.packages/testing/src/eval/reporter/index.ts (1)
1-8: LGTM! Clean public API exports following best practices.The index file correctly:
- Uses
export typefor type-only exports (EvalReporterOptions,EvalReportEntry)- Exposes only the public API surface
- Keeps the module boundary clean
As per coding guidelines for
**/index.tsfiles.packages/testing/src/eval/reporter/types.ts (1)
1-52: LGTM! Well-defined reporter types.The reporter types provide a clean contract for:
- Configurable output with verbose mode, colors, and custom streams
- Structured report entries with comprehensive metadata
- Output abstraction allowing custom logging implementations
packages/testing/src/eval/mcp/cost-utils.ts (2)
23-37: LGTM! Robust cost parsing with good edge case handling.The function correctly handles:
- Undefined/empty input (returns 0)
- Both dollar ($) and cent (¢) formats
- NaN parsing results (returns 0)
- Cent-to-dollar conversion
The JSDoc examples are helpful for understanding expected behavior.
39-48: LGTM! Clean formatting utility.Simple and effective dollar formatting with configurable precision.
packages/testing/src/eval/mcp/providers/openai.ts (2)
27-35: LGTM on lazy loading and provider initialization.The lazy loading pattern for the OpenAI SDK and the fallback handling for different export styles (
default,OpenAI, or the module itself) is well implemented for handling various bundler configurations.
42-129: LGTM oncreateCompletionimplementation.The message and tool conversion logic properly handles all message roles including
toolresponses withtool_call_id. The finish reason determination correctly falls back totool_callswhen tool calls are present even if the API doesn't explicitly return that reason.examples/minimal/tests/eval.test.ts (3)
28-35: Good practice clearing and printing global collector.Clearing previous results before tests and printing summary after ensures clean test runs and useful output aggregation.
41-99: LGTM on basic evaluation tests.The tests properly verify tool calls with correct arguments, token usage tracking, and LLM judgment. Good coverage of single and multiple tool call scenarios.
280-341: LGTM on error injection tests.Good coverage of both global mock errors and per-run error injection. The tests properly verify that errors are handled gracefully and the LLM reports failures to users.
packages/testing/src/eval/index.ts (1)
1-23: LGTM on module exports.The barrel file properly uses
export typefor type-only exports as per coding guidelines, and cleanly organizes the public API surface for the evaluation module.packages/testing/src/eval/reporter/reporter.ts (2)
47-54: LGTM on color detection.Proper handling of
NO_COLORstandard, CI environments, and TTY detection for determining whether to use ANSI colors.
59-279: LGTM on EvalReporter implementation.The reporter provides good functionality including:
- Configurable verbose mode with full/truncated output
- Per-criterion formatting with pass/fail icons and scores
- Summary aggregation with failed test listing
- Immutable returns via spread operators in
getEntries()andgetSummary()packages/testing/src/eval/mcp/providers/types.ts (1)
1-77: LGTM on provider type definitions.Well-structured provider-agnostic types with:
- Clear JSDoc documentation
- Strict typing without
any(usesRecord<string, unknown>)- Proper separation of concerns between tool definitions, messages, responses, and provider configuration
- Extensible
nameliteral union for future providerspackages/testing/src/eval/mcp/session.ts (1)
57-105: LGTM on session state management.Good implementation patterns:
- Internal state encapsulated in closure
- Immutable returns via spread operators in getters
- Proper aggregation of token usage and estimated costs across turns
packages/testing/src/eval/llm/providers/anthropic.ts (2)
43-52: LGTM on API key validation.Proper error handling with
ConfigurationErrorwhen API key is missing, following the project's error handling patterns as per learnings.
248-282: LGTM on prompt construction.The
buildEvaluationPromptfunction creates well-structured prompts with clear JSON output format instructions, including context information when available.packages/testing/src/eval/mcp/providers/index.ts (1)
1-109: LGTM!The provider factory implementation is well-structured with clear separation of concerns. The lazy provider detection pattern is appropriate for a testing library, and the re-exports follow the index.ts public API pattern correctly.
packages/testing/src/eval/mcp/providers/anthropic.ts (1)
29-44: LGTM on lazy loading and SDK instantiation.The dynamic import pattern with fallback for different SDK export shapes handles version compatibility well. The eslint-disable blocks are appropriately scoped and justified in comments.
packages/testing/src/eval/mcp/error-injection.ts (2)
72-144: LGTM!The error injection wrapper correctly implements:
- Call count tracking before incrementing (allowing errorCount to work as "fail N times then succeed")
- Probability-based error injection with configurable thresholds
- Clean delegation pattern for read-only methods
- Reset capability via the returned interface
186-204: Good deprecation pattern.The deprecated function properly uses underscore prefix for the unused parameter and provides clear migration guidance in the JSDoc. Consider removing it in a future major version.
packages/testing/src/eval/mcp/evaluator.ts (4)
586-621: Closure mutatestotalUsageafter result is returned.The
judge()method modifiestotalUsage(lines 588-591) via closure after the result object is already returned. This meansresult.usagewon't reflect judge token costs if accessed beforejudge()is called, but subsequent calls will accumulate. This is likely intentional for multi-call scenarios but could surprise users.Consider documenting this behavior or returning a fresh usage object from
judge().
766-794: LGTM ondescribeEvalimplementation.The auto-skip pattern for missing LLM provider keys is a good DX improvement. The console.warn on line 774 is appropriate for alerting users when no test framework is detected—this is intentional diagnostic output, not debug logging.
818-851: Random port allocation for test isolation.The port range 3100-3999 is reasonable for parallel test execution. The 100ms delay after server start (line 826) is a simple readiness wait—consider using a more robust health check if flakiness occurs.
438-442: Good infinite loop protection.The
maxIterations = 10safety limit prevents runaway tool call loops. This is a sensible default for evaluation scenarios.packages/testing/src/eval/mcp/reporter.ts (1)
50-260: LGTM on MCPEvalReporter class.The reporter provides well-structured output with appropriate color handling, TTY detection, and CI environment awareness. Console statements are expected and appropriate for this reporting module's purpose.
packages/testing/src/eval/mcp/retry.ts (3)
203-224: Potential token accounting edge case in RateLimiter.After waiting for a token (lines 217-219), the refill calculation uses
waitTimewhich was computed before sleeping. If actual sleep duration differs (common due to timer precision), token accounting could drift slightly. For a testing library this is acceptable, but worth noting.
149-182: LGTM on withRetry implementation.The retry logic correctly:
- Checks
isTransientErrorbefore retrying- Uses configurable backoff strategies with jitter
- Respects
maxAttemptslimit- The "unreachable" throw on line 180 satisfies TypeScript's control flow analysis
290-317: LGTM on createResilientWrapper.The composition order (rate limit → timeout → retry) is correct: rate limiting gates entry, timeout bounds individual attempts, and retry handles transient failures across attempts.
packages/testing/src/eval/mcp/batch.ts (2)
122-163: LGTM on tool assertion logic.The
checkToolAssertionhelper correctly validates:
- Tool call count when specified
- Tool name presence
- Partial argument matching (only checking specified keys)
The partial match design is appropriate for flexible test assertions.
190-362: LGTM on runBatch implementation.The batch runner handles both parallel and sequential execution modes correctly, with proper progress reporting, cost aggregation, and summary generation. The sequential mode's
stopOnFailureoption provides useful control for fail-fast scenarios.packages/testing/src/index.ts (1)
1-175: Excellent public API organization and TypeScript best practices.This index file demonstrates strong TypeScript patterns and excellent organization:
- ✅ Correctly uses
export typefor type-only exports (lines 21-97, 138) and regularexportfor runtime values/classes (lines 103-171)- ✅ Clear section markers separate concerns (types, errors, debug, main API)
- ✅ Properly splits value and type exports from the same module (e.g., lines 137-138 from "./eval/behavior")
- ✅ Comprehensive JSDoc with usage example (lines 1-15)
- ✅ Documents subpath exports in comments (lines 173-175)
The file follows all coding guidelines and learnings, providing a clean, well-structured public API surface.
As per coding guidelines: correctly exports only public API and uses
export typefor type-only exports.
|
|
||
| ``` | ||
| OPENAI_API_KEY=sk-your-actual-api-key | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced code block.
The code fence starting at line 88 is missing a language specifier. While this is a minor markdown linting issue, it should be fixed for consistency with the other code block at line 73.
📝 Proposed fix for markdown formatting
2. Add your OpenAI API key to `.env`:
- ```
+ ```bash
OPENAI_API_KEY=sk-your-actual-api-key
```🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
89-89: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @examples/minimal/README.md around lines 88 - 91, The fenced code block
containing "OPENAI_API_KEY=sk-your-actual-api-key" is missing a language
identifier; update the opening triple-backtick for that block to include a
language (e.g., "bash") so it matches the other block and satisfies markdown
linting/consistency.
| // Parse response | ||
| const parsed = JSON.parse(content) as { | ||
| criteria: Array<{ name: string; score: number; explanation: string }>; | ||
| overall: { score: number }; | ||
| }; | ||
|
|
||
| // Build evaluation result as a record keyed by criterion name | ||
| const criteriaResults: CriteriaResults = {}; | ||
| for (const criterion of options.criteria) { | ||
| const result = parsed.criteria.find((c) => c.name === criterion.name); | ||
| if (!result) { | ||
| throw new Error(`Missing evaluation result for criterion: ${criterion.name}`); | ||
| } | ||
|
|
||
| const threshold = criterion.threshold ?? 0.7; | ||
| criteriaResults[criterion.name] = { | ||
| name: criterion.name, | ||
| score: result.score, | ||
| pass: result.score >= threshold, | ||
| explanation: result.explanation, | ||
| }; | ||
| } | ||
|
|
||
| const overallScore = parsed.overall.score; | ||
| const passThreshold = 0.7; // Default overall threshold | ||
| const overallPass = overallScore >= passThreshold; |
There was a problem hiding this comment.
Add runtime validation for parsed JSON response structure.
The parsed JSON response is cast directly to the expected type without validation. If the LLM returns valid JSON with an unexpected structure (e.g., missing criteria array or overall object), this will cause confusing runtime errors like TypeError: Cannot read properties of undefined.
🔧 Suggested validation pattern
// Parse response
- const parsed = JSON.parse(content) as {
- criteria: Array<{ name: string; score: number; explanation: string }>;
- overall: { score: number };
- };
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(content);
+ } catch {
+ throw new Error(`Invalid JSON response from OpenAI: ${content.slice(0, 200)}`);
+ }
+
+ // Validate structure
+ const parsedObj = parsed as Record<string, unknown>;
+ if (!Array.isArray(parsedObj.criteria)) {
+ throw new Error(`Invalid response structure: missing or invalid 'criteria' array`);
+ }
+ if (!parsedObj.overall || typeof (parsedObj.overall as Record<string, unknown>).score !== "number") {
+ throw new Error(`Invalid response structure: missing or invalid 'overall.score'`);
+ }
+
+ const criteriaArray = parsedObj.criteria as Array<{ name: string; score: number; explanation: string }>;
+ const overallScore = (parsedObj.overall as { score: number }).score;
// Build evaluation result as a record keyed by criterion name
const criteriaResults: CriteriaResults = {};
for (const criterion of options.criteria) {
- const result = parsed.criteria.find((c) => c.name === criterion.name);
+ const result = criteriaArray.find((c) => c.name === criterion.name);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @packages/testing/src/eval/llm/providers/openai.ts around lines 84 - 109, The
code blindly casts JSON.parse(content) to the expected shape and then assumes
parsed.criteria and parsed.overall exist; add runtime validation after parsing
to guard against malformed responses by checking parsed is an object,
parsed.criteria is an array, each parsed.criteria item has name (string), score
(number) and explanation (string), and parsed.overall exists with a numeric
score before using them in the loop that builds CriteriaResults and before
computing overallScore; if any check fails, throw a clear Error that includes
the raw content (or a snippet) and a message like "Invalid LLM response: missing
or malformed 'criteria'/'overall'". Ensure you reference and validate parsed
(the variable from JSON.parse), parsed.criteria, parsed.overall, and the loop
over options.criteria/CriteriaResults to avoid TypeError at runtime.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const content: any[] = []; |
There was a problem hiding this comment.
Remove unused eslint-disable directive.
The pipeline indicates this eslint-disable is unnecessary. Remove it to keep the codebase clean.
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
const content: any[] = [];📝 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.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| const content: any[] = []; | |
| const content: any[] = []; |
🧰 Tools
🪛 GitHub Actions: PR Check
[warning] 70-70: Unused eslint-disable directive (no problems were reported from '@typescript-eslint/no-explicit-any')
🤖 Prompt for AI Agents
In @packages/testing/src/eval/mcp/providers/anthropic.ts around lines 70 - 71,
Remove the unused eslint-disable directive preceding the declaration of the
content array: delete the line "// eslint-disable-next-line
@typescript-eslint/no-explicit-any" and leave the declaration "const content:
any[] = [];" as-is (or replace the any type with a more specific type if
desired) so the file no longer contains the unnecessary eslint-disable comment.
| end(): void { | ||
| this.reset(); | ||
| }, |
There was a problem hiding this comment.
Potential issue with this context in end() method.
Using this.reset() inside an object literal method relies on the caller invoking end() with the correct this context. If end is destructured or passed as a callback, this may be undefined.
🔧 Safer implementation
end(): void {
- this.reset();
+ history = [];
+ results = [];
+ aggregatedUsage = {
+ promptTokens: 0,
+ completionTokens: 0,
+ totalTokens: 0,
+ };
},Alternatively, define reset as a closure variable and call it directly:
+ const resetSession = (): void => {
+ history = [];
+ results = [];
+ aggregatedUsage = {
+ promptTokens: 0,
+ completionTokens: 0,
+ totalTokens: 0,
+ };
+ };
+
return {
// ... other methods ...
reset(): void {
- history = [];
- results = [];
- aggregatedUsage = {
- promptTokens: 0,
- completionTokens: 0,
- totalTokens: 0,
- };
+ resetSession();
},
end(): void {
- this.reset();
+ resetSession();
},
};📝 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.
| end(): void { | |
| this.reset(); | |
| }, | |
| end(): void { | |
| history = []; | |
| results = []; | |
| aggregatedUsage = { | |
| promptTokens: 0, | |
| completionTokens: 0, | |
| totalTokens: 0, | |
| }; | |
| }, |
🤖 Prompt for AI Agents
In @packages/testing/src/eval/mcp/session.ts around lines 117 - 119, The end()
method calls this.reset() which will break if end is extracted or used as a
callback; change its implementation so it doesn't rely on dynamic this: either
convert end into a bound arrow property (e.g., end = () => { this.reset(); }) or
move reset into a closure/local function and call that directly from end; update
the declaration of end() in the same object/class where reset() is defined to
use one of these approaches so calling end without the original this still
invokes reset.
| const defaultOutput: EvalReporterOutput = { | ||
| log: (msg: string) => { | ||
| console.log(msg); | ||
| }, | ||
| warn: (msg: string) => { | ||
| console.warn(msg); | ||
| }, | ||
| error: (msg: string) => { | ||
| console.error(msg); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Address ESLint no-console warning from pipeline.
The pipeline reports a warning for unexpected console statements. Since this is a reporter that intentionally outputs to console, consider suppressing the lint rule for this specific block.
🔧 Suggested fix
/**
* Default console output implementation
*/
+/* eslint-disable no-console */
const defaultOutput: EvalReporterOutput = {
log: (msg: string) => {
console.log(msg);
},
warn: (msg: string) => {
console.warn(msg);
},
error: (msg: string) => {
console.error(msg);
},
};
+/* eslint-enable no-console */📝 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.
| const defaultOutput: EvalReporterOutput = { | |
| log: (msg: string) => { | |
| console.log(msg); | |
| }, | |
| warn: (msg: string) => { | |
| console.warn(msg); | |
| }, | |
| error: (msg: string) => { | |
| console.error(msg); | |
| }, | |
| }; | |
| /* eslint-disable no-console */ | |
| const defaultOutput: EvalReporterOutput = { | |
| log: (msg: string) => { | |
| console.log(msg); | |
| }, | |
| warn: (msg: string) => { | |
| console.warn(msg); | |
| }, | |
| error: (msg: string) => { | |
| console.error(msg); | |
| }, | |
| }; | |
| /* eslint-enable no-console */ |
🧰 Tools
🪛 GitHub Actions: PR Check
[warning] 34-40: Unexpected console statement (no-console)
🤖 Prompt for AI Agents
In @packages/testing/src/eval/reporter/reporter.ts around lines 32 - 42, The
ESLint no-console warning comes from the console calls in the defaultOutput
object; suppress it by disabling the rule only for this block (e.g., add a
block-level suppression like /* eslint-disable no-console */ immediately before
the defaultOutput definition and /* eslint-enable no-console */ immediately
after) so the EvalReporterOutput defaultOutput (log/warn/error) can
intentionally use console without triggering pipeline warnings.
| /** | ||
| * Summary of all evaluations | ||
| */ | ||
| export interface EvalReportSummary { | ||
| /** Total evaluations run */ | ||
| total: number; | ||
| /** Evaluations that passed */ | ||
| passed: number; | ||
| /** Evaluations that failed */ | ||
| failed: number; | ||
| /** Average overall score */ | ||
| averageScore: number; | ||
| /** Total duration in ms */ | ||
| totalDuration: number; | ||
| /** Individual entries */ | ||
| entries: EvalReportEntry[]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check usages of EvalReportSummary across the codebase
rg -n "EvalReportSummary" --type=ts -C2Repository: AndurilCode/mcp-apps-kit
Length of output: 1632
🏁 Script executed:
# Check what's exported from the testing package's index.ts
fd -n "index.ts" packages/testing/src -type f | head -5Repository: AndurilCode/mcp-apps-kit
Length of output: 298
🏁 Script executed:
# Examine the main index.ts files in the testing package
find packages/testing/src -name "index.ts" -type f | xargs catRepository: AndurilCode/mcp-apps-kit
Length of output: 12319
Export EvalReportSummary from the public API.
EvalReportSummary is the return type of public methods printSummary() and getSummary() on the exported EvalReporter class, but it's not exported from packages/testing/src/index.ts. Consumers need access to this type to properly type their code. Add it to the eval/reporter exports in index.ts.
Additionally, consider using export type syntax instead of export interface to explicitly mark it as a type-only export, per the TypeScript guidelines.
🤖 Prompt for AI Agents
In @packages/testing/src/eval/reporter/types.ts around lines 54 - 70, The
EvalReportSummary type is used as the return type for EvalReporter methods
printSummary() and getSummary() but isn't exported from the package public API;
export it from the eval/reporter exports in your public index (add
EvalReportSummary to the exported symbols alongside EvalReporter) so consumers
can import it, and optionally convert the declaration from "export interface
EvalReportSummary" to "export type EvalReportSummary = { ... }" to mark it as a
type-only export per TypeScript guidelines.
- Clean up the error-injection.ts file by removing unnecessary trailing whitespace, improving code readability and consistency.
Code Review: Testing Library ImplementationThis PR introduces a comprehensive testing framework for MCP applications. Overall, this is a high-quality implementation with excellent architecture and thorough testing. Below is detailed feedback organized by category. ✅ StrengthsArchitecture & Design
Code Quality
Testing
🔍 Issues & Concerns1. Security: Command Injection Risk
|
Code Review: Testing Library ImplementationOverall AssessmentThis is an impressive and comprehensive testing library implementation. The code is well-structured, thoroughly documented, and demonstrates strong engineering practices. The scope is ambitious (~13,800 additions) but well-executed. ✅ Strengths1. Excellent Architecture
2. Documentation
3. Code Quality
4. Testing Coverage
🔍 Critical Issues1. Use of
2. Console.log usage
3. Error swallowing in lazy-loader
4. Race condition in cachedClientFactory
📋 Medium Priority5. Peer dependency version
6. Test environment detection
7. Hardcoded LLM pricing
🎯 Security & Best Practices ✅
Actionable SummaryMust Fix (2-3 hours):
Should Fix: Nice to Have: 🎉 ConclusionProduction-ready with minor fixes needed. Excellent architecture, solid testing, great documentation. Once critical items addressed, this will be a valuable addition to mcp-apps-kit. Test coverage is exemplary. Maintain 80% target per CLAUDE.md. Great work! 🚀 |
2 participants
Note
Introduces a comprehensive testing library and wires up example tests across the repo.
@mcp-apps-kit/testingpackage with server/client test utilities (startTestServer,createTestClient), behavior assertions (expectToolResult), declarative suites (defineTestSuite/runTestSuite), property-based testing (generators,forAllInputs), LLM evaluation (createLLMEvaluator,criteria), UI testing (createMockHost,createTestEnvironment,TestEnvironmentBuilder), Vitest/Jest adapters, structured types, debug loggers, and error typespackages/testing/README.mdand extensive unit/integration tests underpackages/testing/testsexamples/minimalto be testable: exportsapp, guards server start in tests, adds Vitest config/setup, and new tests for v1/v2 tools, versioning, integration, advanced features, and React UI widgetsexamples/minimal/package.json, extendseslint.config.jsto includepackages/testing, and ignores.cursor/in.gitignoreWritten by Cursor Bugbot for commit da29196. Configure here.