fix: presubmit failure for samples backups test #2492
Conversation
Summary of ChangesHello @alkatrivedi, 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 and resolves presubmit failures occurring in the Spanner backup samples tests. The core change involves implementing a robust cleanup mechanism that ensures databases are properly deleted before test retries. This prevents resource conflicts and ensures that each test run, especially after a failure, starts from a clean and predictable state, thereby improving the overall reliability and stability of the test suite. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 introduces a mechanism to clean up resources before retrying a failed test, which is a great way to improve test reliability. A new cleanupDatabase function has been added, and the delay function used for retries has been updated to accept a cleanup function. This is then applied to several backup restoration tests.
The implementation is mostly correct, but I've found one critical issue in one of the tests where the wrong database ID is being used for cleanup, which would prevent the test from recovering correctly on a retry. Please see the specific comment for details.
| await delay(this.test, async () => { | ||
| await cleanupDatabase(INSTANCE_ID, ENCRYPTED_DATABASE_ID); | ||
| }); |
There was a problem hiding this comment.
It seems there's a mismatch in the database being cleaned up. The test 'should restore database from a backup using an encryption key' restores to ENCRYPTED_RESTORE_DATABASE_ID, but the cleanup function passed to delay is attempting to clean up ENCRYPTED_DATABASE_ID. To ensure the correct database is cleaned up on retry, you should use ENCRYPTED_RESTORE_DATABASE_ID in the cleanupDatabase call.
| await delay(this.test, async () => { | |
| await cleanupDatabase(INSTANCE_ID, ENCRYPTED_DATABASE_ID); | |
| }); | |
| await delay(this.test, async () => { | |
| await cleanupDatabase(INSTANCE_ID, ENCRYPTED_RESTORE_DATABASE_ID); | |
| }); |
3d4ba82 to
9d58d9a
Compare
|
/gemini |
|
It looks like you tried to invoke a command, but the correct syntax is For your convenience, here is the help message: 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. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a cleanup mechanism for retrying failing tests, specifically for tests that restore Spanner backups. A cleanupDatabase function is added and integrated into the existing delay function used for test retries. This ensures that any partially created databases from a failed test run are deleted before a retry attempt. My review includes a suggestion to make the cleanupDatabase function more robust by handling cases where the database might not exist, preventing unhandled errors that could disrupt the test retry logic.
| async function cleanupDatabase(instanceId, databaseId) { | ||
| const database = spanner.instance(instanceId).database(databaseId); | ||
| await database.delete(); | ||
| } |
There was a problem hiding this comment.
The cleanupDatabase function could fail if the database it's trying to delete doesn't exist. This might happen if the test fails before the database is created. An unhandled exception here would prevent the test from retrying correctly. It's better to wrap the database.delete() call in a try...catch block and ignore NOT_FOUND errors (gRPC status code 5).
async function cleanupDatabase(instanceId, databaseId) {
const database = spanner.instance(instanceId).database(databaseId);
try {
await database.delete();
} catch (err) {
// The database may not exist if the test failed before it was created.
// In that case, we can ignore the error.
if (err.code !== 5) { // 5 = grpc.status.NOT_FOUND
throw err;
}
}
}🤖 I have created a release *beep* *boop* --- ## [8.4.0](https://togithub.com/googleapis/nodejs-spanner/compare/v8.3.1...v8.4.0) (2026-01-09) ### Features * Add a ClientContext field to Spanner requests ([#2493](https://togithub.com/googleapis/nodejs-spanner/issues/2493)) ([37504ad](https://togithub.com/googleapis/nodejs-spanner/commit/37504adcc37a7e95acfb2530313ff783d0c1fe7d)) * Exposing total CPU related fields in AutoscalingConfig ([#2490](https://togithub.com/googleapis/nodejs-spanner/issues/2490)) ([508f0ff](https://togithub.com/googleapis/nodejs-spanner/commit/508f0ff95636b004f4200522018a199263eda8ca)) * **spanner:** Support for type UUID ([#2482](https://togithub.com/googleapis/nodejs-spanner/issues/2482)) ([0047e94](https://togithub.com/googleapis/nodejs-spanner/commit/0047e9407d86521571626c69011b70307f83f8ba)) ### Bug Fixes * **deps:** Update dependency google-gax to v5.0.6 ([#2452](https://togithub.com/googleapis/nodejs-spanner/issues/2452)) ([f9e6b86](https://togithub.com/googleapis/nodejs-spanner/commit/f9e6b86ff4da03110642c17e5ebc8fac8d903d3a)) * Flaky metric test ([#2472](https://togithub.com/googleapis/nodejs-spanner/issues/2472)) ([e169cc5](https://togithub.com/googleapis/nodejs-spanner/commit/e169cc5344d38812b1ebf20c7a987715a73d6f79)) * Memory leak and deadlock due to error event in multiplexed session ([#2477](https://togithub.com/googleapis/nodejs-spanner/issues/2477)) ([c624619](https://togithub.com/googleapis/nodejs-spanner/commit/c624619a3960892b1d2d412ff79faa5a74de45df)) * Presubmit failure for samples backups test ([#2492](https://togithub.com/googleapis/nodejs-spanner/issues/2492)) ([01eb3d5](https://togithub.com/googleapis/nodejs-spanner/commit/01eb3d5801ddb21517f185b9d585fbce4fa1475c)) * Type check for key in deleteRows ([#2486](https://togithub.com/googleapis/nodejs-spanner/issues/2486)) ([7347a16](https://togithub.com/googleapis/nodejs-spanner/commit/7347a1628ad8635b8f84b36ad1d3850b78862ac7)) * Type mismatch in Snapshot.run error handler ([#2487](https://togithub.com/googleapis/nodejs-spanner/issues/2487)) ([4ac0360](https://togithub.com/googleapis/nodejs-spanner/commit/4ac036047e3a03c073300f288c746389a38d8e42)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
The restore backup tests are failing because of timeouts.
Since the database is created and the restore operation getting timeout, the automatic retry logic fails with a
DATABASE ALREADY EXISTSerror. A cleanup logic has been added to delete the existing database before the retry logic triggers.