Add comprehensive test suite with vitest#66
Add comprehensive test suite with vitest#66louisabraham wants to merge 5 commits intodelorenj:mainfrom
Conversation
- copy_card: duplicate a card to any list (cross-board) via Trello's idCardSource API, with configurable keepFromSource properties - copy_checklist: copy a checklist with all items to another card (cross-board) via idChecklistSource API - add_cards_to_list: create multiple cards in a single tool call with sequential API requests and automatic rate limiting Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- 121 unit tests covering rate-limiter, validators, and trello-client - 26 smoke tests against live Trello API (skipped when no credentials) - Add vitest as test framework with test scripts in package.json - Tests cover all existing tools plus new copy_card, copy_checklist, and add_cards_to_list features Bug found during testing: some tools (set_active_board, delete_comment, delete_label) return plain text instead of JSON, inconsistent with other tools that return JSON objects. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello @louisabraham, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's robustness and functionality by introducing a comprehensive testing framework and new Trello interaction capabilities. The addition of unit and smoke tests ensures greater reliability across core components and new features, while the new tools expand the range of automated Trello operations available. This change improves overall code quality and provides a more stable foundation for future development. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive test suite using vitest, which is a great addition for ensuring code quality and stability. The new features for copying cards/checklists and batch-adding cards are also well-implemented. The tests are thorough, covering unit tests with mocks and smoke tests against the live API. I've identified a few areas for improvement, mainly around making the code more robust and efficient.
src/index.ts
Outdated
| cards: z | ||
| .array( | ||
| z.object({ | ||
| name: z.string().describe('Name of the card'), | ||
| description: z.string().optional().describe('Description of the card'), | ||
| dueDate: z | ||
| .string() | ||
| .optional() | ||
| .describe('Due date for the card (ISO 8601 format)'), | ||
| start: z | ||
| .string() | ||
| .optional() | ||
| .describe('Start date for the card (YYYY-MM-DD format)'), | ||
| labels: z | ||
| .array(z.string()) | ||
| .optional() | ||
| .describe('Array of label IDs to apply to the card'), | ||
| }) | ||
| ) | ||
| .describe('Array of cards to create'), |
There was a problem hiding this comment.
The cards array in the add_cards_to_list tool's input schema can be empty. While not a critical issue, allowing an empty array could lead to a successful tool call that performs no action, which might be confusing for the user. It's better to enforce that at least one card is provided.
cards: z
.array(
z.object({
name: z.string().describe('Name of the card'),
description: z.string().optional().describe('Description of the card'),
dueDate: z
.string()
.optional()
.describe('Due date for the card (ISO 8601 format)'),
start: z
.string()
.optional()
.describe('Start date for the card (YYYY-MM-DD format)'),
labels: z
.array(z.string())
.optional()
.describe('Array of label IDs to apply to the card'),
})
)
.min(1, { message: 'Must provide at least one card to create.' })
.describe('Array of cards to create'),
src/trello-client.ts
Outdated
| for (const card of cards) { | ||
| const result = await this.addCard(undefined, { | ||
| listId, | ||
| name: card.name, | ||
| description: card.description, | ||
| dueDate: card.dueDate, | ||
| start: card.start, | ||
| labels: card.labels, | ||
| }); | ||
| results.push(result); | ||
| } | ||
| return results; | ||
| } |
There was a problem hiding this comment.
The current implementation of batchAddCards uses a sequential for...of loop with await, which adds cards one by one. This can be inefficient for a large number of cards. You can improve performance by executing the card creation requests in parallel using Promise.all. The existing rate limiter will automatically handle the concurrent requests and queue them as needed.
| for (const card of cards) { | |
| const result = await this.addCard(undefined, { | |
| listId, | |
| name: card.name, | |
| description: card.description, | |
| dueDate: card.dueDate, | |
| start: card.start, | |
| labels: card.labels, | |
| }); | |
| results.push(result); | |
| } | |
| return results; | |
| } | |
| const cardPromises = cards.map(card => | |
| this.addCard(undefined, { | |
| listId, | |
| name: card.name, | |
| description: card.description, | |
| dueDate: card.dueDate, | |
| start: card.start, | |
| labels: card.labels, | |
| }) | |
| ); | |
| return Promise.all(cardPromises); |
tests/smoke/smoke.test.ts
Outdated
| const TEST_BOARD_ID = process.env.TRELLO_TEST_BOARD_ID || '698bd319de70fa4f7a3c7ccd'; | ||
|
|
||
| const canRunSmoke = Boolean(TRELLO_API_KEY && TRELLO_TOKEN); |
There was a problem hiding this comment.
The smoke tests include a hardcoded fallback for TEST_BOARD_ID. This is not ideal as it can lead to flaky tests or unintended side effects if multiple developers run tests against the same shared board. It's better to rely exclusively on environment variables for configuration and skip the tests if they are not provided. This makes the tests more portable and safer to run in a collaborative environment.
| const TEST_BOARD_ID = process.env.TRELLO_TEST_BOARD_ID || '698bd319de70fa4f7a3c7ccd'; | |
| const canRunSmoke = Boolean(TRELLO_API_KEY && TRELLO_TOKEN); | |
| const TEST_BOARD_ID = process.env.TRELLO_TEST_BOARD_ID; | |
| const canRunSmoke = Boolean(TRELLO_API_KEY && TRELLO_TOKEN && TEST_BOARD_ID); |
There was a problem hiding this comment.
Pull request overview
This PR introduces a Vitest-based test suite for the Trello MCP server, adding broad unit coverage for core modules plus optional live “smoke” tests against the Trello API. It also adds three new user-facing tools (copy_card, copy_checklist, add_cards_to_list) with corresponding client methods.
Changes:
- Add Vitest configuration and npm scripts for unit and smoke test runs
- Add unit tests for
validators,rate-limiter, andtrello-client(with mocked axios/fs) - Add new Trello features/tools: copy card, copy checklist, and batch-add cards to a list (plus live smoke tests)
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Adds Vitest runner configuration (globals, include pattern, timeout). |
| tests/unit/validators.test.ts | Unit tests for primitive + request validator helpers. |
| tests/unit/trello-client.test.ts | Unit tests for TrelloClient behaviors using mocked axios/rate-limiter/fs. |
| tests/unit/rate-limiter.test.ts | Unit tests for token-bucket limiter timing/consumption behavior. |
| tests/smoke/smoke.test.ts | Live Trello API smoke tests via spawning the built MCP server and calling tools over stdio JSON-RPC. |
| src/trello-client.ts | Adds copyCard, copyChecklist, and batchAddCards methods. |
| src/index.ts | Registers new MCP tools: copy_card, copy_checklist, add_cards_to_list. |
| package.json | Adds vitest devDependency and test:* scripts. |
| package-lock.json | Lockfile updates for Vitest/Vite dependency tree. |
tests/smoke/smoke.test.ts
Outdated
| @@ -0,0 +1,448 @@ | |||
| import { describe, it, expect, beforeAll } from 'vitest'; | |||
There was a problem hiding this comment.
afterAll is used later in this file but isn’t imported from vitest, which will cause the smoke test suite to fail to compile/run. Add afterAll to the import list (or remove the hook).
| import { describe, it, expect, beforeAll } from 'vitest'; | |
| import { describe, it, expect, beforeAll, afterAll } from 'vitest'; |
tests/smoke/smoke.test.ts
Outdated
| const TEST_BOARD_ID = process.env.TRELLO_TEST_BOARD_ID || '698bd319de70fa4f7a3c7ccd'; | ||
|
|
||
| const canRunSmoke = Boolean(TRELLO_API_KEY && TRELLO_TOKEN); |
There was a problem hiding this comment.
The smoke tests default TEST_BOARD_ID to a hard-coded board ID. If someone runs smoke tests with credentials but forgets to set TRELLO_TEST_BOARD_ID, these tests will create/archive data on an arbitrary board. Consider requiring TRELLO_TEST_BOARD_ID to be set (and skip otherwise), or default to a clearly local/test board created during setup.
| "prettier": "^3.7.4", | ||
| "terser": "^5.44.1", | ||
| "typescript": "^5.9.3" | ||
| "typescript": "^5.9.3", | ||
| "vitest": "^4.0.18" | ||
| }, |
There was a problem hiding this comment.
Adding vitest@^4.0.18 implicitly raises the minimum Node.js version required to run tests/install devDependencies (per the lockfile, Vitest requires Node ^20.0.0 || ^22.0.0 || >=24.0.0, and it pulls in Vite with a Node 20+ engine). If this repo is meant to keep npx/Node workflows working on older Node versions, consider either (a) pinning to a Vitest/Vite version compatible with the supported Node range, or (b) explicitly documenting/enforcing the required Node version via engines.node and CI.
src/trello-client.ts
Outdated
| const results: TrelloCard[] = []; | ||
| for (const card of cards) { | ||
| const result = await this.addCard(undefined, { | ||
| listId, | ||
| name: card.name, | ||
| description: card.description, | ||
| dueDate: card.dueDate, | ||
| start: card.start, | ||
| labels: card.labels, | ||
| }); | ||
| results.push(result); | ||
| } | ||
| return results; |
There was a problem hiding this comment.
batchAddCards is non-atomic: if a later addCard call fails, earlier cards have already been created but the error aborts without returning which cards succeeded. To make this safer for users/ops, consider returning partial results + per-card errors, or adding an option like continueOnError / rollbackOnFailure (best-effort archive) so callers can avoid leaving orphaned cards.
| // Copy a card (supports cross-board copy) | ||
| this.server.registerTool( | ||
| 'copy_card', | ||
| { | ||
| title: 'Copy Card', | ||
| description: | ||
| 'Copy/duplicate a Trello card to any list (even on a different board). Copies all properties by default including checklists, attachments, comments, labels, etc.', | ||
| inputSchema: { |
There was a problem hiding this comment.
New user-facing tools (copy_card, copy_checklist, add_cards_to_list) are added here, but the README’s “Available Tools” section appears to be the canonical tool documentation and currently doesn’t mention them. Please update README.md to document these tools (arguments + return shape) so users can discover and use them.
|
Thanks for the reviews! Addressed the feedback in https://github.com/louisabraham/mcp-server-trello/tree/anthony: Fixed:
Note on Node version: vitest@4 does require Node 20+, which is reasonable for a dev dependency. The runtime itself (bun) has its own requirements. |
Return {created, errors} so callers see which cards succeeded even
if some fail. Enforce a 50-card maximum to prevent long-running calls.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tests
- Import afterAll from vitest (was used but not imported)
- Require TRELLO_TEST_BOARD_ID env var instead of hardcoding
- Update test assertions for new batchAddCards {created, errors} return
- Add tests for partial failure handling and card limit
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Uses dotenv to load TRELLO_API_KEY, TRELLO_TOKEN, and TRELLO_TEST_BOARD_ID from .env so smoke tests can run with just `npm run test:smoke` (no manual env vars needed). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Update: Added Setup: Create a Then run: npm run test # all tests (unit + smoke)
npm run test:unit # unit tests only (no credentials needed)
npm run test:smoke # smoke tests only (requires .env)The |
Summary
rate-limiter,validators, andtrello-client(with mocked axios)TRELLO_API_KEY/TRELLO_TOKENenv vars are absent)vitestas the test framework with scripts:npm run test,npm run test:unit,npm run test:smokecopy_card,copy_checklist, andadd_cards_to_listfeaturesBug found during testing
Some tools return plain text instead of JSON, inconsistent with the rest of the codebase:
set_active_boardreturns"Successfully set active board to..."delete_comment/delete_labelreturn"success"/"Label deleted successfully"JSON.stringify(result)This is a minor inconsistency (not a breaking issue), noted for future cleanup.
Test plan
🤖 Generated with Claude Code