test: expand coverage for GenAIApp #38
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors the test harness to iterate across multiple models with an optional afterRun hook, removes explicit vision enabling, and adds four tests for file attachment, metadata/log logging, response ID tracking, and vector store lifecycle using blob utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as runTestAcrossModels
participant Model as Model (GPT/Reasoning/Gemini)
participant Chat as Chat
participant After as afterRun (optional)
Runner->>Model: for each model
Runner->>Chat: create chat for model
Chat->>Model: run(test setup/options)
Model-->>Chat: response
Chat-->>Runner: response
alt afterRun provided
Runner->>After: afterRun(chat, response, model)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes ✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/testFunctions.gs (1)
23-44: Bug: runOptions can override the per-iteration model. Also, isolate per-model failures so one failure doesn’t stop the batch.Because
{ model: model.name, ...runOptions }puts runOptions last, any caller-providedrunOptions.modelwill silently override the model under test. Reverse the merge order and add per-model try/catch to keep the runner resilient.Apply this diff:
models.forEach(model => { - const chat = GenAIApp.newChat(); - setupFunction(chat); - const options = { model: model.name, ...runOptions }; - const response = chat.run(options); - console.log(`${testName} ${model.label}:\n${response}`); - if (afterRun) { - afterRun(chat, response, model); - } + try { + const chat = GenAIApp.newChat(); + setupFunction(chat); + const options = { ...runOptions, model: model.name }; // ensure the loop's model wins + const response = chat.run(options); + console.log(`${testName} ${model.label}:\n${response}`); + if (afterRun) { + afterRun(chat, response, model); + } + } catch (err) { + console.error(`Error running "${testName}" for ${model.label}: ${err && err.stack || err}`); + } });Optional: Consider setting API keys once (e.g., in testAll) to avoid repeated calls in every runTestAcrossModels invocation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/testFunctions.gs(3 hunks)
🔇 Additional comments (1)
src/testFunctions.gs (1)
137-144: Verify addFile support across all models or gate the test per capability.Attaching files may not be supported uniformly (e.g., some model backends may not accept file blobs). If unsupported, this will fail that iteration and abort the rest unless the runner is hardened.
Would you like me to add a capability check and skip the model(s) that don’t support addFile?
Comment on lines
+169
to
+191
| function testResponseIdTracking() { | ||
| GenAIApp.setGeminiAPIKey(GEMINI_API_KEY); | ||
| GenAIApp.setOpenAIAPIKey(OPEN_AI_API_KEY); | ||
|
|
||
| const models = [ | ||
| { name: GPT_MODEL, label: "GPT" }, | ||
| { name: REASONING_MODEL, label: "reasoning" }, | ||
| { name: GEMINI_MODEL, label: "gemini" } | ||
| ]; | ||
|
|
||
| models.forEach(model => { | ||
| const chat = GenAIApp.newChat(); | ||
| chat.addMessage("Hello"); | ||
| chat.run({ model: model.name }); | ||
| const lastId = chat.retrieveLastResponseId(); | ||
| chat.addMessage("Continue this conversation."); | ||
| if (lastId) { | ||
| chat.setPreviousResponseId(lastId); | ||
| } | ||
| const response = chat.run({ model: model.name }); | ||
| console.log(`Response ID test ${model.label}:\n${response}`); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Harden the response ID test: log the retrieved ID and isolate per-model failures.
This reduces flakiness, surfaces whether an ID was retrieved, and prevents one failure from aborting subsequent iterations. Also, API keys are redundantly set elsewhere; ideally centralize.
Apply this diff:
- models.forEach(model => {
- const chat = GenAIApp.newChat();
- chat.addMessage("Hello");
- chat.run({ model: model.name });
- const lastId = chat.retrieveLastResponseId();
- chat.addMessage("Continue this conversation.");
- if (lastId) {
- chat.setPreviousResponseId(lastId);
- }
- const response = chat.run({ model: model.name });
- console.log(`Response ID test ${model.label}:\n${response}`);
- });
+ models.forEach(model => {
+ try {
+ const chat = GenAIApp.newChat();
+ chat.addMessage("Hello");
+ chat.run({ model: model.name });
+ const lastId = chat.retrieveLastResponseId();
+ console.log(`Last response id for ${model.label}: ${lastId || 'n/a'}`);
+ chat.addMessage("Continue this conversation.");
+ if (lastId) {
+ chat.setPreviousResponseId(lastId);
+ } else {
+ console.warn(`No response id retrieved for ${model.label}; running without previous id`);
+ }
+ const response = chat.run({ model: model.name });
+ console.log(`Response ID test ${model.label}:\n${response}`);
+ } catch (err) {
+ console.error(`Response ID test failed for ${model.label}: ${err && err.stack || err}`);
+ }
+ });Please confirm that retrieveLastResponseId/setPreviousResponseId are implemented for all three models; if not, we should feature-detect and skip unsupported models.
🏁 Script executed:
#!/bin/bash
# Search for retrieveLastResponseId and setPreviousResponseId definitions across .gs files
rg -n --glob '*.gs' 'retrieveLastResponseId'
rg -n --glob '*.gs' 'setPreviousResponseId'Length of output: 379
🏁 Script executed:
#!/bin/bash
# Display implementation of retrieveLastResponseId and setPreviousResponseId in code.gs
sed -n '300,360p' src/code.gsLength of output: 1871
Harden the response ID test and confirm ID methods exist
I’ve verified that retrieveLastResponseId and setPreviousResponseId are implemented on the Chat object, so no per-model feature detection is needed. Applying the below diff will:
- Log the retrieved ID (or warn if missing)
- Isolate failures per model to avoid aborting the loop
- Centralize error handling with
try/catch
Also, consider centralizing API-key setup in your test harness instead of setting them in each test.
Apply this diff in src/testFunctions.gs (lines 169–191):
- models.forEach(model => {
- const chat = GenAIApp.newChat();
- chat.addMessage("Hello");
- chat.run({ model: model.name });
- const lastId = chat.retrieveLastResponseId();
- chat.addMessage("Continue this conversation.");
- if (lastId) {
- chat.setPreviousResponseId(lastId);
- }
- const response = chat.run({ model: model.name });
- console.log(`Response ID test ${model.label}:\n${response}`);
- });
+ models.forEach(model => {
+ try {
+ const chat = GenAIApp.newChat();
+ chat.addMessage("Hello");
+ chat.run({ model: model.name });
+ const lastId = chat.retrieveLastResponseId();
+ console.log(`Last response id for ${model.label}: ${lastId || 'n/a'}`);
+ chat.addMessage("Continue this conversation.");
+ if (lastId) {
+ chat.setPreviousResponseId(lastId);
+ } else {
+ console.warn(`No response id retrieved for ${model.label}; running without previous id`);
+ }
+ const response = chat.run({ model: model.name });
+ console.log(`Response ID test ${model.label}:\n${response}`);
+ } catch (err) {
+ console.error(`Response ID test failed for ${model.label}: ${err.stack || err}`);
+ }
+ });🤖 Prompt for AI Agents
In src/testFunctions.gs around lines 169 to 191, the response ID test should be
hardened: wrap each model iteration in a try/catch so one model failure doesn't
abort the loop; after first run capture the ID and log it (or console.warn if
missing) before calling setPreviousResponseId; ensure setPreviousResponseId is
only called when an ID exists; and log per-model errors inside the catch so
failures are isolated and informative; also consider moving the API-key setup
out of this test into shared test harness rather than setting keys here.
Comment on lines
+193
to
+210
| function testVectorStoreLifecycle() { | ||
| GenAIApp.setOpenAIAPIKey(OPEN_AI_API_KEY); | ||
| const blob = Utilities.newBlob("Vector store content", "text/plain", "vs.txt"); | ||
|
|
||
| const store = GenAIApp.newVectorStore() | ||
| .setName("test-store-" + Date.now()) | ||
| .setDescription("Temporary store for tests") | ||
| .createVectorStore(); | ||
|
|
||
| const storeId = store.getId(); | ||
| const fileId = store.uploadAndAttachFile(blob, { source: "test" }); | ||
| const files = store.listFiles(); | ||
| console.log(`Vector store files: ${JSON.stringify(files)}`); | ||
|
|
||
| store.deleteFile(fileId); | ||
| store.deleteVectorStore(); | ||
| console.log(`Vector store ${storeId} cleaned up.`); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent resource leaks in vector store tests; add try/finally and brief wait for eventual consistency.
If any step throws, the vector store won’t be deleted, leaving orphaned resources. Also, a short delay before listing files can reduce flakiness while the backend indexes the upload.
Apply this diff:
- const store = GenAIApp.newVectorStore()
- .setName("test-store-" + Date.now())
- .setDescription("Temporary store for tests")
- .createVectorStore();
-
- const storeId = store.getId();
- const fileId = store.uploadAndAttachFile(blob, { source: "test" });
- const files = store.listFiles();
- console.log(`Vector store files: ${JSON.stringify(files)}`);
-
- store.deleteFile(fileId);
- store.deleteVectorStore();
- console.log(`Vector store ${storeId} cleaned up.`);
+ const store = GenAIApp.newVectorStore()
+ .setName("test-store-" + Date.now())
+ .setDescription("Temporary store for tests")
+ .createVectorStore();
+
+ const storeId = store.getId();
+ let fileId;
+ try {
+ fileId = store.uploadAndAttachFile(blob, { source: "test" });
+ // Brief wait for indexing/consistency if backend is eventually consistent
+ Utilities.sleep(1500);
+ const files = store.listFiles();
+ console.log(`Vector store files: ${JSON.stringify(files)}`);
+ if (fileId) {
+ store.deleteFile(fileId);
+ }
+ } finally {
+ store.deleteVectorStore();
+ console.log(`Vector store ${storeId} cleaned up.`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function testVectorStoreLifecycle() { | |
| GenAIApp.setOpenAIAPIKey(OPEN_AI_API_KEY); | |
| const blob = Utilities.newBlob("Vector store content", "text/plain", "vs.txt"); | |
| const store = GenAIApp.newVectorStore() | |
| .setName("test-store-" + Date.now()) | |
| .setDescription("Temporary store for tests") | |
| .createVectorStore(); | |
| const storeId = store.getId(); | |
| const fileId = store.uploadAndAttachFile(blob, { source: "test" }); | |
| const files = store.listFiles(); | |
| console.log(`Vector store files: ${JSON.stringify(files)}`); | |
| store.deleteFile(fileId); | |
| store.deleteVectorStore(); | |
| console.log(`Vector store ${storeId} cleaned up.`); | |
| } | |
| function testVectorStoreLifecycle() { | |
| GenAIApp.setOpenAIAPIKey(OPEN_AI_API_KEY); | |
| const blob = Utilities.newBlob("Vector store content", "text/plain", "vs.txt"); | |
| const store = GenAIApp.newVectorStore() | |
| .setName("test-store-" + Date.now()) | |
| .setDescription("Temporary store for tests") | |
| .createVectorStore(); | |
| const storeId = store.getId(); | |
| let fileId; | |
| try { | |
| fileId = store.uploadAndAttachFile(blob, { source: "test" }); | |
| // Brief wait for indexing/consistency if backend is eventually consistent | |
| Utilities.sleep(1500); | |
| const files = store.listFiles(); | |
| console.log(`Vector store files: ${JSON.stringify(files)}`); | |
| if (fileId) { | |
| store.deleteFile(fileId); | |
| } | |
| } finally { | |
| store.deleteVectorStore(); | |
| console.log(`Vector store ${storeId} cleaned up.`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/testFunctions.gs around lines 193 to 210, the test currently uploads a
file and deletes the vector store without protection against exceptions and
without a short wait for eventual indexing; wrap the upload/list/delete
lifecycle in a try/finally so the file and the vector store are always removed
even if an intermediate step throws, and insert a brief sleep (e.g., a few
hundred milliseconds) after upload before listing files to reduce flakiness from
eventual consistency; ensure you capture the returned fileId so the finally
block can safely delete the file (guarding deletion calls if ids are undefined)
and still delete the store in all cases.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Testing
npm test(fails: Could not read package.json)https://chatgpt.com/codex/tasks/task_b_689c3e2f1bb0832f9bfdf6cc354ae817