fix(spanner): preserve user call options in execute/read and consolidate test databases by cy-yun · Pull Request #9329 · googleapis/google-cloud-php

cy-yun · 2026-07-08T04:17:00Z

Fix Spanner System Tests Reliability and Provisioning Overhead

This PR addresses the ongoing flakiness and high database provisioning overhead of the Spanner system tests (resolving #9230).

While this PR consolidates test database provisioning (reducing overall execution time by reusing databases), the test suite as a whole will still take a long time to run (~1.5 hours) due to the inherent slowness of operations like Backup and Restore. (Parallelizing these tests with paratest to bring the time down to minutes is planned for a subsequent follow-up).

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

1. Dropped `$options` Array (Fixing `ReadTest` flakiness & SDK Bug)

Problem: An issue was identified where the user-provided $options array was inadvertently being omitted when making calls through Database::execute() and TransactionalReadTrait methods. In the test suite, this surfaced as flakiness in testReadFailsOnDeadlineExceeded (because the 1ms timeoutMillis option was not passed through, allowing the request to reach the backend). For users, this meant that custom RPC settings (like requestTimeout, headers, or retrySettings) passed to queries or reads were not being applied to the underlying gRPC requests.
Fix:

Plucked timeoutMillis and requestOptions explicitly from $options in Database::execute, Database::read, TransactionalReadTrait::execute, and TransactionalReadTrait::read.
This ensures that timeout parameters cascade properly down to the GAX layer to satisfy deadline exceeded tests, without forwarding the entire $options array (which contains internal tracking state that would otherwise cause "Unexpected option" fatal errors in OptionsValidator).

2. Test Database Provisioning Overhead

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.
Fix: Introduced TestDatabaseManager to provision a single shared Spanner test database and reuse it across all test classes.

3. PostgreSQL DDL Compatibility

Problem: Reusing the same database across multiple test runs or classes caused CREATE TABLE commands to fail because the tables already existed.
Fix: Added IF NOT EXISTS to PostgreSQL DDL statements in the Pg*Test classes.

4. 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. Specifically, testLockHintReadWriteTransaction was leaving a transaction hanging without rollback.
Fix: Added a manual $res->transaction()->rollback() to testLockHintReadWriteTransaction, and added an unhandled exception catch block in Database::runTransaction to rollback non-single-use active transactions before bubbling up the exception.

5. `BackupTest` Reliability

Problem: BackupTest operations were flaking due to transient errors and improper polling configurations (e.g. timeoutMillis instead of maxPollingDurationSeconds for GAX long-running operations). The backup cancellation test was also flawed because it tried to wait for a cancelled backup to exist.
Fix: Updated long-running operation polling to use maxPollingDurationSeconds. Added retry logic for UNAVAILABLE errors in testRestoreDatabase. Split the backup creation and cancellation tests to prevent the cancellation from interfering with tests that depend on a successfully created backup.

6. PHP 8.1 Deprecation Warnings

Problem: Passing null to non-nullable internal string parameters threw deprecation warnings in PHP 8.1 (e.g. in ValueMapper). Additionally, the method key was sometimes missing when resuming operations in admin clients.
Fix: Cast nullable array values to string in ValueMapper and added isset checks for method in LongRunningClientConnection and *AdminClient.
gapic-generator-php fix: fix: Avoid passing null as an array offset for longRunning descriptors in resumeOperation gapic-generator-php#842

7. 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.

Hectorhammett · 2026-07-08T21:36:41Z

        // Poll for completion with the extended timeout
-        $op->pollUntilComplete([
+        $op->pollUntilComplete(
+            [


Did the linter suggested this change? This change feels wrong to me. I mean it still is valid, but I would expect either the previous iteration or something like:

$op->pollUntilComplete( [ 'timeoutMillis' => ... ] )

As it is right now, having the braces on the same level as the key feels off.

But I still think the previous one should be valid 🤷

I fixed the formatting of the code such that it is consistent with the original & phpcbf is not upset

Hectorhammett · 2026-07-08T21:40:35Z


-        $this->assertTrue($backup->exists());
+        // Cancellation usually drops the backup. We don't assert exists()
+        // to avoid flakiness with asynchronous deletion.


Wait, isn't this testing that if you execute a backup deletion and cancel it, it shouldn't be still available? Or am i misreading the test?

Actually, this test is meant to test canceling a backup creation operation, not a deletion. Previously, the test was calling $op->pollUntilComplete() before $op->cancel(). That meant it was waiting for the backup to fully finish creating, and then issuing a cancel command on a completed operation (which basically does nothing), hence why it used to assert that the backup still existed.

By removing the polling, we are now properly canceling the operation while it is still in-progress. Because we cancelled it mid-flight, Spanner usually drops the incomplete backup, so asserting that it still exists makes the test extremely flaky.

bshaffer

I know this is still in Draft mode but a few things jumped out at me so I wanted to go ahead and comment on them

bshaffer · 2026-07-22T22:50:05Z

+        if (isset($args['method'])) {
+            $operationResponse = $this->gapicClient->resumeOperation($args['name'], $args['method']);
+        } else {
+            $operationResponse = $this->gapicClient->resumeOperation($args['name']);
+        }


these can all be simplied

Suggested change

if (isset($args['method'])) {

$operationResponse = $this->gapicClient->resumeOperation($args['name'], $args['method']);

} else {

$operationResponse = $this->gapicClient->resumeOperation($args['name']);

}

$operationResponse = $this->gapicClient->resumeOperation($args['name'], $args['method'] ?? null);

bshaffer · 2026-07-22T22:52:30Z

    public function resumeOperation($operationName, $methodName = null)
    {
-        $options = $this->descriptors[$methodName]['longRunning'] ?? [];
+        $options = isset($methodName) ? ($this->descriptors[$methodName]['longRunning'] ?? []) : [];


Hmm, I do not see a functional difference between these two lines which were changed. I think this needs to be reverted.

It's also a generated file so it shouldn't be part of this PR anyway, but would need to be a change in the GAPIC generator

bshaffer · 2026-07-22T22:52:57Z

    public function resumeOperation($operationName, $methodName = null)
    {
-        $options = $this->descriptors[$methodName]['longRunning'] ?? [];
+        $options = isset($methodName) ? ($this->descriptors[$methodName]['longRunning'] ?? []) : [];


Same here, I think this change should be reverted

bshaffer · 2026-07-22T22:53:56Z

+                    } catch (\Throwable $rollbackException) {
+                        // ignore rollback failure and bubble up the original exception
+                    }
+                }


this is great!

cy-yun force-pushed the fix-spanner-tests-9230 branch 2 times, most recently from d69b03a to 169514a Compare July 8, 2026 19:41

cy-yun changed the title ~~Fix spanner tests 9230~~ test(spanner): optimize and fix system tests and enable in CI Jul 8, 2026

cy-yun mentioned this pull request Jul 8, 2026

fix(Spanner): prevent unnecessary GCE metadata probing during client … #9327

Merged

cy-yun force-pushed the fix-spanner-tests-9230 branch 2 times, most recently from bfd11cb to 41272ef Compare July 8, 2026 20:18

cy-yun marked this pull request as ready for review July 8, 2026 20:34

cy-yun requested a review from a team as a code owner July 8, 2026 20:34

Hectorhammett reviewed Jul 8, 2026

View reviewed changes

cy-yun force-pushed the fix-spanner-tests-9230 branch from 41272ef to 2e68fb4 Compare July 9, 2026 07:57

product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Jul 9, 2026

cy-yun force-pushed the fix-spanner-tests-9230 branch from 2e68fb4 to 921a05c Compare July 9, 2026 20:33

cy-yun added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 14, 2026

cy-yun added 4 commits July 20, 2026 18:37

fix(Core,Spanner): resolve PHP 8.1 deprecation warnings

22560dc

test(spanner): add IF NOT EXISTS to PostgreSQL DDL statements

1a41e94

fix(spanner): prevent leaked emulator transactions

e53ebe5

test(spanner): BackupTest reliability and optimization

9d572ec

cy-yun force-pushed the fix-spanner-tests-9230 branch from 921a05c to 122401c Compare July 21, 2026 01:43

cy-yun changed the title ~~test(spanner): optimize and fix system tests and enable in CI~~ fix(spanner): preserve user call options in execute/read and consolidate test databases Jul 21, 2026

cy-yun force-pushed the fix-spanner-tests-9230 branch 4 times, most recently from 5798d9e to 1de72fc Compare July 21, 2026 17:47

cy-yun added 2 commits July 21, 2026 12:52

test(spanner): consolidate test database provisioning

a6759ec

chore: enable Spanner system tests in CI

7f8dbc4

cy-yun force-pushed the fix-spanner-tests-9230 branch from 1de72fc to 06d7dd9 Compare July 21, 2026 19:57

cy-yun marked this pull request as draft July 21, 2026 20:12

cy-yun force-pushed the fix-spanner-tests-9230 branch from 06d7dd9 to b81c04e Compare July 21, 2026 20:21

fix(spanner): pluck call options like timeoutMillis and lockHint

cd9613c

cy-yun force-pushed the fix-spanner-tests-9230 branch 2 times, most recently from 92f159b to cd9613c Compare July 21, 2026 21:08

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

2838cc6

cy-yun force-pushed the fix-spanner-tests-9230 branch from b36da9c to 2838cc6 Compare July 21, 2026 22:49

Merge branch 'main' into fix-spanner-tests-9230

ab3fb29

bshaffer reviewed Jul 22, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(spanner): preserve user call options in execute/read and consolidate test databases#9329

fix(spanner): preserve user call options in execute/read and consolidate test databases#9329
cy-yun wants to merge 9 commits into
googleapis:mainfrom
cy-yun:fix-spanner-tests-9230

cy-yun commented Jul 8, 2026 •

edited

Loading

Uh oh!

Hectorhammett Jul 8, 2026

Uh oh!

Hectorhammett Jul 8, 2026

Uh oh!

cy-yun Jul 9, 2026

Uh oh!

Hectorhammett Jul 8, 2026

Uh oh!

cy-yun Jul 9, 2026

Uh oh!

bshaffer left a comment

Uh oh!

bshaffer Jul 22, 2026

Uh oh!

bshaffer Jul 22, 2026

Uh oh!

bshaffer Jul 22, 2026

Uh oh!

bshaffer Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

cy-yun commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix Spanner System Tests Reliability and Provisioning Overhead

1. Dropped $options Array (Fixing ReadTest flakiness & SDK Bug)

2. Test Database Provisioning Overhead

3. PostgreSQL DDL Compatibility

4. Emulator Transaction Leaks

5. BackupTest Reliability

6. PHP 8.1 Deprecation Warnings

7. CI Configuration

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bshaffer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cy-yun commented Jul 8, 2026 •

edited

Loading

1. Dropped `$options` Array (Fixing `ReadTest` flakiness & SDK Bug)

5. `BackupTest` Reliability