refactor: extend test helper to eliminate remaining duplication#106
refactor: extend test helper to eliminate remaining duplication#106
Conversation
Summary of ChangesHello @teng-lin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors test code within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively reduces test duplication in consumer-message-mapper.test.ts by extending the makeAssistantMsg helper, which improves code maintainability. The accompanying documentation updates are also valuable. I've left two comments: one concerning the removal of the 'Programmatic usage' section from the README, and another suggesting a small adjustment to a test case to better preserve its original data.
| ## Security |
There was a problem hiding this comment.
The 'Programmatic usage' section has been removed. This seems to be a significant documentation change that is out of scope for a test refactoring PR. This section appears valuable for users who want to integrate BeamCode as a library. If this removal was intentional, could you please explain the reasoning in the PR description? If the example is outdated, it would be better to update it or mark it as such, rather than removing it completely.
There was a problem hiding this comment.
Fixed — rebased onto current main so the README and docs changes are no longer in this PR's diff. The scope is now limited to the test refactoring only.
| const msg = makeAssistantMsg( | ||
| [ | ||
| { type: "text", text: "Let me run that" }, | ||
| { type: "tool_use", id: "tu-1", name: "Bash", input: { command: "ls" } }, | ||
| ], | ||
| metadata: { | ||
| message_id: "msg-004", | ||
| model: "claude-sonnet-4-5-20250929", | ||
| stop_reason: "end_turn", | ||
| usage: { | ||
| input_tokens: 100, | ||
| output_tokens: 50, | ||
| cache_creation_input_tokens: 10, | ||
| cache_read_input_tokens: 5, | ||
| }, | ||
| parent_tool_use_id: null, | ||
| }, | ||
| }); | ||
| "msg-004", | ||
| ); |
There was a problem hiding this comment.
The refactoring to use makeAssistantMsg here omits the metadata argument that was present in the original createUnifiedMessage call. The original call included specific stop_reason and usage values. While the current test assertions don't check these fields, removing this data makes the test case less representative. It would be better to preserve the original metadata by passing it to the helper to maintain test data fidelity.
const msg = makeAssistantMsg(
[
{ type: "text", text: "Let me run that" },
{ type: "tool_use", id: "tu-1", name: "Bash", input: { command: "ls" } },
],
"msg-004",
{
stop_reason: "end_turn",
usage: {
input_tokens: 100,
output_tokens: 50,
cache_creation_input_tokens: 10,
cache_read_input_tokens: 5,
},
},
);There was a problem hiding this comment.
Good catch — restored the original stop_reason and usage values via metadata overrides to preserve test data fidelity.
Address the broader Gemini review feedback from PR #105 by converting pre-existing mapAssistantMessage tests to use the makeAssistantMsg helper, adding optional metadata overrides support. Net -49 lines.
teng-lin
force-pushed
the
fix/pr105-review-followup
branch
from
a9a870c to
8a65834
Compare
1 participant
Summary
Follow-up to PR #105's Gemini code review, which suggested extracting repeated logic into helpers to reduce test duplication.
makeAssistantMsghelper with optionalmetadataoverrides parametermapAssistantMessagetests to use the helpermessage_idmissing,usagemissing) with rawcreateUnifiedMessagesince they intentionally test absent fieldsTest plan
consumer-message-mappertests pass