test(a2a-server): use vi.stubEnv() for env vars per GEMINI.md#27584
test(a2a-server): use vi.stubEnv() for env vars per GEMINI.md#27584Pluviobyte wants to merge 1 commit into
Conversation
Summary of ChangesHello, 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 suites in the a2a-server package to align with established testing conventions. By replacing direct modifications of process.env with Vitest's stubbing utilities, the changes improve test isolation and reliability, ensuring that environment variable changes do not persist across test boundaries. Highlights
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 the 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 counterproductive. 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. Footnotes
|
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request refactors the test files init.test.ts and app.test.ts to use Vitest's environment variable stubbing utilities (vi.stubEnv and vi.unstubAllEnvs) instead of directly modifying process.env. The review feedback correctly points out that, according to the repository style guide, environment variables should be unset using an empty string (e.g., vi.stubEnv('CODER_AGENT_WORKSPACE_PATH', '')) rather than passing undefined.
| vi.spyOn(commandRegistry, 'get').mockReturnValue(mockCommand); | ||
|
|
||
| delete process.env['CODER_AGENT_WORKSPACE_PATH']; | ||
| vi.stubEnv('CODER_AGENT_WORKSPACE_PATH', undefined); |
There was a problem hiding this comment.
According to the repository style guide, to "unset" an environment variable during testing, you should use an empty string vi.stubEnv('NAME', '') instead of passing undefined.
| vi.stubEnv('CODER_AGENT_WORKSPACE_PATH', undefined); | |
| vi.stubEnv('CODER_AGENT_WORKSPACE_PATH', ''); |
References
- To "unset" a variable, use an empty string
vi.stubEnv('NAME', ''). (link)
| vi.spyOn(commandRegistry, 'get').mockReturnValue(mockWorkspaceCommand); | ||
|
|
||
| delete process.env['CODER_AGENT_WORKSPACE_PATH']; | ||
| vi.stubEnv('CODER_AGENT_WORKSPACE_PATH', undefined); |
There was a problem hiding this comment.
According to the repository style guide, to "unset" an environment variable during testing, you should use an empty string vi.stubEnv('NAME', '') instead of passing undefined.
| vi.stubEnv('CODER_AGENT_WORKSPACE_PATH', undefined); | |
| vi.stubEnv('CODER_AGENT_WORKSPACE_PATH', ''); |
References
- To "unset" a variable, use an empty string
vi.stubEnv('NAME', ''). (link)
Use an empty string with vi.stubEnv() when testing an unset CODER_AGENT_WORKSPACE_PATH, matching the repository convention in GEMINI.md and addressing the review feedback on google-gemini#27584. Co-authored-by: Cursor <cursoragent@cursor.com>
GEMINI.md asks tests that depend on environment variables to use vi.stubEnv()/vi.unstubAllEnvs() instead of mutating process.env directly, which can leak state across tests. config.test.ts already follows this; this change migrates the remaining direct process.env usage in the a2a-server tests. - init.test.ts: stub CODER_AGENT_WORKSPACE_PATH in beforeEach and add an afterEach that calls vi.unstubAllEnvs(). - app.test.ts: replace the inline set/delete of CODER_AGENT_WORKSPACE_PATH with vi.stubEnv(); use an empty string when testing an unset value, matching the convention documented in GEMINI.md. No production code changes. Fixes google-gemini#19826
2a77e9d to
eb63c8e
Compare
Summary
Migrate the remaining direct
process.envusage in thea2a-servertests tovi.stubEnv()/vi.unstubAllEnvs(), following the testing convention documented inGEMINI.md. Directprocess.envmutation can leak state between tests.config.test.tsalready follows the convention, so this completes the migration for the package.Details
From
GEMINI.md:Changes:
src/commands/init.test.ts: stubCODER_AGENT_WORKSPACE_PATHinbeforeEach, and add anafterEachthat callsvi.unstubAllEnvs().src/http/app.test.ts: replace the inline set/delete ofCODER_AGENT_WORKSPACE_PATHwithvi.stubEnv(); use an empty string when testing an unset value, matchingGEMINI.md; and addvi.unstubAllEnvs()to the suiteafterEach.No production code changes.
Related Issues
Fixes #19826
How to Validate
From
packages/a2a-server:npx vitest run src/commands/init.test.ts src/http/app.test.ts-> 25 tests pass (5 + 20).npx prettier --check src/commands/init.test.ts src/http/app.test.ts-> passes.npx eslint src/commands/init.test.ts src/http/app.test.ts-> no warnings/errors.Pre-Merge Checklist