test(core): add missing tests for prompts/utils.ts#19941
test(core): add missing tests for prompts/utils.ts#19941jacob314 merged 7 commits intogoogle-gemini:mainfrom
Conversation
Summary of ChangesHello @krrishverma1805-web, 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 addresses a significant test coverage gap 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
The pull request introduces comprehensive test coverage for packages/core/src/prompts/utils.ts, which is a significant improvement for code quality and maintainability. The tests cover various scenarios for resolvePathFromEnv, isSectionEnabled, and applySubstitutions, including edge cases and environment variable handling. The use of vi.stubEnv and vi.mock demonstrates good testing practices. The new test file also includes the correct license header and year, adhering to the repository's guidelines.
gemini-cli
bot
added
the
area/core
gemini-cli
bot
added
priority/p3
| it('should return true when the env var is not set', () => { | ||
| expect(isSectionEnabled('SOME_KEY')).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Here, the test assumes GEMINI_PROMPT_SOME_KEY won't exist. However, if a
developer happens to have set this in their local system environment:
export GEMINI_PROMPT_SOME_KEY=0This test will fail locally.
While you correctly included vi.unstubAllEnvs() in the afterEach block, this
Vitest function only restores the environment back to its initial state before
the test started—it doesn't aggressively clear pre-existing variables.
To make this test block 100% bulletproof and isolated from global state leakage,
we should explicitly clear the variable inside the test itself (similar to how
you use vi.stubEnv in the other tests):
it('should return true when the env var is not set', () => {
vi.stubEnv('GEMINI_PROMPT_SOME_KEY', '');
expect(isSectionEnabled('SOME_KEY')).toBe(true);
});Let me know what you think.
| it('should resolve a regular path', () => { | ||
| const result = resolvePathFromEnv('/some/absolute/path'); | ||
| expect(result.isSwitch).toBe(false); | ||
| expect(result.value).toBe('/some/absolute/path'); |
There was a problem hiding this comment.
In here, the expected strings hardcode POSIX (/) forward slashes:
// Line 127 and Line 135
expect(result.value).toBe('/some/absolute/path');When resolvePathFromEnv ( line 125 ) runs on a Windows computer, it uses Node's path.resolve and path.join to return a proper Windows-style path (like "C:\some\absolute\path"). However, the test is trying to match that result
against the hardcoded string "/some/absolute/path". Because of this mismatch,
the test will always fail on Windows.
To guarantee cross-platform compatibility without changing your test logic, we
should wrap the expected strings in path.resolve() and path.join(). This
delegates the slash-formatting to Node.js natively:
import path from 'node:path';
// ...
it('should resolve a regular path', () => {
// Dynamically resolves to C:\... on Windows and /... on POSIX
expect(result.value).toBe(path.resolve('/some/absolute/path'));
});Let me know what you think?
| const result = resolvePathFromEnv('~/my/custom/path'); | ||
| expect(result.isSwitch).toBe(false); | ||
| expect(result.value).toContain('/mock/home'); | ||
| expect(result.value).toContain('my/custom/path'); |
There was a problem hiding this comment.
Same thing as my previous comment on line 127
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Co-authored-by: Jacob Richman <jacob314@gmail.com>
Co-authored-by: Jacob Richman <jacob314@gmail.com>
Co-authored-by: Jacob Richman <jacob314@gmail.com>
Co-authored-by: Jacob Richman <jacob314@gmail.com>
Co-authored-by: Jacob Richman <jacob314@gmail.com>
3 participants
Summary
Adds comprehensive test coverage for
packages/core/src/prompts/utils.ts,which previously had no corresponding test file despite containing
three exported utility functions used in prompt construction.
Fixes #19940
Changes Made
packages/core/src/prompts/utils.test.tswith 27 testscovering all three exported functions:
resolvePathFromEnv()(14 tests)true,false,1,0)~) expansion to home directoryisSectionEnabled()(8 tests)1,true)0,false)applySubstitutions()(8 tests)${AgentSkills}single and multiple replacements${SubAgents}with Gemini 3 vs legacy snippets${AvailableTools}with and without tools${toolName_ToolName}variablesTesting
npx vitest run packages/core/src/prompts/utils.test.ts)npm test -w @google/gemini-cli-core)vi.stubEnv(),vi.mock()for dependencies)Context
This fills a test coverage gap identified by comparing source files
to test files in
packages/core/src/prompts/. All other files inthis directory (
promptProvider.ts,prompt-registry.ts,mcp-prompts.ts) have corresponding test files.