chore(bq jdbc): Add integration test suites#12102
Conversation
logachev
changed the title
logachev
force-pushed
the
kirl/integration-test-suites
branch
from
e397b0c to
4922142
Compare
logachev
force-pushed
the
kirl/integration-test-suites
branch
from
4922142 to
669153d
Compare
|
/gemini review |
![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 test suites to logically group integration tests and adds a new pom-it.xml for building a standalone test JAR, which are excellent improvements for test organization and CI/CD. The refactoring of authentication tests into a dedicated ITAuthTests class also enhances code structure. However, I've identified several critical issues with incorrect dependency versions in the pom.xml and pom-it.xml files that will cause the build to fail. Additionally, I've provided some suggestions to improve the quality and maintainability of the test code.
| <dependency> | ||
| <groupId>com.google.cloud</groupId> | ||
| <artifactId>google-cloud-bigquery-jdbc</artifactId> | ||
| <version>0.4.1-SNAPSHOT</version> | ||
| <type>test-jar</type> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
|
|
||
| <!-- Console launcher to run tests from command line without maven --> | ||
| <dependency> | ||
| <groupId>org.junit.platform</groupId> | ||
| <artifactId>junit-platform-console-standalone</artifactId> | ||
| <version>1.11.4</version> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
|
|
||
| <!-- Test dependencies are usually test scope, we make them compile here so they get shaded into the jar --> | ||
| <dependency> | ||
| <groupId>com.google.truth</groupId> | ||
| <artifactId>truth</artifactId> | ||
| <version>1.4.4</version> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.junit.jupiter</groupId> | ||
| <artifactId>junit-jupiter-api</artifactId> | ||
| <version>5.11.4</version> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.junit.jupiter</groupId> | ||
| <artifactId>junit-jupiter-engine</artifactId> | ||
| <version>5.11.4</version> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.junit.jupiter</groupId> | ||
| <artifactId>junit-jupiter-params</artifactId> | ||
| <version>5.11.4</version> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.mockito</groupId> | ||
| <artifactId>mockito-core</artifactId> | ||
| <version>4.11.0</version> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.mockito</groupId> | ||
| <artifactId>mockito-junit-jupiter</artifactId> | ||
| <version>4.11.0</version> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.junit.platform</groupId> | ||
| <artifactId>junit-platform-suite-api</artifactId> | ||
| <version>1.11.4</version> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.junit.platform</groupId> | ||
| <artifactId>junit-platform-suite-engine</artifactId> | ||
| <version>1.11.4</version> | ||
| <scope>compile</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Several dependency versions in this file are incorrect and do not exist in Maven Central, which will cause the build to fail. Specifically:
google-cloud-bigquery-jdbc: The version is hardcoded to0.4.1-SNAPSHOT, which is brittle. It's better to use a property that can be updated easily.junit-platform-console-standalone: Version1.11.4is incorrect. The latest is1.10.2.junit-jupiter-*artifacts: Version5.11.4is incorrect. The latest is5.10.2.junit-platform-suite-*artifacts: Version1.11.4is incorrect. The latest is1.10.2.
I recommend defining these versions as properties for consistency and easier maintenance.
| <dependency> | ||
| <groupId>org.apache.maven.surefire</groupId> | ||
| <artifactId>surefire-junit-platform</artifactId> | ||
| <version>3.5.2</version> |
There was a problem hiding this comment.
The version 3.5.2 for surefire-junit-platform is incorrect and does not exist in Maven Central. The latest version for maven-surefire-plugin artifacts is 3.2.5. Using a non-existent version will break the build.
| <version>3.5.2</version> | |
| <version>3.2.5</version> |
| <dependency> | ||
| <groupId>org.junit.platform</groupId> | ||
| <artifactId>junit-platform-suite-api</artifactId> | ||
| <version>1.11.4</version> |
There was a problem hiding this comment.
The version 1.11.4 for junit-platform-suite-api is incorrect as it doesn't exist in Maven Central. The latest version is 1.10.2. This will cause the build to fail.
| <version>1.11.4</version> | |
| <version>1.10.2</version> |
| + "ProjectId=" | ||
| + authJson.get("project_id").getAsString() | ||
| + ";OAuthType=0;" | ||
| + ";OAuthServiceAcctEmail=;" |
There was a problem hiding this comment.
The connection string is being built with an extra leading semicolon. While this might be handled gracefully by the driver, it makes the code confusing and should be removed for clarity and correctness.
This same issue is present in other tests within this file, for example on lines 131, 166, and 316.
| + ";OAuthServiceAcctEmail=;" | |
| + "OAuthServiceAcctEmail=;" |
| try { | ||
| DriverManager.getConnection(connection_uri); | ||
| Assertions.fail(); | ||
| } catch (Exception ex) { | ||
| assertTrue(ex.getMessage() != null); | ||
| } |
There was a problem hiding this comment.
This test now catches a generic Exception and performs a weak assertion on the message. This is a regression from the original test, which caught a more specific exception and checked for a particular error message. To make the test more robust and specific, it should catch SQLException and assert the content of the exception message.
| try { | |
| DriverManager.getConnection(connection_uri); | |
| Assertions.fail(); | |
| } catch (Exception ex) { | |
| assertTrue(ex.getMessage() != null); | |
| } | |
| try { | |
| DriverManager.getConnection(connection_uri); | |
| Assertions.fail("Expected an exception to be thrown."); | |
| } catch (SQLException ex) { | |
| assertTrue(ex.getMessage().contains("No valid credentials provided.")); | |
| } |
| protected static String requireEnvVar(String varName) { | ||
| String value = System.getenv(varName); | ||
| assertNotNull( | ||
| System.getenv(varName), | ||
| "Environment variable " + varName + " is required to perform these tests."); | ||
| return value; | ||
| } |
There was a problem hiding this comment.
The requireEnvVar method calls System.getenv(varName) twice. It's more efficient and cleaner to call it once, store the result in a local variable, and use that variable for both the assertion and the return value.
| protected static String requireEnvVar(String varName) { | |
| String value = System.getenv(varName); | |
| assertNotNull( | |
| System.getenv(varName), | |
| "Environment variable " + varName + " is required to perform these tests."); | |
| return value; | |
| } | |
| protected static String requireEnvVar(String varName) { | |
| String value = System.getenv(varName); | |
| assertNotNull( | |
| value, | |
| "Environment variable " + varName + " is required to perform these tests."); | |
| return value; | |
| } |
2 participants
Introduction of Suites to logically group tests
This will allow us to split enormous integration test files into scenario/feature focused test files & include them in relevant suites (tbd: can we mark some tests as "Long-running" & automatically exclude from presubmit, but include in nightly).
Starting with 3 suites: presubmit/nightly & Driver Agnostic tests.
Continuous integration for released JARs
New
pom-it.xmlfile to build test jar. It will allow running tests with standalone uber jar, e.g. as a validation step during release & continuous tests using previously released versions, but leveraging new tests.I moved auth tests primarily for validation.