Repository navigation
test(django-google-spanner): run live Spanner test apps in parallel with unique DB per app - #18983
sakthivelmanii wants to merge 3 commits into
Conversation
…ith unique DB per app
## Problem
- `run_testing_worker.py` invokes `django_test_suite_6.0.sh` against a live Cloud Spanner instance without setting `RUNNING_SPANNER_BACKEND_TESTS=1`, leaving `supports_foreign_keys = True` and `sql_create_fk` enabled during Django test database setup and causing every test app to execute synchronous `ALTER TABLE ... ADD CONSTRAINT ... FOREIGN KEY` deferred DDL operations.
- `django_test_suite_6.0.sh` and `django_test_suite_5.2.sh` reuse a single database name and execute `python3 runtests.py $DJANGO_TEST_APP` sequentially across all apps in `django_test_apps.txt` when running against a live Cloud Spanner instance.
## Solution
- Set `RUNNING_SPANNER_BACKEND_TESTS=1` in `run_testing_worker.py` and default `RUNNING_SPANNER_BACKEND_TESTS="${RUNNING_SPANNER_BACKEND_TESTS:-1}"` in `django_test_suite_6.0.sh` and `django_test_suite_5.2.sh` so foreign key DDL constraints are skipped, unsupported tests in `DatabaseFeatures.skip_tests` are marked skipped, and test column names are sanitized.
- Generate a dedicated database name and settings file per test app with a unique random ID per run (`${APP_PREFIX}_${RUN_ID}`) in `django_test_suite_6.0.sh` and `django_test_suite_5.2.sh`, and run all test apps concurrently in parallel on the same instance when running against live Cloud Spanner (`SPANNER_EMULATOR_HOST` unset).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces parallel execution for Django test suites when running against a live Google Cloud Spanner instance. It generates unique database names and settings files per app to support concurrency. However, a critical issue was identified: if SPANNER_TEST_DB is predefined, the database name and settings file will be identical across all test apps, leading to conflicts and test failures during parallel execution. It is recommended to append the app prefix to SPANNER_TEST_DB to guarantee uniqueness while respecting Spanner's 30-character database ID limit.
olavloite
left a comment
olavloite
left a comment
There was a problem hiding this comment.
The changes in this PR are not actually being tested in the PR checks, as the integration tests on real Spanner do not run for pull requests, and the intended changes for concurrency on real Spanner are not used for the Emulator. I think that it would be better to refactor this in such a way that:
- The Emulator and real Spanner use (as much as possible) the same setup.
- The Emulator does not have a problem with running test suites in parallel if each test suite uses its own database. The Emulator only has limitations for concurrency within a database. So running test suites with separate databases concurrently on the emulator should work fine.
| APP_PREFIX=$(echo "$DJANGO_TEST_APP" | tr -cd 'a-z0-9' | cut -c1-6) | ||
| RUN_ID=$(python3 -c 'import os; print(os.urandom(6).hex())') | ||
| if [ -n "$SPANNER_TEST_DB" ]; then | ||
| DB_PREFIX=$(echo "$SPANNER_TEST_DB" | cut -c1-20) | ||
| TEST_DBNAME="${DB_PREFIX}_${APP_PREFIX}" | ||
| else | ||
| TEST_DBNAME="${APP_PREFIX}_${RUN_ID}" | ||
| fi | ||
| TEST_DBNAME_OTHER="${TEST_DBNAME}-ot" | ||
| SETTINGS_FILE="${TEST_DBNAME}_settings" |
There was a problem hiding this comment.
I think that there is a high risk of name collisions here. Also, won't all of the tests use the same DB name if SPANNER_TEST_DB is set?
| if [ -n "$SPANNER_TEST_DB" ]; then | ||
| DB_PREFIX=$(echo "$SPANNER_TEST_DB" | cut -c1-20) | ||
| TEST_DBNAME="${DB_PREFIX}_${APP_PREFIX}" |
There was a problem hiding this comment.
I think that this still risks exceeding the 30 char limit for a database name, as Django adds a 'test_' prefix for the database names.
| TEST_DBNAME_OTHER="${TEST_DBNAME}-ot" | ||
| SETTINGS_FILE="${TEST_DBNAME}_settings" | ||
| create_settings | ||
| if [ -z "$SPANNER_EMULATOR_HOST" ]; then |
There was a problem hiding this comment.
The GitHub pull request checks set SPANNER_EMULATOR_HOST, so the non-emulator path is never tested for this pull request. And I think that the intention to limit the concurrency to max 4 does not actually work if you run this on real Spanner (or if you were to enable concurrency on the Emulator).
| TEST_DBNAME=${SPANNER_TEST_DB:-$(python3 -c 'import os, time; print(chr(ord("a") + time.time_ns() % 26)+os.urandom(10).hex())')} | ||
| TEST_DBNAME_OTHER="$TEST_DBNAME-ot" | ||
| INSTANCE=${SPANNER_TEST_INSTANCE:-spanner-django-python-systest} | ||
| PROJECT=${PROJECT_ID:-$GOOGLE_CLOUD_PROJECT} | ||
| SETTINGS_FILE="$TEST_DBNAME-settings" | ||
| TESTS_DIR=${DJANGO_TESTS_DIR:-django_tests} |
There was a problem hiding this comment.
Can't we remove (some of) this here as well?
| python3 runtests.py $DJANGO_TEST_APP --verbosity=3 --noinput --settings $SETTINGS_FILE || EXIT_STATUS=$? | ||
| APP_PREFIX=$(echo "$DJANGO_TEST_APP" | tr -cd 'a-z0-9' | cut -c1-6) | ||
| RUN_ID=$(python3 -c 'import os; print(os.urandom(6).hex())') | ||
| if [ -n "$SPANNER_TEST_DB" ]; then |
There was a problem hiding this comment.
This will also fail if the database name length is too long (here we do not even try to truncate it)
Problem
django_test_suite_5.2.shanddjango_test_suite_6.0.sh) against live Spanner, sharing a single database or creating separate Spanner instances per worker leads to DDL lock contention, instance quota exhaustion, or long sequential test runtimes.SPANNER_TEST_DBis provided in the environment, parallel worker invocations collide on the same database name unless each worker appends its unique app prefix.Solution
django_test_suite_5.2.shanddjango_test_suite_6.0.shto create a single shared Spanner instance per test run and run the test apps in parallel background jobs (MAX_PARALLEL=4).run_testing_worker.pyand the test suite shell scripts to generate a unique database name per app and per run (dj_<timestamp>_<random4>_<app_prefix>, or${SPANNER_TEST_DB}_${APP_PREFIX}whenSPANNER_TEST_DBis set) on the shared instance so parallel workers isolate their schema DDL and data cleanly.