Skip to content

test(spanner): add integration tests for Batch DML#5723

Open
olavloite wants to merge 8 commits into
googleapis:mainfrom
olavloite:spanner-batch-dml-integration-tests
Open

test(spanner): add integration tests for Batch DML#5723
olavloite wants to merge 8 commits into
googleapis:mainfrom
olavloite:spanner-batch-dml-integration-tests

Conversation

@olavloite
Copy link
Copy Markdown
Contributor

  • Adds integration tests for executing Batch DML on Spanner.
  • Fixes an issue in the BeginTransaction handling for Batch DML on the Emulator, as the Emulator can return Aborted even for BeginTransaction (this happens when there already is an active transaction on the Emulator)
  • Fixes an issue where the transaction ID would not be extracted from the ExecuteBatchDmlResponse if the response returned a non-zero status code. This indicates that an error occurred for one of the statements in the batch (but not for the first statement).

@olavloite olavloite requested review from a team as code owners May 22, 2026 16:28
@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label May 22, 2026
@olavloite olavloite requested a review from sakthivelmanii May 22, 2026 16:28
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request implements robust error handling for Batch DML operations in Spanner, specifically addressing cases where service-level errors are returned within successful gRPC responses. It introduces a CheckServiceError trait to normalize these responses and updates the execute_with_retry! macro to handle transaction state resets correctly during failures. Additionally, the PR includes comprehensive integration tests for various Batch DML scenarios and refactors response processing for better readability. Feedback was provided regarding the use of unwrap() in production code, which violates the project's style guide.

Comment thread src/spanner/src/read_write_transaction.rs Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented May 22, 2026

Codecov Report

❌ Patch coverage is 99.04459% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.89%. Comparing base (b6b59db) to head (16ba8ef).

Files with missing lines Patch % Lines
src/spanner/src/read_write_transaction.rs 98.90% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5723      +/-   ##
==========================================
+ Coverage   97.88%   97.89%   +0.01%     
==========================================
  Files         226      226              
  Lines       55691    55985     +294     
==========================================
+ Hits        54512    54806     +294     
  Misses       1179     1179              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

olavloite added 3 commits May 22, 2026 18:44
Removes the `unwrap()` call in `execute_with_retry` and also refactors the
method to use a guard to ensure the transaction state remains valid.
@olavloite
Copy link
Copy Markdown
Contributor Author

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request enhances the Spanner client's handling of Batch DML and lazy transaction starts. It introduces a LazyTransactionStartGuard to manage state transitions during inline begins and a CheckServiceError trait to identify service-level errors within successful RPC responses. The execute_with_retry! macro was refactored to utilize these components, ensuring more reliable transaction recovery and retries, particularly for aborted errors. Comprehensive integration tests for various Batch DML failure and continuation scenarios were also added. Reviewer feedback suggests further simplifying the macro logic by adopting more idiomatic Rust patterns, such as using Option combinators and the ? operator to handle result propagation and error extraction more concisely.

Comment thread src/spanner/src/read_write_transaction.rs Outdated
Comment thread src/spanner/src/read_write_transaction.rs Outdated
Comment thread src/spanner/src/read_write_transaction.rs Outdated
Comment thread src/spanner/src/read_write_transaction.rs Outdated
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.

1 participant