test: cover platform-driven region discovery - #192
Conversation
jwfing
left a comment
There was a problem hiding this comment.
Summary
The PR adds focused coverage confirming that insta regions requests the platform catalog and renders only its returned entries in both human-readable and JSON modes. The dependency-injection change follows repository testing conventions and leaves production behavior unchanged. No correctness, security, or performance problems were found.
Requirements context
Intent was derived primarily from the PR title and description: region availability must come from the platform’s /regions catalog so retired regions disappear without a CLI release. README.md confirms that insta regions lists regions available for postgres and compute, and .claude/skills/developing-insta-cli/SKILL.md recommends dependency injection for side-effectful command tests. The linked insta-cloud#136 issue was not retrievable, and no more detailed matching requirement was found locally; assessment therefore uses the PR description and repository documentation.
Findings
Critical
(none)
Suggestion
(none)
Information
- test/regions.test.ts:12: Focused regression coverage matches the stated behavior - The parameterized test exercises both output modes, verifies
GET /regions, and compares output against the injected Virginia, Frankfurt, and Singapore catalog. Exact output comparison would expose additional hard-coded or retired entries. - src/commands/regions.ts:5: Dependency injection preserves production behavior - The optional API dependency is limited to the
requestmethod, while normal CLI invocation still loadsApiClientand performs the same request. This follows the repository’s documented dependency-injection testing convention. - Security: No security-sensitive behavior changed - The CLI command remains wired to the authenticated
ApiClient; the optional injected client is used only by direct programmatic callers and introduces no new shell execution, secret handling, logging, dependency, or authorization bypass in the CLI path. - Performance: No performance regression identified - The production path still performs one API request and a linear pass over the returned catalog. The change adds no extra I/O, loops, dependencies, or allocations of practical significance.
Tests
git diff --check main...HEAD passed. npm run typecheck and npm test were attempted but could not execute because node_modules is absent and the local tsc/vitest binaries are unavailable; dependencies were not installed because the review environment is read-only.
Verdict
approved: Approved because the review found zero Critical findings; all observations are informational.
Region discovery must follow the platform catalog when regions are retired. Add coverage for the human-readable and JSON output of
insta regions, using an injected API client and the Virginia, Frankfurt, and Singapore catalog.Part of InsForge/insta-cloud#136. The CLI continues to fetch
/regions; the platform change controls availability.Validation:
npm run typecheck;npm test(799 passed, outside the sandbox for the subprocess timeout test).Summary by cubic
Adds test coverage for
insta regionsso region discovery follows the platform catalog, as required byInsForge/insta-cloud#136.regionsListaccepts an optional injected API client for testing.GET /regionsand the platform controls availability.Written for commit 2c55f0e. Summary will update on new commits.