[sakthivelmanii]

Problem

• The system session in packages/django-google-spanner/noxfile.py (tests/system) was skipped with session.skip("System tests are temporarily disabled.").
• When enabled against live Cloud Spanner, each of the 4 system test classes (TestCheckConstraint, TestDecimal, TestJsonField, TestQueries) created and dropped the database and ran full call_command("migrate") across unused django.contrib.* apps (auth, contenttypes, sites, sessions) 4 separate times, causing the 8-test suite to take ~52 minutes.

Solution

• Remove session.skip("System tests are temporarily disabled.") from def system(session): in packages/django-google-spanner/noxfile.py.
• Remove unused django.contrib.* entries from INSTALLED_APPS in tests/settings.py and generate a unique database name per test session (dj_sys_<timestamp>_<hex6>).
• Update tests/system/django_spanner/utils.py to connect to the existing shared Spanner instance on live Spanner (creating an instance only on the Emulator), clean up stale dj_sys_<timestamp>_* databases older than 1 hour, reuse the active database across all system test classes in the session, and drop the database via an atexit cleanup handler.

[gemini-code-assist]

Code Review

This pull request re-enables system tests, introduces dynamic database naming in settings, and refactors test resource setup and teardown in utils.py using an atexit cleanup handler. The review feedback highlights three key issues: first, removing the CREATE_INSTANCE guard during instance checks could leak orphaned Spanner instances when CREATE_INSTANCE is disabled; second, using the un-prefixed database name in setup_database leads to duplicate database creation alongside Django's prefixed test database; and third, generating random database names per run risks accumulating orphaned databases if tests terminate abruptly, suggesting a static naming strategy or a startup cleanup routine instead.

[gemini-code-assist]

By removing the if CREATE_INSTANCE: check and instead checking if not Config.INSTANCE.exists(), the test suite will now automatically create a Spanner instance if it does not exist, even if GOOGLE_CLOUD_TESTS_CREATE_SPANNER_INSTANCE is not set.

However, in _cleanup_test_resources(), the instance is only deleted if CREATE_INSTANCE is True:

if CREATE_INSTANCE and Config.INSTANCE is not None:
    Config.INSTANCE.delete()

This means if CREATE_INSTANCE is False and the instance did not exist, the suite will create the Spanner instance but never delete it, leading to extremely expensive orphaned Spanner instances running indefinitely.

To prevent this, restore the CREATE_INSTANCE guard so that instances are only created when explicitly requested.

Suggested change

	if not Config.INSTANCE.exists():
	if CREATE_INSTANCE and not Config.INSTANCE.exists():

References

1. Avoid eagerly creating or initializing a client or resource if a downstream process can lazily create it only when needed.

[sakthivelmanii]

Removed live Spanner instance creation and deletion from utils.py since the shared test instance already exists in CI (only creating an instance when running against the Emulator).

[gemini-code-assist]

When running tests via Django's test runner (e.g., manage.py test), Django automatically prefixes the database name with test_ (e.g., test_dj_sys_...) and updates connections['default'].settings_dict['NAME'] accordingly.

However, DATABASE_NAME in utils.py is initialized at module-import time using settings.DATABASES['default']['NAME'], which does not have the test_ prefix (e.g., dj_sys_...).

As a result:

1. setup_database() checks and creates the un-prefixed database (dj_sys_...).
2. Django's test runner creates and runs tests against the prefixed database (test_dj_sys_...).
3. This leads to two databases being created on Spanner, with the un-prefixed one being completely unused and wasted, doubling the creation/deletion overhead.

To fix this, retrieve the active database name dynamically from connections['default'].settings_dict['NAME'] inside setup_database() instead of using the module-level DATABASE_NAME constant.

Suggested change

	if Config.DATABASE is not None:
	return
	Config.DATABASE = Config.INSTANCE.database(DATABASE_NAME)
	if not Config.DATABASE.exists():
	creation = DatabaseCreation(connection)
	creation._create_test_db(verbosity=0, autoclobber=False, keepdb=True)
	# Running migrations on the db.
	call_command("migrate", interactive=False)
	try:
	created_op = Config.DATABASE.create()
	created_op.result(SPANNER_OPERATION_TIMEOUT_IN_SECONDS)
	except exceptions.AlreadyExists:
	pass
	if Config.DATABASE is not None:
	return
	active_db_name = connections['default'].settings_dict['NAME']
	Config.DATABASE = Config.INSTANCE.database(active_db_name)
	if not Config.DATABASE.exists():
	try:
	created_op = Config.DATABASE.create()
	created_op.result(SPANNER_OPERATION_TIMEOUT_IN_SECONDS)
	except exceptions.AlreadyExists:
	pass

[sakthivelmanii]

Updated setup_database() to read connections['default'].settings_dict['NAME'] dynamically and removed the redundant module-level DATABASE_NAME constant.

[gemini-code-assist]

Generating a completely random database name (dj_sys_{int(time.time())}_{uuid.uuid4().hex[:6]}) per test run can lead to orphaned databases accumulating in the Spanner instance if a test run crashes or is terminated abruptly (e.g., via SIGKILL, CI timeout, or runner cancellation), as atexit handlers do not run in those cases.

Since Spanner has a limit on the number of databases per instance (typically 100), these orphaned databases will eventually cause all subsequent test runs to fail with quota/limit errors.

Consider one of the following improvements:

1. Use a static database name (or a name based on a stable environment variable like the CI build ID/job ID) so that subsequent runs can reuse or clean up the same database.
2. Implement a startup cleanup routine in setup_instance() that lists existing databases in the Spanner instance and deletes any databases with the dj_sys_ prefix that are older than a certain threshold (e.g., 2 hours).

[sakthivelmanii]

Added _cleanup_stale_databases() in setup_instance() to drop any stale dj_sys_<timestamp>_* databases older than 1 hour.

[olavloite]

The changes achieve the main objective: sharing a single database and removing the unused django.contrib.* apps reduced the system test execution time in Kokoro from ~52 minutes down to under 3 minutes.

The current failure in Kokoro (TestJsonField.test_insert_and_fetch_value) is caused by an orthogonal JSON serialization issue that is being addressed in a separate PR and is unrelated to this change.

A few suggestions before merging:

1. Database name length and entropy

The current template f"dj_sys_{int(time.time())}_{uuid.uuid4().hex[:6]}" generates valid 24-character database IDs against Spanner’s 30-character limit. However, time.time() only provides 1-second resolution, so uniqueness across concurrent matrix runs started within the same second relies on 6 hex characters (24 bits).

• Expanding to hex[:12] (e.g. f"dj_sys_{int(time.time())}_{uuid.uuid4().hex[:12]}") uses the full 30 characters allowed by Spanner and increases entropy to 48 bits, virtually eliminating collision risk.

2. Stale database cleanup metadata

In _cleanup_stale_databases, parsing the creation timestamp out of the database ID via .split("_") is somewhat brittle. Cloud Spanner database protobufs already include native creation metadata in database_pb.create_time. You can inspect database_pb.create_time directly when iterating over instance.list_databases() rather than relying on string conventions (similar to cleanup_stale_databases in packages/google-cloud-spanner/tests/system/_helpers.py).

3. Session lifecycle (atexit vs pytest fixture)

Relying on Python's atexit to execute remote gRPC operations can be fragile if background channels or worker threads are already shutting down. Since the system tests are driven by pytest, managing the database creation and deletion via a session-scoped fixture in tests/system/conftest.py is more idiomatic and deterministic. It also avoids leaving teardown_database() and teardown_instance() as empty stub calls in each test class's tearDownClass.

4. Table DDL churn (optional optimization)

Each test class still creates tables in setUpClass() and drops them in tearDownClass(). Because Cloud Spanner DDL statements take several seconds each, table creation and deletion accounts for most of the ~3-minute runtime. Creating all test model tables (Event, Author, Number, Detail) once during initial database setup—and letting each test clean up its own rows—would reduce the runtime even further.

5. Minor deprecation warning

In tests/system/django_spanner/models.py:33, models.CheckConstraint(check=...) generates a RemovedInDjango60Warning: CheckConstraint.check is deprecated in favor of .condition. Since this repository supports Django 6.0, updating this to condition= would keep the test output clean.

[olavloite]

I'm not sure if this will always work.