test: command hints, batch tool, finops formatting — 3 untested modules#427
test: command hints, batch tool, finops formatting — 3 untested modules#427anandgupta42 wants to merge 1 commit intomainfrom
Conversation
Cover Command.hints template parser (TUI command palette), BatchTool schema validation + formatValidationError (LLM parallel tool calls), and extend finops-formatting with TB/PB unit boundaries (warehouse cost reports at scale). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> https://claude.ai/code/session_01JLTB1JYf2qUPUPqfcLepKd
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughThree new test suites added across the codebase: extending Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/opencode/test/tool/batch.test.ts (2)
92-99: Consider a more precise describe block name.The current title "DISALLOWED set enforcement" suggests we're testing the DISALLOWED set filtering logic, but the tests verify the tool id and schema capacity. The comments explain the relationship well, but a title like
"BatchTool: metadata and schema capacity"or"BatchTool: id and runtime-enforced limits"would better reflect the actual test coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/tool/batch.test.ts` around lines 92 - 99, Rename the describe block to accurately reflect what the tests assert: update the describe title from "BatchTool: DISALLOWED set enforcement" to something like "BatchTool: id and runtime-enforced limits" or "BatchTool: metadata and schema capacity" so it matches the assertions (e.g., checking BatchTool.id and any schema/capacity behavior). Locate the describe that wraps the test verifying BatchTool.id and change only the string identifier to the clearer title; leave the test body and comments untouched.
66-89: Consider removing redundant.toBeDefined()checks.Lines 68 and 80 repeat the assertion already covered by the dedicated test on line 63. Since these tests depend on
formatValidationErrorexisting (the!assertion would throw anyway), the redundant guards add noise without extra safety.♻️ Suggested simplification
test("produces readable error message for empty array", async () => { const tool = await getToolInfo() - expect(tool.formatValidationError).toBeDefined() const result = tool.parameters.safeParse({ tool_calls: [] }) expect(result.success).toBe(false) if (!result.success) { const msg = tool.formatValidationError!(result.error) expect(msg).toContain("Invalid parameters for tool 'batch'") expect(msg).toContain("Expected payload format") } }) test("includes field path in type error", async () => { const tool = await getToolInfo() - expect(tool.formatValidationError).toBeDefined() const result = tool.parameters.safeParse({ tool_calls: [{ tool: 123, parameters: {} }], })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/tool/batch.test.ts` around lines 66 - 89, Remove the redundant existence checks for formatValidationError in the two tests "produces readable error message for empty array" and "includes field path in type error": delete the expect(tool.formatValidationError).toBeDefined() calls and rely on the non-null assertion (tool.formatValidationError!) when formatting the error; keep the rest of each test (calling tool.parameters.safeParse, asserting result.success is false, and invoking tool.formatValidationError!(result.error) to assert message contents).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/command/hints.test.ts`:
- Around line 13-15: The test description/comment is contradictory: update the
test name/comment that currently reads "sorts numbered placeholders numerically
by string sort" to explicitly state lexicographic/string sorting (e.g., "sorts
numbered placeholders lexicographically (string order)") so it matches the
assertion on Command.hints("$2 then $1 then $10") expecting ["$1","$10","$2"];
ensure the comment about "$10" sorting before "$2" is worded to reflect string
order rather than "numerically".
---
Nitpick comments:
In `@packages/opencode/test/tool/batch.test.ts`:
- Around line 92-99: Rename the describe block to accurately reflect what the
tests assert: update the describe title from "BatchTool: DISALLOWED set
enforcement" to something like "BatchTool: id and runtime-enforced limits" or
"BatchTool: metadata and schema capacity" so it matches the assertions (e.g.,
checking BatchTool.id and any schema/capacity behavior). Locate the describe
that wraps the test verifying BatchTool.id and change only the string identifier
to the clearer title; leave the test body and comments untouched.
- Around line 66-89: Remove the redundant existence checks for
formatValidationError in the two tests "produces readable error message for
empty array" and "includes field path in type error": delete the
expect(tool.formatValidationError).toBeDefined() calls and rely on the non-null
assertion (tool.formatValidationError!) when formatting the error; keep the rest
of each test (calling tool.parameters.safeParse, asserting result.success is
false, and invoking tool.formatValidationError!(result.error) to assert message
contents).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 59cce8c9-7aa4-4e22-9426-20ca89b4b70a
📒 Files selected for processing (3)
packages/opencode/test/altimate/tools/finops-formatting.test.tspackages/opencode/test/command/hints.test.tspackages/opencode/test/tool/batch.test.ts
| test("sorts numbered placeholders numerically by string sort", () => { | ||
| // $10 sorts after $1 and $2 in string order | ||
| expect(Command.hints("$2 then $1 then $10")).toEqual(["$1", "$10", "$2"]) |
There was a problem hiding this comment.
Fix contradictory sorting description in this test.
Line 13/Line 14 describe sorting ambiguously/incorrectly, while Line 15 correctly asserts lexicographic behavior ("$10" before "$2"). Please align the wording with the assertion.
Suggested wording fix
- test("sorts numbered placeholders numerically by string sort", () => {
- // $10 sorts after $1 and $2 in string order
+ test("sorts numbered placeholders lexicographically (string sort)", () => {
+ // In string sort: $1, $10, $2
expect(Command.hints("$2 then $1 then $10")).toEqual(["$1", "$10", "$2"])
})📝 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.
| test("sorts numbered placeholders numerically by string sort", () => { | |
| // $10 sorts after $1 and $2 in string order | |
| expect(Command.hints("$2 then $1 then $10")).toEqual(["$1", "$10", "$2"]) | |
| test("sorts numbered placeholders lexicographically (string sort)", () => { | |
| // In string sort: $1, $10, $2 | |
| expect(Command.hints("$2 then $1 then $10")).toEqual(["$1", "$10", "$2"]) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/command/hints.test.ts` around lines 13 - 15, The test
description/comment is contradictory: update the test name/comment that
currently reads "sorts numbered placeholders numerically by string sort" to
explicitly state lexicographic/string sorting (e.g., "sorts numbered
placeholders lexicographically (string order)") so it matches the assertion on
Command.hints("$2 then $1 then $10") expecting ["$1","$10","$2"]; ensure the
comment about "$10" sorting before "$2" is worded to reflect string order rather
than "numerically".
|
Superseded by #439 which consolidates all 12 test PRs into one, deduplicates overlapping tests, and fixes bugs found during review. |
2 participants
What does this PR do?
Adds 40 new tests across 3 previously untested modules, discovered via automated test-discovery rotation targeting the command, tool, and altimate areas.
1.
Command.hints—src/command/index.ts(9 new tests)This pure function parses template placeholder patterns (
$1,$2,$ARGUMENTS) and drives the TUI command palette's argument hint display. Zero tests existed. New coverage includes:$ARGUMENTSextraction (alone and combined with numbered placeholders)$FOOcorrectly ignored)$10sorts before$2)2.
BatchToolschema +formatValidationError—src/tool/batch.ts(12 new tests)The batch tool is how the LLM executes parallel tool calls. Zero tests existed. New coverage includes:
formatValidationErroris verified to exist (no vacuous conditional guards)execute())BatchTool.id === "batch")3.
formatByteshigher units —src/altimate/tools/finops-formatting.ts(5 new tests added to existing file)The existing test file only covered up to GB. For warehouse cost reports at scale (multi-TB/PB data volumes), higher unit formatting matters. New coverage includes:
1024 PBnot1 EB)Type of change
Issue for this PR
N/A — proactive test coverage via automated test-discovery
How did you verify your code works?
All 40 tests pass locally. Test-only changes — no source modifications.
Critic review
Tests were reviewed by a critic agent. Key feedback incorporated:
test/altimate/finops-formatting.test.ts— merged into existingtest/altimate/tools/fileifguards in batch tests — added explicitexpect(tool.formatValidationError).toBeDefined()Checklist
https://claude.ai/code/session_01JLTB1JYf2qUPUPqfcLepKd
Summary by CodeRabbit