Repository navigation
Conversation
initTable creates a table and then runs four INSERTs. If any insert failed, t.Fatal fired before the matching `defer dropTable` was registered, so the table leaked on the shared cluster. Register the drop via t.Cleanup before creating each table (using DROP TABLE IF EXISTS so it is safe if creation itself failed), and close the session via t.Cleanup so it outlives the table drops.
dropTable only log.Printf'd errors, so failed drops never surfaced and leaked tables on the shared cluster went unnoticed. Report them with t.Errorf so a failed teardown fails the test.
The tests used the fixed keyspace example_keyspace, created with IF NOT EXISTS and never dropped, so every run shared it on the long-lived cluster and it was never cleaned up. Generate a UUID-suffixed keyspace per run and drop it via t.Cleanup after the table drops.
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the Cassandra integration tests to dynamically spin up an ephemeral Cassandra container using testcontainers-go when CASSANDRA_HOST is not set, allowing the removal of Cassandra secrets from the Cloud Build configuration. It also improves test isolation with per-run keyspaces and utilizes Go's t.Cleanup for robust resource management. One issue was identified in the container setup where registering a cleanup on a potentially nil container could cause a panic during test teardown.
Run the Cassandra integration tests against an ephemeral cassandra container instead of a long-lived shared cluster. Setting CASSANDRA_HOST (plus CASSANDRA_USER and CASSANDRA_PASS) still runs the tests against an external cluster. The container: - runs cassandra:5.0.9 with PasswordAuthenticator enabled, matching the shared cluster, and only the CQL port published, - waits for the CQL port and the default superuser role to be created, - provisions a dedicated role with a random per-run password, then rotates the default superuser to a random password, - is cleaned up via testcontainers.CleanupContainer, and the package-level connection variables are restored so repeated runs (-count>1) start a fresh container. Drop the CASSANDRA_* secrets from the integration Cloud Build config.
emagda97
force-pushed
the
fix/cassandra-testcontainers
branch
from
November 4, 2026 21:50
44f924c to
1af9526
Compare
duwenxin99
approved these changes
Nov 4, 2026
This branch has not been deployed
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
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.
Description
Fixes three resource-cleanup issues in the Cassandra integration test, then migrates it to testcontainers (following the pattern used for ScyllaDB, CockroachDB, TiDB and others), so CI no longer depends on a long-lived shared Cassandra cluster or its stored credentials.
One commit per change:
initTablecreates a table and then runs four INSERTs. If an insert failed,t.Fatalfired before the matchingdefer dropTablewas registered, so the table leaked. The drop is now registered viat.Cleanup(withDROP TABLE IF EXISTS) before the table is created.dropTableonlylog.Printf'd errors, so failed drops never surfaced. They now fail the test witht.Errorf.example_keyspacewas created withIF NOT EXISTS, shared by every run, and never dropped. Each run now uses a UUID-suffixed keyspace that is dropped on cleanup.CASSANDRA_HOSTis unset, the test starts an ephemeralcassandra:5.0.9container:PasswordAuthenticatoris enabled to match the shared cluster, and only the CQL port is published.cassandrasuperuser to a random password.-count>1) get a fresh container.CASSANDRA_HOST,CASSANDRA_USERandCASSANDRA_PASSstill runs the tests against an external cluster..ci/integration.cloudbuild.yaml: removes thecassandra_user/cassandra_pass/cassandra_hostsecrets.Security: no source or tool changes. Test credentials are now generated per run inside an ephemeral container instead of being stored as CI secrets.
Note: retiring the old shared cluster and its secrets is a separate follow-up after merge.
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
review
!if this involve a breaking change