Skip to content

fix(Spanner): Make System Tests Work & Work Faster - #9401

Open
cy-yun wants to merge 6 commits into
mainfrom
fix-spanner-system-tests
Open

fix(Spanner): Make System Tests Work & Work Faster#9401
cy-yun wants to merge 6 commits into
mainfrom
fix-spanner-system-tests

Conversation

@cy-yun

@cy-yun cy-yun commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Screenshot 2026-07-23 at 1 07 38 PMScreenshot 2026-07-27 at 2 03 06 PM

This PR addresses the ongoing flakiness, high database provisioning overhead, and parallel-execution collisions of the Spanner system tests.

By consolidating test database provisioning, we reduce overall execution time through database reuse. Furthermore, we've removed structural bottlenecks (such as ID collisions and schema mismatches) that prevented these tests from running concurrently. (Parallelizing these tests with paratest to bring the time down to minutes locally is planned for a subsequent follow-up).

Below is a detailed breakdown of the specific problems and fixes implemented in this PR:

1. Test Database Provisioning Overhead & DDL Consolidation

  • Problem: The system test suite was creating and dropping a new test database for every single test class, adding massive overhead to the already long execution time. Furthermore, individual test classes independently executed DDL statements (CREATE TABLE), causing Table already exists collisions when tests were executed against a shared persistent database.
  • Fix:
    • Introduced TestDatabaseManager to provision a single shared Spanner test database and reuse it across all test classes.
    • Centralized all table and schema creation logic into unified traits: SystemTestCaseTrait.php (Standard Spanner) and PgSystemTestCaseTrait.php (Postgres Spanner).
    • Removed redundant CREATE TABLE and DROP TABLE operations from over a dozen individual test classes (e.g., BatchTest, WriteTest, PgBatchTest).

2. PostgreSQL DDL Compatibility & Schema Mismatches

  • Problem: Reusing the same database across multiple test runs or classes caused PostgreSQL CREATE TABLE commands to fail because the tables already existed. Additionally, there was schema drift between individual tests.
  • Fix:
    • Added IF NOT EXISTS to PostgreSQL DDL statements in the Pg*Test classes.
    • Reconciled schema drift and mismatches in PgQueryTest and PgPartitionedDmlTest to align exactly with the unified setup suite definitions.
    • Adjusted PgReadTest.php so that secondary index creation operates safely on shared databases without conflicting with concurrent tests.

3. Row ID Collisions on Persistent Databases

  • Problem: Because multiple tests (like TransactionTest, OperationsTest, BackupTest) share the same Users test table, tests running sequentially on a persistent database were slowly filling up the table. Tests generated random keys using narrow ranges like rand(1, 346464), leading to frequent ALREADY_EXISTS errors during insertion due to the Birthday Paradox.
  • Fix:
    • Updated the central SystemTestCase::randId() generator to use a massive 1-billion key space: rand(1, 999999999).
    • Replaced 11 instances of hardcoded, narrow rand() ranges in TransactionTest.php and PgTransactionTest.php to use the unified self::randId() helper.

4. BackupTest Reliability & Timeouts

  • Problem: BackupTest operations were flaking due to transient errors, DEADLINE_EXCEEDED errors when GCP was slow, and improper polling configurations.
  • Fix:
    • Updated long-running operation polling to use maxPollingDurationSeconds via an extended timeout loop for all Backup and Restore operations.
    • Refactored the timeout try/catch logic into a DRY pollWithExtendedTimeout($op) helper method in BackupTest.
    • Caught and ignored DEADLINE_EXCEEDED exceptions emitted during intermediate polling attempts.

5. Emulator Transaction Leaks

  • Problem: When running tests against the emulator, unhandled or failed transactions were leaking and persisting on the emulator, causing subsequent test failures or slowdowns.
  • Fix: Added an unhandled exception catch block in Database::runTransaction to rollback non-single-use active transactions before bubbling up the exception. Ensured ReadTest.php properly cleans up local emulator transaction state.

6. CI Configuration

  • Problem: Spanner system tests were not executing in the standard CI pipeline.
  • Fix: Enabled the Spanner test suite in phpunit-system.xml.dist now that stability is restored.

@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Jul 27, 2026
@cy-yun cy-yun changed the title fix(Spanner): Make system tests work fix(Spanner): Make System Tests Work & Work Faster Jul 27, 2026
@cy-yun
cy-yun force-pushed the fix-spanner-system-tests branch 2 times, most recently from a0bfe04 to 6f74d5e Compare July 27, 2026 21:26
@cy-yun
cy-yun marked this pull request as ready for review July 28, 2026 18:23
@cy-yun
cy-yun requested a review from a team as a code owner July 28, 2026 18:23
@cy-yun
cy-yun force-pushed the fix-spanner-system-tests branch from b6fc922 to 03cee14 Compare July 28, 2026 21:26
Comment thread Core/src/LongRunning/LongRunningClientConnection.php Outdated
Comment thread Core/src/LongRunning/LongRunningClientConnection.php Outdated
Comment thread Core/src/LongRunning/LongRunningClientConnection.php Outdated
Comment thread Gax/tests/Unit/SerializerTest.php Outdated
$this->assertEquals(
$serializer->encodeMessage($message),
$serializer->encodeMessage($deserializedMessage)
);

@bshaffer bshaffer Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we have here is testing something different, so I don't like the change. I also don't understand why the change was made (because I do not ever recall seeing issues with this test). Could you explain it?

We do have a specific method assertProtobufEquals in Testing\GeneratedTest, so if there's a problem here, it may be better to use that (although that also calls assertEquals under the hood)

Comment thread Spanner/src/Admin/Database/V1/Client/DatabaseAdminClient.php Outdated
public function resumeOperation($operationName, $methodName = null)
{
$options = $this->descriptors[$methodName]['longRunning'] ?? [];
$options = isset($methodName) ? ($this->descriptors[$methodName]['longRunning'] ?? []) : [];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are generated classes and shouldnt be updated manually

Comment thread Spanner/src/Database.php
// ignore rollback failure and bubble up the original exception
}
}
throw $e;

@bshaffer bshaffer Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really important, but this seems like a separate fix or a feature, as it's ensuring an active transaction which fails is rolled back appropriately. I think it should be in a separate PR (with tests)

@cy-yun
cy-yun force-pushed the fix-spanner-system-tests branch 7 times, most recently from 520dae2 to 40ca056 Compare July 28, 2026 23:46
cy-yun added 5 commits July 28, 2026 16:47
test(spanner): consolidate test database provisioning

fix(spanner): fix PgReadTest index creation on shared database

test(spanner): Consolidate DDL operations into test setup suite

test(spanner): fix schema mismatches for partitionedDml and PgQueryTest

test(spanner): rename PgQueryTest_2 back to PgQueryTest

test(spanner): fix PostgreSQL column name mismatches for test tables
test(spanner): Handle DEADLINE_EXCEEDED in BackupTest

test(spanner): fix BackupTest timeout and PgQueryTest schema collision

test(spanner): add deadline exceeded polling to testCreateBackup2

refactor(spanner): DRY up extended polling loop in BackupTest
…ger range

test(spanner): restore seedTable in BatchTest to fix test coverage

test(spanner): fix array_rand bug and batch mutations in seedTable

test(spanner): fix fundamentally broken partitionRead test logic in BatchTest

fix: move seedTable back to its original location

fix(cs): fix style issues in BatchTest.php

fix(cs): format multi-line args
@cy-yun
cy-yun force-pushed the fix-spanner-system-tests branch from 40ca056 to 213aec2 Compare July 28, 2026 23:47
…tent databases, and apply code review fixes for tests
@cy-yun
cy-yun force-pushed the fix-spanner-system-tests branch from 213aec2 to bed320e Compare July 29, 2026 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: spanner Issues related to the Spanner API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants