test: add vitest coverage for tool render modules#123
Conversation
Add per-renderer vitest coverage with XSS escaping assertions, a shared test helper, registry-driven smoke tests, and raised coverage thresholds.
📝 WalkthroughWalkthroughThis PR adds a shared test helper module and extensive Vitest behavioral test suites for ChangesRenderer Behavioral Test Coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 (1)
static/js/render/tool_result/file_edit.test.js (1)
1-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared test factory for summary-only renderers.
This file and
plan.test.jsfollow an identical three-test pattern (render field, missing field defaults, XSS escape) for renderers that just build a"<Label>: <field>"summary viafinishToolResult. A small parametrized helper (e.g.,testSummaryOnlyRenderer(name, renderFn, field, label)) could reduce duplication if more such renderers are added later.🤖 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 `@static/js/render/tool_result/file_edit.test.js` around lines 1 - 29, The tests in renderFileEditResult duplicate the same three-case pattern used by other summary-only renderers, so extract a shared parametrized test helper for this shape. Create a reusable factory (for example, around mountToolResult, expectNoRawHtml, and the renderer under test) that accepts the render function, field name, and label, then use it from file_edit.test.js and plan.test.js to cover summary, missing-field default, and XSS escaping consistently.
🤖 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 `@static/js/render/tool_result/task.test.js`:
- Around line 6-19: The completed-task rendering test is asserting a
locale-specific token string, which can vary across environments. Update the
`renderTaskResult` test to avoid hard-coding the formatted token value in the
`renders completed task with duration and token stats` case, and instead compare
against a locale-agnostic expectation or derive the expected text using
`toLocaleString()` from the same numeric value.
---
Nitpick comments:
In `@static/js/render/tool_result/file_edit.test.js`:
- Around line 1-29: The tests in renderFileEditResult duplicate the same
three-case pattern used by other summary-only renderers, so extract a shared
parametrized test helper for this shape. Create a reusable factory (for example,
around mountToolResult, expectNoRawHtml, and the renderer under test) that
accepts the render function, field name, and label, then use it from
file_edit.test.js and plan.test.js to cover summary, missing-field default, and
XSS escaping consistently.
🪄 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
Run ID: 759aa381-9776-4d07-9d03-ffacba6ed9a2
📒 Files selected for processing (23)
static/js/render/registry.test.jsstatic/js/render/test_helpers.jsstatic/js/render/tool_result/bash.test.jsstatic/js/render/tool_result/fallback.test.jsstatic/js/render/tool_result/file_edit.test.jsstatic/js/render/tool_result/file_write.test.jsstatic/js/render/tool_result/glob.test.jsstatic/js/render/tool_result/grep.test.jsstatic/js/render/tool_result/plan.test.jsstatic/js/render/tool_result/task.test.jsstatic/js/render/tool_result/todo_write.test.jsstatic/js/render/tool_result/user_input.test.jsstatic/js/render/tool_result/web_fetch.test.jsstatic/js/render/tool_result/web_search.test.jsstatic/js/render/tool_use/ask_user_question.test.jsstatic/js/render/tool_use/fallback.test.jsstatic/js/render/tool_use/glob.test.jsstatic/js/render/tool_use/grep.test.jsstatic/js/render/tool_use/read.test.jsstatic/js/render/tool_use/task.test.jsstatic/js/render/tool_use/todo_write.test.jsstatic/js/render/tool_use/write.test.jsvitest.config.js
| it('renders completed task with duration and token stats', () => { | ||
| const html = renderTaskResult({ | ||
| result_type: 'task', | ||
| status: 'completed', | ||
| total_duration_ms: 2500, | ||
| total_tokens: 1500, | ||
| total_tool_use_count: 3, | ||
| }); | ||
| expect(html).toContain('Task completed'); | ||
| expect(html).toContain('2.5s'); | ||
| expect(html).toContain('1,500 tokens'); | ||
| expect(html).toContain('3 tool calls'); | ||
| expect(html).toContain('tool-result'); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Node.js Intl.NumberFormat / toLocaleString default to en-US regardless of OS locale in typical CI (GitHub Actions) environments?
💡 Result:
In typical CI environments like GitHub Actions, Node.js Intl.NumberFormat and toLocaleString do not necessarily default to en-US solely because of the OS locale [1][2][3]. Instead, their behavior is primarily dictated by the ICU (International Components for Unicode) data bundle included with the specific Node.js binary being used [4][2][3]. Many standard Node.js distributions, particularly smaller builds, are configured with 'small-icu' data, which includes only English-language locale data by default [3]. Consequently, even if your OS locale is set to something else, these environments will frequently fall back to en-US [1][2]. To ensure consistent behavior in CI environments, you should consider the following: 1. Locale-Aware Code: Always explicitly specify the locale in your Intl constructor or methods (e.g., new Intl.NumberFormat('en-GB',...)) rather than relying on the environment's default [2][5]. 2. Full ICU Data: If you require full internationalization support, ensure the Node.js environment is configured with 'full-icu' data [4][3]. This can often be achieved by installing the full-icu npm package and setting the NODE_ICU_DATA environment variable to point to that package's data file [4][2]. 3. Verify Environment: Use Intl.NumberFormat.resolvedOptions.locale to programmatically check the effective default locale in your CI environment at runtime [6][7]. Relying on the implicit host OS locale in CI is generally discouraged because it introduces environmental dependencies that can cause tests to pass locally but fail in CI, or vice-versa [1][2][8].
Citations:
- 1: How to get a node image with full internationalization support? (--with-intl=full-icu) actions/setup-node#267
- 2: Intl.NumberFormat returning error, server and client side locales misaligned vercel/next.js#19409
- 3: http://nodejs.org/docs/latest/api/intl.html
- 4: https://stackoverflow.com/questions/55183776/different-behaviour-of-intl-numberformat-in-node-and-browser
- 5: [Bug]: formatCost tests depend on the runtime default locale danny-avila/LibreChat#13776
- 6: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat/resolvedOptions
- 7: Getting the default locale tc39/proposal-intl-locale#84
- 8: https://github.com/orgs/community/discussions/26860
Avoid locale-dependent token assertions. toLocaleString() follows the runtime’s default ICU locale, so hard-coding 1,500 tokens can make this test flaky across environments. Use a locale-agnostic expectation or build the string from (1500).toLocaleString() in the test.
🤖 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 `@static/js/render/tool_result/task.test.js` around lines 6 - 19, The
completed-task rendering test is asserting a locale-specific token string, which
can vary across environments. Update the `renderTaskResult` test to avoid
hard-coding the formatted token value in the `renders completed task with
duration and token stats` case, and instead compare against a locale-agnostic
expectation or derive the expected text using `toLocaleString()` from the same
numeric value.
1 participant
Closes #115
Summary
Adds vitest files for the remaining registered renderers in
tool_use/andtool_result/. We had five test files covering a small slice of 32 render modules; now every registered renderer has at least one behavioral test.test_helpers.jswrapsrenderToolUse/renderToolResultand gives a consistent way to check that raw HTML does not leak through. Most tests also cover empty or malformed input.registry.test.jsruns representative fixtures through every entry inTOOL_USE_RENDERERSandTOOL_RESULT_RENDERERS.Coverage thresholds in
vitest.config.jsmove from 80/70/50 to 85/75/55.Test plan
npm test(156 passing, up from 93)npm run test:coverage(tool_result/andtool_use/both around 99%)Week 28 item #3. Wednesday PR 2 (adversarial dispatch fixtures) is separate.
Summary by CodeRabbit
Bug Fixes
Tests