chore: add tests, prettier, pathalias #8
Conversation
|
""" WalkthroughThis update introduces new and revised GitHub Actions workflows, pins the Bun runtime to version 1.2.17, and adds comprehensive test suites for geocoding tools. It establishes Prettier formatting with configuration and scripts, adopts path aliasing in TypeScript, and standardizes import styles and string delimiters throughout the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub Actions
participant Bun
participant Test Suite
Developer->>GitHub Actions: Push or PR triggers workflows
GitHub Actions->>Bun: Setup Bun v1.2.17
GitHub Actions->>GitHub Actions: Checkout code
GitHub Actions->>GitHub Actions: Install dependencies (bun install --frozen-lockfile)
GitHub Actions->>Test Suite: Run tests (bun test)
Test Suite-->>GitHub Actions: Report results
GitHub Actions-->>Developer: Show CI status
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
.github/workflows/test.yml (1)
14-23: Optional: add dependency caching to speed up CI
bun installre-downloads the store on every run. A smallactions/cachestep (keyed bybun.lockb) shaves ~30-40 s off each PR run.- name: Setup Bun uses: oven-sh/setup-bun@v2 with: bun-version: 1.2.17 + + - name: Cache Bun dependencies + uses: actions/cache@v4 + with: + path: ~/.bun/install/cache + key: bun-${{ hashFiles('bun.lockb') }} + restore-keys: | + bun-[nice_to_have]
.github/workflows/lint.yml (1)
14-20: Keep the Bun setup action consistent & freeze the lock-fileTwo tiny inconsistencies with the other workflows:
oven-sh/setup-bunis still on v1 here.bun installruns without--frozen-lockfile, so CI could silently mutatebun.lockb.- - name: Setup Bun - uses: oven-sh/setup-bun@v1 + - name: Setup Bun + uses: oven-sh/setup-bun@v2 ... - - name: Install dependencies - run: bun install + - name: Install dependencies + run: bun install --frozen-lockfilepackage.json (1)
37-47: Formatting script: broaden the glob or run prettier on staged files
"prettier --write \"**/*.ts\""skips.tsx,.js,.json, etc.
If you intend full-repo formatting, consider:- "format": "prettier --write \"**/*.ts\"", + "format": "prettier --write .",or add a separate
format:checkscript for CI (--check).src/index.ts (1)
21-21: Consider downgrading this to informational logging
console.erroris generally reserved for errors; here it’s used for a healthy startup message.
Usingconsole.log(or a dedicated logger at “info” level) keeps stderr clean for real failures, which matters when other processes consume stdout/stderr separately.src/clients/__tests__/nominatimClient.test.ts (3)
2-2: Consider using a more conventional path for package.json import.The import path
@/../package.jsonis unusual. Consider using a direct relative path../../package.jsonor configuring the path alias to point directly to the project root.-import packageJson from '@/../package.json' with { type: 'json' } +import packageJson from '../../package.json' with { type: 'json' }
14-32: Document the testing strategy for skipped axios-dependent tests.The skipped tests are well documented, but consider adding a TODO or issue reference to track when these tests should be re-enabled once axios mocking is resolved.
Would you like me to help create an issue to track implementing these skipped tests or explore alternative mocking strategies for axios with Bun?
35-44: Enhance User-Agent test to verify actual usage.The current test only verifies string construction but doesn't test that the User-Agent header is actually set in HTTP requests. Consider testing the integration with axios configuration.
Consider adding a test that verifies the User-Agent is properly configured in the axios instance:
describe('User-Agent', () => { it('should correctly form User-Agent string based on package.json', () => { const expectedUserAgent = `GeocodingMCP github.com/geocoding-ai/mcp ${packageJson.version}` - // This test primarily verifies the construction of the USER_AGENT string itself. - // The actual header being set by axios relies on axios's internal mechanisms. expect(expectedUserAgent).toContain( 'GeocodingMCP github.com/geocoding-ai/mcp', ) expect(expectedUserAgent).toContain(packageJson.version) }) + + // TODO: Add test to verify User-Agent header is set in axios requests + // when axios mocking is resolved })src/tools/__tests__/reverseGeocode.test.ts (1)
27-39: Consider removing unnecessary geocodeAddress mock.The mock for
geocodeAddresson lines 29-31 appears to be precautionary but may not be necessary since this test file only focuses on reverse geocoding functionality.// Mock only nominatimClient mock.module('@/clients/nominatimClient.js', () => ({ - // Also mock geocodeAddress to avoid interference if it's imported by other modules under test - geocodeAddress: mock(async (params: any) => [ - { place_id: 999, display_name: `Geocode mock for ${params.query}` }, - ]), reverseGeocode: mock(async (params: ReverseGeocodeParams) => { // Default successful mock implementation return { place_id: 456, display_name: `Address for ${params.lat},${params.lon}`, } }), }))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.github/workflows/lint.yml(1 hunks).github/workflows/release.yml(1 hunks).github/workflows/test.yml(1 hunks)package.json(2 hunks)prettier.config.ts(1 hunks)src/clients/__tests__/nominatimClient.test.ts(1 hunks)src/clients/nominatimClient.ts(2 hunks)src/index.ts(2 hunks)src/tools/__tests__/geocode.test.ts(1 hunks)src/tools/__tests__/prepareResponse.test.ts(1 hunks)src/tools/__tests__/reverseGeocode.test.ts(1 hunks)src/tools/geocode.ts(1 hunks)src/tools/prepareResponse.ts(2 hunks)src/tools/reverseGeocode.ts(1 hunks)src/types/commonTypes.ts(1 hunks)src/types/geocodeTypes.ts(2 hunks)src/types/reverseGeocodeTypes.ts(2 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/tools/__tests__/prepareResponse.test.ts (1)
src/tools/prepareResponse.ts (1)
handleGeocodeResult(3-24)
src/tools/__tests__/geocode.test.ts (3)
src/types/geocodeTypes.ts (1)
GeocodeParams(12-12)src/tools/geocode.ts (1)
registerGeocodeTool(9-54)src/tools/prepareResponse.ts (1)
handleGeocodeResult(3-24)
🔇 Additional comments (18)
.github/workflows/release.yml (1)
19-23: Bun version pin is 👍 — consider aligning the action major version across all workflowsYou’re using
oven-sh/setup-bun@v2here, while.github/workflows/lint.ymlis still on@v1. Keeping the same major tag everywhere avoids surprises when v1/v2 diverge in behaviour or cache keys.
[ suggest_nitpick ]tsconfig.json (1)
27-32: Path-alias config looks good
baseUrl+@/*mapping is correct and plays nicely withmoduleResolution: "NodeNext". No issues spotted.src/index.ts (1)
3-6: Ensure runtime resolver knows the@/aliasThe switch from relative paths to
@/…relies on a custom path-mapping intsconfig.json.
Make sure the same mapping is honoured at runtime (e.g.ts-nodewithtsconfig-paths, Bun’s loader, or a bundler). Otherwise Node will fail to resolve these*.jsimports after transpilation.src/types/geocodeTypes.ts (1)
1-2: Alias import looks goodImports align with the new path-alias convention and compile-time schema typing remains intact.
src/types/reverseGeocodeTypes.ts (1)
1-2: Consistent alias usage acknowledgedChange mirrors the pattern in
geocodeTypes.ts; no issues spotted.src/tools/reverseGeocode.ts (1)
1-7: Double-check tree-shaking & circular-import safetyMoving to absolute aliases is fine, but keep an eye on possible circular references between
@/tools/*and@/types/*once everything compiles todist.
If you hit odd runtimeundefinedvalues, a quickmadge --circular distcan confirm.src/tools/geocode.ts (1)
1-7: No functional change – implementation remains soundThe alias/import refactor compiles cleanly, and the handler logic is untouched.
src/tools/prepareResponse.ts (1)
1-24: LGTM! Formatting improvements align with Prettier configuration.The consistent use of single quotes and multi-line object formatting enhances readability while maintaining the same functionality.
src/clients/nominatimClient.ts (2)
2-4: Excellent adoption of path aliases for cleaner imports.The transition from relative paths to
@/aliases improves maintainability and makes the import structure more consistent across the codebase.
15-16: Good formatting improvement for readability.The multi-line arrow function format enhances readability while preserving the same destructuring and mapping logic.
src/types/commonTypes.ts (1)
1-37: Excellent formatting improvements for complex schema readability.The multi-line formatting of the Zod schema significantly improves readability and makes individual field configurations easier to understand and maintain.
prettier.config.ts (1)
1-19: Well-configured Prettier setup with sensible defaults.The configuration appropriately enforces consistent formatting across the codebase with TypeScript-specific settings and a practical override for declaration files.
src/tools/__tests__/prepareResponse.test.ts (1)
1-106: Excellent comprehensive test coverage with clear understanding of implementation behavior.The test suite thoroughly covers all major scenarios including edge cases. The circular reference test (lines 60-79) demonstrates good understanding of the implementation logic - correctly identifying that non-array objects hit the validation check before JSON serialization.
src/tools/__tests__/geocode.test.ts (3)
12-26: Well-structured mocking approach for McpServer.The mock implementation correctly captures the handler function for testing while maintaining the expected interface.
65-85: Excellent integration test using actual handleGeocodeResult.This test effectively verifies the integration between the tool handler and the response processing function while maintaining proper isolation through mocking.
108-128: Correct error handling test implementation.The test properly verifies that errors from the geocoding client propagate through the handler, which aligns with the expected behavior where
handleGeocodeResultis not called whengeocodeAddressthrows an error.src/tools/__tests__/reverseGeocode.test.ts (2)
67-89: Excellent test coverage for successful reverse geocoding.The test properly verifies the integration between the tool handler and the actual
handleGeocodeResultfunction while ensuring the client is called with correct parameters.
112-128: Consistent error handling pattern with geocode tests.The error propagation test correctly follows the same pattern as the geocode tests, ensuring consistent behavior across both tools.
Summary by CodeRabbit
New Features
Chores
Style
Documentation