Skip to content

Write acceptance tests#25

Open
stevelordbq wants to merge 5 commits intomainfrom
acceptance-write
Open

Write acceptance tests#25
stevelordbq wants to merge 5 commits intomainfrom
acceptance-write

Conversation

@stevelordbq
Copy link
Copy Markdown

No description provided.

@stevelordbq
Copy link
Copy Markdown
Author

/gemini review

@stevelordbq stevelordbq requested a review from MaxKsyunz March 9, 2026 18:43
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 adds acceptance tests for write operations. The changes look good overall, but I have a few suggestions to improve code quality and test correctness.
In the Java test file, I've suggested using Google Truth's assertThat for consistency with existing assertions.
In the Python test script, I've pointed out several unused imports that should be removed. More importantly, I've proposed a refactoring of the data verification logic to make it more efficient and to provide clearer, more accurate results in case of a data mismatch. The current logic is inefficient and can produce confusing failure messages.

@stevelordbq
Copy link
Copy Markdown
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 adds acceptance tests for the write functionality using a new Python Spark script. It also refactors the DataprocServerlessAcceptanceTestBase to use a one-time setup and teardown, which is a good improvement for test performance.

I've found a critical issue in the refactored setup logic where the initialization flag was not being set, causing the setup to run for every test. I've also suggested a minor improvement for assertion consistency. Please see the detailed comments.

Comment on lines +118 to +130
if (!initialized) {
try {
context =
new AcceptanceTestContext(
testId, generateClusterName(testId), testBaseGcsDir, connectorJarUri);
AcceptanceTestUtils.uploadConnectorJar(
connectorJarDirectory, connectorJarPrefix, context.connectorJarUri);
createSpannerDataset();
} catch (Exception e) {
initializationException = e;
throw e;
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

The one-time initialization logic has a bug: the initialized flag is never set to true. This causes the expensive setup to run before every test, defeating the purpose of the refactoring. Additionally, the check is not thread-safe, which could lead to race conditions if tests are run in parallel. I suggest wrapping the initialization logic in a synchronized block and setting the flag upon successful completion.

    synchronized (DataprocServerlessAcceptanceTestBase.class) {
      if (!initialized) {
        try {
          context =
              new AcceptanceTestContext(
                  testId, generateClusterName(testId), testBaseGcsDir, connectorJarUri);
          AcceptanceTestUtils.uploadConnectorJar(
              connectorJarDirectory, connectorJarPrefix, context.connectorJarUri);
          createSpannerDataset();
          initialized = true;
        } catch (Exception e) {
          initializationException = e;
          throw e;
        }
      }
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant