fix(source/cloud-healthcare): make integration test store cleanup deterministic#2962
fix(source/cloud-healthcare): make integration test store cleanup deterministic#2962Iheanacho-ai wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the Cloud Healthcare integration tests to implement synchronous resource deletion with polling and timeouts. The reviewer identified several areas for improvement: restoring age-based filtering in cleanupOrphanedStores to prevent flakiness in shared environments, handling ignored errors from the Pages function, and avoiding synchronous waits during initial cleanup to reduce test latency. Additionally, it is recommended to use errors.As for more robust error type checking when handling API responses.
There was a problem hiding this comment.
Code Review
This pull request refactors the integration test cleanup logic for Cloud Healthcare FHIR and DICOM stores, transitioning from age-based deletion to name-pattern matching. It introduces helper functions such as waitForStoreDeletion and deleteFHIRStoreAndWait to ensure resources are properly removed with appropriate timeouts. The reviewer recommended using errors.As instead of direct type assertions for googleapi.Error to align with idiomatic Go error handling practices.
There was a problem hiding this comment.
Code Review
This pull request refactors the Cloud Healthcare integration tests to improve resource cleanup by introducing helper functions that delete FHIR and DICOM stores and wait for their deletion. The previous time-based cleanup logic has been replaced with a more robust polling mechanism. The reviewer suggested adding a timeout to the cleanup context in cleanupOrphanedStores to prevent potential hangs, handling errors when listing stores for cleanup, and replacing a magic number with a constant for the polling interval.
|
|
||
| func cleanupOrphanedStores(ctx context.Context, service *healthcare.Service) { | ||
| now := time.Now().Unix() | ||
| datasetName := fmt.Sprintf("projects/%s/locations/%s/datasets/%s", healthcareProject, healthcareRegion, healthcareDataset) |
There was a problem hiding this comment.
The cleanupOrphanedStores function is called from TestMain with context.Background(), which has no timeout. The new waiting logic could cause the test setup to hang indefinitely if a store is not deleted. To prevent this, you should create a new context with a timeout at the beginning of this function and use it for all subsequent API calls.
ctx, cancel := context.WithTimeout(ctx, testStoreDeleteTimeout)
defer cancel()
datasetName := fmt.Sprintf("projects/%s/locations/%s/datasets/%s", healthcareProject, healthcareRegion, healthcareDataset)…erministic Signed-off-by: Amarachi Iheanacho <amarachi.iheanacho@siderolabs.com>
|
The issue was previously fixed in #2742, closing this PR due to redundancy for now. Please submit an issue if you found any bugs or issue with the current implementation. Thank you! |
Description
This fixes flaky Cloud Healthcare integration tests by improving cleanup for test-created FHIR and DICOM stores in the shared dataset.
The change:
• cleans up test stores by naming pattern at test startup
• waits for store deletion to complete during test cleanup
This keeps the current shared-dataset test setup and avoids broader test infrastructure changes.
A stronger long-term option would be to use isolated datasets per test run and delete the dataset after each run.
PR Checklist
• [ ] Make sure to open an issue as a bug/issue before writing your code!
• [ ] 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 involves a breaking change
Issue Reference
Fixes #2685