fix(sdk): move dynamic instructions error test to unit test#20409
fix(sdk): move dynamic instructions error test to unit test#20409Rawdyrathaur wants to merge 4 commits intogoogle-gemini:mainfrom
Conversation
Summary of ChangesHello @Rawdyrathaur, 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 an issue where a specific SDK test, designed to propagate errors from dynamic instructions, consistently failed outside of GCP environments due to unintended network calls. The solution involves refactoring this test into a dedicated unit test file and implementing mocking for the Highlights
Changelog
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
|
There was a problem hiding this comment.
Code Review
This pull request successfully addresses the issue of a flaky integration test by moving it to a unit test and mocking external dependencies. This significantly improves test reliability and execution speed. The suggestion to make the mock for the initialize method more robust is valid and should be considered to ensure it fully covers the expected behavior of the mocked object, even for code paths not currently exercised by this specific test.
| vi.spyOn(GeminiCliSession.prototype, 'initialize').mockImplementation( | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| async function (this: any) { | ||
| this.client = { getHistory: () => [] }; | ||
| this.initialized = true; | ||
| }, |
There was a problem hiding this comment.
The mock for GeminiCliSession.prototype.initialize is incomplete. While it correctly mocks getHistory, the sendStream method also calls client.sendMessageStream. If the instructions function did not throw an error, the test would likely fail with a TypeError because sendMessageStream is not defined on the mocked client. A more robust mock should include all methods that sendStream expects to find on the client object to prevent future breakage or unexpected behavior if the test path changes.
vi.spyOn(GeminiCliSession.prototype, 'initialize').mockImplementation(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
async function (this: any) {
this.client = {
getHistory: () => [],
sendMessageStream: async function* () {},
};
this.initialized = true;
},
);5a6ee60 to
32fa265
Compare
Summary
The SDK test
propagates errors from dynamic instructionsalways failed outside GCP because it was missingfakeResponses, causinginitialize()to hit the real GCP metadata server. Moved the test to a unit test file and mockedinitialize()so no network call is made.Details
agent.integration.test.tstoagent.test.tsvi.spyOnto mockGeminiCliSession.prototype.initializeHow to Validat?
npx vitest run packages/sdk/src/agent.test.ts --reporter=verboseRelated Issues
Fixes #20408
How to Validate
npx vitest run packages/sdk/src/agent.test.ts --reporter=verbose ## Pre-Merge Checklist - [x] Added/updated tests (if needed)