test(tools): add comprehensive unit tests for AskFollowupQuestionTool#212
Conversation
proyectoauraorg
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 0199cd8c7783b46e1ab632b71362f41490580509 and 7f89daa. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRewrites AskFollowupQuestionTool tests to call execute and handlePartial directly, expands coverage for validation, success/error flows, partial streaming, and edge cases, and normalizes task.ask returned text to a non-null string used in messaging and tool-result payloads. ChangesTest Suite Rewrite: AskFollowupQuestionTool Behavior Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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)
src/core/tools/__tests__/askFollowupQuestionTool.spec.ts (2)
418-431: ⚡ Quick winClarify the misleading comment about nullish coalescing behavior.
The comment on line 429 states "nativeArgs.question is "" (falsy), so it should use "" from ?? operator". This is misleading because the nullish coalescing operator (
??) only falls back fornullorundefined, not for falsy values like empty strings.When
nativeArgs.question = "", the empty string is used directly because the property exists, not because of fallback behavior. The test is correct, but the comment incorrectly describes the operator's behavior.📝 Suggested comment correction
- // nativeArgs.question is "" (falsy), so it should use "" from ?? operator + // nativeArgs.question is "", which is used directly (not a fallback since ?? only falls back for null/undefined)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/__tests__/askFollowupQuestionTool.spec.ts` around lines 418 - 431, Update the misleading test comment: clarify that the nullish coalescing operator (??) only falls back for null or undefined, not for falsy values like an empty string; in the test for askFollowupQuestionTool.handlePartial where nativeArgs.question === "", state that the empty string is used directly (because the property exists) rather than implying fallback behavior from ??. Locate the comment near the test block that references nativeArgs.question and replace it with a concise note stating that ?? does not treat "" as nullish and thus the empty string is passed through to mockTask.ask.
309-315: 💤 Low valueConsider whether this test adds value or tests implementation details.
This test verifies that
askApprovalis never called. While this documents current behavior, it may be overly specific to implementation details. If the tool's approval requirements change in the future (e.g., requiring approval under certain conditions), this test would break even though the tool's external behavior might still be correct.Consider whether this constraint is a genuine requirement or just an implementation detail.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/__tests__/askFollowupQuestionTool.spec.ts` around lines 309 - 315, The test "should not call askApproval" tightly couples to the implementation by asserting mockCallbacks.askApproval is never invoked; either remove this test or rewrite it to assert external behavior instead (e.g., call tool.execute(params, mockTask, mockCallbacks) and verify the observable output/side-effect that reflects "no approval required" rather than checking mockCallbacks.askApproval directly). If keeping an internal-call assertion is required, narrow its scope (for example only when specific input flags indicate no approval) and reference askApproval, tool.execute, and mockCallbacks so the change is made in the existing spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/tools/__tests__/askFollowupQuestionTool.spec.ts`:
- Around line 433-443: The code inconsistently handles undefined text: normalize
undefined to empty string when building both the say payload and the tool
result; in the implementation (where task.ask is consumed inside execute) change
uses of `${text}` to `${text ?? ""}` (or use a normalized const like const
safeText = text ?? "" and pass safeText to task.say and to
pushToolResult(formatResponse.toolResult(..., images))). Also update the test
expectation in askFollowupQuestionTool.spec.ts to assert an empty string inside
the formatted response instead of the literal "undefined".
---
Nitpick comments:
In `@src/core/tools/__tests__/askFollowupQuestionTool.spec.ts`:
- Around line 418-431: Update the misleading test comment: clarify that the
nullish coalescing operator (??) only falls back for null or undefined, not for
falsy values like an empty string; in the test for
askFollowupQuestionTool.handlePartial where nativeArgs.question === "", state
that the empty string is used directly (because the property exists) rather than
implying fallback behavior from ??. Locate the comment near the test block that
references nativeArgs.question and replace it with a concise note stating that
?? does not treat "" as nullish and thus the empty string is passed through to
mockTask.ask.
- Around line 309-315: The test "should not call askApproval" tightly couples to
the implementation by asserting mockCallbacks.askApproval is never invoked;
either remove this test or rewrite it to assert external behavior instead (e.g.,
call tool.execute(params, mockTask, mockCallbacks) and verify the observable
output/side-effect that reflects "no approval required" rather than checking
mockCallbacks.askApproval directly). If keeping an internal-call assertion is
required, narrow its scope (for example only when specific input flags indicate
no approval) and reference askApproval, tool.execute, and mockCallbacks so the
change is made in the existing spec.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d860b07-dd53-4279-9943-6bc904098856
📒 Files selected for processing (1)
src/core/tools/__tests__/askFollowupQuestionTool.spec.ts
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
proyectoauraorg
force-pushed
the
test/add-ask-followup-question-tool-tests
branch
from
0199cd8 to
e310f4f
Compare
![roomote[bot]](https://avatars.githubusercontent.com/in/2924042?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for adding coverage here. I found two issues before this is ready to merge:
-
The new test for
task.ask()returningundefinedis asserting the wrong behavior. InAskFollowupQuestionTool.execute(), the user-facingsay()path already normalizestext ?? "", so this case should be treated as an empty user message. The new assertion instead expects<user_message>undefined</user_message>, which locks in obviously bad output instead of catching it. Please change that assertion so the test describes the intended behavior. -
This rewrite drops the existing
NativeToolCallParsercoverage forask_followup_question. The current suite has focused cases that verify partial streaming builds and finalizesnativeArgscorrectly, which is valuable protocol-edge coverage for this tool. If you replace that suite, please preserve those cases or add equivalent coverage in the same PR.
The broader direction makes sense, but I'd want those two points fixed before approving.
proyectoauraorg
force-pushed
the
test/add-ask-followup-question-tool-tests
branch
from
e310f4f to
304f089
Compare
|
Thanks @roomote — both points are addressed in the latest commit:
Re-requesting review when you have a moment 🙏 |
|
I saw the mention, but I could not start work on this PR with the current Roomote GitHub setup. |
- Add 36 unit tests covering execute(), handlePartial(), and error paths - Fix undefined text handling: use safeText = text ?? "" to prevent literal 'undefined' in user_message output - Add NativeToolCallParser integration tests for streaming ask_followup_question tool calls - Address review feedback: fix undefined assertion and improve empty string comment clarity
…oolResult and nest NativeToolCallParser tests - Add assertion verifying pushToolResult argument does not contain 'undefined' - Move NativeToolCallParser integration tests into main describe block for better organization
proyectoauraorg
force-pushed
the
test/add-ask-followup-question-tool-tests
branch
from
7f89daa to
7f37ce2
Compare
|
Both points are handled in the latest commit:
36 tests pass locally. @roomote ready for another look when you have a moment 🙏 |
|
I saw the mention, but I could not start work on this PR with the current Roomote GitHub setup. |
1 participant
Summary
This PR adds comprehensive unit tests for
AskFollowupQuestionTool, expanding test coverage from 4 tests to 34 tests (8.5x increase).What's tested
Test approach
src/core/tools/__tests__/askFollowupQuestionTool.spec.tsvi.mock()for isolating dependencies (task.tsandmodes.ts)Motivation
Following the same pattern as PR #1496 (MiMo provider tests) and PR #1497 (SwitchModeTool tests), this PR contributes to the project's goal of improving test coverage across tool implementations.
Summary by CodeRabbit
Tests
Bug Fixes