fix(tests/bigquery): implement uuid-based isolation and reliable resource cleanup#2547
Conversation
Summary of ChangesHello @NirajNandre, 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 significantly enhances the robustness and reliability of BigQuery integration tests. By standardizing on a single UUID for all test-created resources and implementing a 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 improves BigQuery integration tests by implementing UUID-based isolation and a centralized resource cleanup mechanism. Using a shared uniqueID across a test run ensures that all temporary datasets and tables are consistently named and can be reliably identified for deletion. The identified critical issue in the cleanup implementation, where a non-existent method is called on the BigQuery client, has been retained as it does not contradict any provided rules and requires correction.
6dd191f to
d7b6f39
Compare
|
/gcbrun |
1 similar comment
|
/gcbrun |
113aaac to
a742d84
Compare
|
/gcbrun |
9785760 to
2cdfa16
Compare
|
/gcbrun |
1 similar comment
|
/gcbrun |
caa660b to
033098b
Compare
|
/gcbrun |
|
/gcbrun |
…ble resource cleanup (googleapis#2547) ## Description ### **Description of Changes:** - **Centralized Cleanup**: Added `CleanupBigQueryDatasets` to `tests/common.go` which uses `DeleteWithContents` to recursively wipe test datasets. - **UUID Isolation**: Refactored BigQuery integration tests to generate a single `uniqueID` per run, used as a suffix for all datasets and tables. - **Reliable Teardown**: Replaced defer calls with `t.Cleanup` to guarantee resource removal even after test timeouts or fatal errors. - **Logging**: Added `t.Logf` statements to track the `uniqueID` throughout the test lifecycle for easier debugging. - **Deadlock Resolution (Log Draining)**: Added a background goroutine to drain `cmd.Out` using `io.Copy(io.Discard, cmd.Out)` immediately after the server starts. ### **Why it was required:** - **Prevent Resource Leaks**: Previous tests often leaked "Disallowed" datasets if table deletion failed, cluttering the `toolbox-testing-438616` project. - **Enable Concurrency**: Using a single shared project for CI/CD requires strict isolation so concurrent builds do not delete or overwrite each other's data. - **Timeout Resilience**: Standard `defer` blocks could fail if the test context was cancelled; using `context.Background()` in t.Cleanup ensures teardown logic has a fresh context to complete. - **Timeout & Deadlock Fix**: I observed a deadlock where the Toolbox server would block while attempting to write logs to a full `64KB` OS pipe buffer. Since the test runner was waiting for an HTTP response from the blocked server, the entire suite would time out at 10 minutes. ## PR Checklist > Thank you for opening a Pull Request! Before submitting your PR, there are a > few things you can do to make sure it goes smoothly: - [ ] Make sure you reviewed [CONTRIBUTING.md](https://github.com/googleapis/genai-toolbox/blob/main/CONTRIBUTING.md) - [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/genai-toolbox/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [ ] Appropriate docs were updated (if necessary) - [ ] Make sure to add `!` if this involve a breaking change 🛠️ Fixes #<issue_number_goes_here> 479d842
…ble resource cleanup (googleapis#2547) ## Description ### **Description of Changes:** - **Centralized Cleanup**: Added `CleanupBigQueryDatasets` to `tests/common.go` which uses `DeleteWithContents` to recursively wipe test datasets. - **UUID Isolation**: Refactored BigQuery integration tests to generate a single `uniqueID` per run, used as a suffix for all datasets and tables. - **Reliable Teardown**: Replaced defer calls with `t.Cleanup` to guarantee resource removal even after test timeouts or fatal errors. - **Logging**: Added `t.Logf` statements to track the `uniqueID` throughout the test lifecycle for easier debugging. - **Deadlock Resolution (Log Draining)**: Added a background goroutine to drain `cmd.Out` using `io.Copy(io.Discard, cmd.Out)` immediately after the server starts. ### **Why it was required:** - **Prevent Resource Leaks**: Previous tests often leaked "Disallowed" datasets if table deletion failed, cluttering the `toolbox-testing-438616` project. - **Enable Concurrency**: Using a single shared project for CI/CD requires strict isolation so concurrent builds do not delete or overwrite each other's data. - **Timeout Resilience**: Standard `defer` blocks could fail if the test context was cancelled; using `context.Background()` in t.Cleanup ensures teardown logic has a fresh context to complete. - **Timeout & Deadlock Fix**: I observed a deadlock where the Toolbox server would block while attempting to write logs to a full `64KB` OS pipe buffer. Since the test runner was waiting for an HTTP response from the blocked server, the entire suite would time out at 10 minutes. ## PR Checklist > Thank you for opening a Pull Request! Before submitting your PR, there are a > few things you can do to make sure it goes smoothly: - [ ] Make sure you reviewed [CONTRIBUTING.md](https://github.com/googleapis/genai-toolbox/blob/main/CONTRIBUTING.md) - [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/genai-toolbox/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [ ] Appropriate docs were updated (if necessary) - [ ] Make sure to add `!` if this involve a breaking change 🛠️ Fixes #<issue_number_goes_here> 479d842
🤖 I have created a release *beep* *boop* --- ## [0.32.0](v0.31.0...v0.32.0) (2026-04-08) ### ⚠ BREAKING CHANGES * update repo name ([#2968](#2968)) ### Features * Add MCP tool annotations to all remaining tools ([#2221](#2221)) ([ea09db9](ea09db9)) * **bigquery:** Add conversational analytics tools for Data Agents ([#2517](#2517)) ([2490a4b](2490a4b)) * **embeddingModel:** Add Backend API selection fields ([#2592](#2592)) ([912aa9e](912aa9e)) * **skills:** Add Claude Code support to generated scripts ([#2966](#2966)) ([a1609e1](a1609e1)) * **skills:** Add codex user agent ([#2973](#2973)) ([070e939](070e939)) * **skills:** Tool invocation via npx ([#2916](#2916)) ([377dc5b](377dc5b)) * **sources/singlestore:** Add ConnectionParams to SingleStore Config ([#2555](#2555)) ([73e2a8c](73e2a8c)) * **tool/dataplex-lookup-context:** Relax project constraint and enforce location ([#2952](#2952)) ([7ebfdf1](7ebfdf1)) * **tools/looker:** Looker agent management from MCP ([#2830](#2830)) ([649d4ad](649d4ad)) * **ui:** Update to use `/mcp` endpoint ([#2829](#2829)) ([c3059c2](c3059c2)) ### Bug Fixes * **bigquery:** Add impersonateServiceAccount to prebuilt config ([#2770](#2770)) ([9c3a748](9c3a748)) * **quickstart:** Robust tool lookup and modernize dependencies in Python samples ([#2863](#2863)) ([4c0845d](4c0845d)) * **skills:** Fix skill generation template ([#2914](#2914)) ([a01a15e](a01a15e)) * **skills:** Prevent empty strings overriding optional env vars in node scripts ([#2963](#2963)) ([c52adeb](c52adeb)) * **tests/bigquery:** Implement uuid-based isolation and reliable resource cleanup ([#2547](#2547)) ([479d842](479d842)) * **tests/Bigtable:** Implement uuid-based isolation and reliable resource cleanup ([#2880](#2880)) ([a769f15](a769f15)) * Update error for ConvertConfig function ([#2993](#2993)) ([62bdabb](62bdabb)) ### Code Refactoring * Update repo name ([#2968](#2968)) ([3aae809](3aae809)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Yuan Teoh <45984206+Yuan325@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [0.32.0](v0.31.0...v0.32.0) (2026-04-08) ### ⚠ BREAKING CHANGES * update repo name ([#2968](#2968)) ### Features * Add MCP tool annotations to all remaining tools ([#2221](#2221)) ([ea09db9](ea09db9)) * **bigquery:** Add conversational analytics tools for Data Agents ([#2517](#2517)) ([2490a4b](2490a4b)) * **embeddingModel:** Add Backend API selection fields ([#2592](#2592)) ([912aa9e](912aa9e)) * **skills:** Add Claude Code support to generated scripts ([#2966](#2966)) ([a1609e1](a1609e1)) * **skills:** Add codex user agent ([#2973](#2973)) ([070e939](070e939)) * **skills:** Tool invocation via npx ([#2916](#2916)) ([377dc5b](377dc5b)) * **sources/singlestore:** Add ConnectionParams to SingleStore Config ([#2555](#2555)) ([73e2a8c](73e2a8c)) * **tool/dataplex-lookup-context:** Relax project constraint and enforce location ([#2952](#2952)) ([7ebfdf1](7ebfdf1)) * **tools/looker:** Looker agent management from MCP ([#2830](#2830)) ([649d4ad](649d4ad)) * **ui:** Update to use `/mcp` endpoint ([#2829](#2829)) ([c3059c2](c3059c2)) ### Bug Fixes * **bigquery:** Add impersonateServiceAccount to prebuilt config ([#2770](#2770)) ([9c3a748](9c3a748)) * **quickstart:** Robust tool lookup and modernize dependencies in Python samples ([#2863](#2863)) ([4c0845d](4c0845d)) * **skills:** Fix skill generation template ([#2914](#2914)) ([a01a15e](a01a15e)) * **skills:** Prevent empty strings overriding optional env vars in node scripts ([#2963](#2963)) ([c52adeb](c52adeb)) * **tests/bigquery:** Implement uuid-based isolation and reliable resource cleanup ([#2547](#2547)) ([479d842](479d842)) * **tests/Bigtable:** Implement uuid-based isolation and reliable resource cleanup ([#2880](#2880)) ([a769f15](a769f15)) * Update error for ConvertConfig function ([#2993](#2993)) ([62bdabb](62bdabb)) ### Code Refactoring * Update repo name ([#2968](#2968)) ([3aae809](3aae809)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Yuan Teoh <45984206+Yuan325@users.noreply.github.com> b9ae1c6
🤖 I have created a release *beep* *boop* --- ## [0.32.0](googleapis/mcp-toolbox@v0.31.0...v0.32.0) (2026-04-08) ### ⚠ BREAKING CHANGES * update repo name ([googleapis#2968](googleapis#2968)) ### Features * Add MCP tool annotations to all remaining tools ([googleapis#2221](googleapis#2221)) ([ea09db9](googleapis@ea09db9)) * **bigquery:** Add conversational analytics tools for Data Agents ([googleapis#2517](googleapis#2517)) ([2490a4b](googleapis@2490a4b)) * **embeddingModel:** Add Backend API selection fields ([googleapis#2592](googleapis#2592)) ([912aa9e](googleapis@912aa9e)) * **skills:** Add Claude Code support to generated scripts ([googleapis#2966](googleapis#2966)) ([a1609e1](googleapis@a1609e1)) * **skills:** Add codex user agent ([googleapis#2973](googleapis#2973)) ([070e939](googleapis@070e939)) * **skills:** Tool invocation via npx ([googleapis#2916](googleapis#2916)) ([377dc5b](googleapis@377dc5b)) * **sources/singlestore:** Add ConnectionParams to SingleStore Config ([googleapis#2555](googleapis#2555)) ([73e2a8c](googleapis@73e2a8c)) * **tool/dataplex-lookup-context:** Relax project constraint and enforce location ([googleapis#2952](googleapis#2952)) ([7ebfdf1](googleapis@7ebfdf1)) * **tools/looker:** Looker agent management from MCP ([googleapis#2830](googleapis#2830)) ([649d4ad](googleapis@649d4ad)) * **ui:** Update to use `/mcp` endpoint ([googleapis#2829](googleapis#2829)) ([c3059c2](googleapis@c3059c2)) ### Bug Fixes * **bigquery:** Add impersonateServiceAccount to prebuilt config ([googleapis#2770](googleapis#2770)) ([9c3a748](googleapis@9c3a748)) * **quickstart:** Robust tool lookup and modernize dependencies in Python samples ([googleapis#2863](googleapis#2863)) ([4c0845d](googleapis@4c0845d)) * **skills:** Fix skill generation template ([googleapis#2914](googleapis#2914)) ([a01a15e](googleapis@a01a15e)) * **skills:** Prevent empty strings overriding optional env vars in node scripts ([googleapis#2963](googleapis#2963)) ([c52adeb](googleapis@c52adeb)) * **tests/bigquery:** Implement uuid-based isolation and reliable resource cleanup ([googleapis#2547](googleapis#2547)) ([479d842](googleapis@479d842)) * **tests/Bigtable:** Implement uuid-based isolation and reliable resource cleanup ([googleapis#2880](googleapis#2880)) ([a769f15](googleapis@a769f15)) * Update error for ConvertConfig function ([googleapis#2993](googleapis#2993)) ([62bdabb](googleapis@62bdabb)) ### Code Refactoring * Update repo name ([googleapis#2968](googleapis#2968)) ([3aae809](googleapis@3aae809)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Yuan Teoh <45984206+Yuan325@users.noreply.github.com> b9ae1c6
🤖 I have created a release *beep* *boop* --- ## [0.32.0](googleapis/mcp-toolbox@v0.31.0...v0.32.0) (2026-04-08) ### ⚠ BREAKING CHANGES * update repo name ([googleapis#2968](googleapis#2968)) ### Features * Add MCP tool annotations to all remaining tools ([googleapis#2221](googleapis#2221)) ([ea09db9](googleapis@ea09db9)) * **bigquery:** Add conversational analytics tools for Data Agents ([googleapis#2517](googleapis#2517)) ([2490a4b](googleapis@2490a4b)) * **embeddingModel:** Add Backend API selection fields ([googleapis#2592](googleapis#2592)) ([912aa9e](googleapis@912aa9e)) * **skills:** Add Claude Code support to generated scripts ([googleapis#2966](googleapis#2966)) ([a1609e1](googleapis@a1609e1)) * **skills:** Add codex user agent ([googleapis#2973](googleapis#2973)) ([070e939](googleapis@070e939)) * **skills:** Tool invocation via npx ([googleapis#2916](googleapis#2916)) ([377dc5b](googleapis@377dc5b)) * **sources/singlestore:** Add ConnectionParams to SingleStore Config ([googleapis#2555](googleapis#2555)) ([73e2a8c](googleapis@73e2a8c)) * **tool/dataplex-lookup-context:** Relax project constraint and enforce location ([googleapis#2952](googleapis#2952)) ([7ebfdf1](googleapis@7ebfdf1)) * **tools/looker:** Looker agent management from MCP ([googleapis#2830](googleapis#2830)) ([649d4ad](googleapis@649d4ad)) * **ui:** Update to use `/mcp` endpoint ([googleapis#2829](googleapis#2829)) ([c3059c2](googleapis@c3059c2)) ### Bug Fixes * **bigquery:** Add impersonateServiceAccount to prebuilt config ([googleapis#2770](googleapis#2770)) ([9c3a748](googleapis@9c3a748)) * **quickstart:** Robust tool lookup and modernize dependencies in Python samples ([googleapis#2863](googleapis#2863)) ([4c0845d](googleapis@4c0845d)) * **skills:** Fix skill generation template ([googleapis#2914](googleapis#2914)) ([a01a15e](googleapis@a01a15e)) * **skills:** Prevent empty strings overriding optional env vars in node scripts ([googleapis#2963](googleapis#2963)) ([c52adeb](googleapis@c52adeb)) * **tests/bigquery:** Implement uuid-based isolation and reliable resource cleanup ([googleapis#2547](googleapis#2547)) ([479d842](googleapis@479d842)) * **tests/Bigtable:** Implement uuid-based isolation and reliable resource cleanup ([googleapis#2880](googleapis#2880)) ([a769f15](googleapis@a769f15)) * Update error for ConvertConfig function ([googleapis#2993](googleapis#2993)) ([62bdabb](googleapis@62bdabb)) ### Code Refactoring * Update repo name ([googleapis#2968](googleapis#2968)) ([3aae809](googleapis@3aae809)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Yuan Teoh <45984206+Yuan325@users.noreply.github.com> b9ae1c6
Description
Description of Changes:
CleanupBigQueryDatasetstotests/common.gowhich usesDeleteWithContentsto recursively wipe test datasets.uniqueIDper run, used as a suffix for all datasets and tables.t.Cleanupto guarantee resource removal even after test timeouts or fatal errors.t.Logfstatements to track theuniqueIDthroughout the test lifecycle for easier debugging.cmd.Outusingio.Copy(io.Discard, cmd.Out)immediately after the server starts.Why it was required:
toolbox-testing-438616project.deferblocks could fail if the test context was cancelled; usingcontext.Background()in t.Cleanup ensures teardown logic has a fresh context to complete.64KBOS pipe buffer. Since the test runner was waiting for an HTTP response from the blocked server, the entire suite would time out at 10 minutes.PR Checklist
CONTRIBUTING.md
bug/issue
before writing your code! That way we can discuss the change, evaluate
designs, and agree on the general idea
!if this involve a breaking change🛠️ Fixes #<issue_number_goes_here>