Skip to content

test: add validators module and comprehensive test harness#25

Open
jesustorres-code wants to merge 1 commit into
eliottreich:mainfrom
jesustorres-code:feat/test-harness
Open

test: add validators module and comprehensive test harness#25
jesustorres-code wants to merge 1 commit into
eliottreich:mainfrom
jesustorres-code:feat/test-harness

Conversation

@jesustorres-code
Copy link
Copy Markdown

@jesustorres-code jesustorres-code commented May 18, 2026

Summary

  • Extracts all argument-validation logic from index.ts into src/validators.ts (parseGitHubRepo, requireNonEmpty, toolError) — replaces 9 inline validation blocks across 8 tool handlers
  • Adds test/validators.test.mjs using Node's built-in node:test (zero new dependencies) with 19 test cases
  • Wires npm test into ci.yml so the suite gates every PR

Test coverage

parseGitHubRepo (13 cases):

  • owner/name — plain shorthand
  • https:// and http:// full GitHub URLs
  • .git suffix stripping
  • .git + trailing slash
  • Query string stripping (?tab=issues)
  • Fragment stripping (#readme)
  • Trailing slash without .git
  • Empty string → "repo is required"
  • null / undefined → "repo is required"
  • No-slash string → "Could not parse repo"
  • Whitespace-only → "repo is required"

requireNonEmpty (6 cases):

  • Valid string → null (no error)
  • Empty string, whitespace-only, null, undefinedisError: true
  • Field name appears verbatim in error message

Why this is more complete than PR #24

PR #24 extracts only parseGitHubRepo and applies it to one handler. This PR:

  • adds a second generic validator (requireNonEmpty) used across 8 handlers
  • replaces all inline validation in index.ts (not just one case), eliminating the duplication
  • covers 19 test cases vs 5

Closes #16

Extracts all argument-validation logic from index.ts into a dedicated
src/validators.ts module (parseGitHubRepo, requireNonEmpty, toolError)
and replaces 9 inline validation blocks across 8 tool handlers with
calls to those helpers.

Adds test/validators.test.mjs (node:test, no extra dependencies) with
19 cases covering parseGitHubRepo (8 happy paths including query-string
and fragment stripping, 5 error paths) and requireNonEmpty (6 cases
across all field-name permutations used in the handlers).

Wires npm test into ci.yml so the suite gates every PR.

Closes eliottreich#16

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@eliottreich
Copy link
Copy Markdown
Owner

Thanks for this, genuinely useful work. We've put a funded $10 TaskBounty bounty on the underlying issue: https://www.task-bounty.com/task/taskbounty-mcp-server-16-add-a-test-harness-and-ru-q8mnpc . To claim it, register at https://www.task-bounty.com/, then submit your fix through the platform (REST API, MCP server, or the patch-upload endpoint). It runs against the repo's own tests in an isolated sandbox, and on a verified pass you're paid through escrow in USDC, ETH, BTC, or bank. We keep payment on-platform so it stays verified and auditable for everyone. First verified submission wins it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a test harness and run it in CI

3 participants