chore: add new integration tests to ensure topology query doesn't change #910
Conversation
wrapper/src/main/java/software/amazon/jdbc/dialect/AuroraMysqlDialect.java
Outdated
Show resolved
Hide resolved
wrapper/src/main/java/software/amazon/jdbc/dialect/AuroraMysqlDialect.java
Outdated
Show resolved
Hide resolved
wrapper/src/main/java/software/amazon/jdbc/dialect/AuroraMysqlDialect.java
Outdated
Show resolved
Hide resolved
wrapper/src/main/java/software/amazon/jdbc/dialect/AuroraPgDialect.java
Outdated
Show resolved
Hide resolved
wrapper/src/main/java/software/amazon/jdbc/dialect/RdsMultiAzDbClusterMysqlDialect.java
Outdated
Show resolved
Hide resolved
wrapper/src/main/java/software/amazon/jdbc/dialect/RdsMultiAzDbClusterPgDialect.java
Outdated
Show resolved
Hide resolved
| LOGGER.finest("Connecting to " + url); | ||
|
|
||
| String query = null; | ||
| if (TestEnvironment.getCurrent().getCurrentDriver() == TestDriver.PG) { |
There was a problem hiding this comment.
What's about MariaDb? Is it supported by the test?
There was a problem hiding this comment.
I mean I don't see a branch of the code that handle the case with mariaDb. I see AuroraMysql and AuroraPG.
wrapper/src/test/java/integration/container/tests/TopologyQueryTests.java
Outdated
Show resolved
Hide resolved
wrapper/src/test/java/integration/container/tests/TopologyQueryTests.java
Show resolved
Hide resolved
jasonli-improving
force-pushed
the
topology-query-test
branch
from
434087b to
2681ddf
Compare
| final Connection conn = DriverManager.getConnection(url, props); | ||
| assertTrue(conn.isValid(5)); | ||
| Statement stmt = conn.createStatement(); | ||
| stmt.executeQuery(query); |
There was a problem hiding this comment.
Can you use try with resources here? https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
There was a problem hiding this comment.
Same comment for the other locations.
| DriverHelper.setConnectTimeout(testDriver, props, 10, TimeUnit.SECONDS); | ||
| DriverHelper.setSocketTimeout(testDriver, props, 10, TimeUnit.SECONDS); | ||
|
|
||
| String url = |
There was a problem hiding this comment.
Can you please add the final keyword to all your constant local variables?
Also you can just call ConnectionStringHelper.getWrapperUrl() here.
| if (TestEnvironment.getCurrent().getCurrentDriver() == TestDriver.MARIADB | ||
| && TestEnvironment.getCurrent().getInfo().getRequest().getDatabaseEngine() == DatabaseEngine.MYSQL) { | ||
| props.setProperty("permitMysqlScheme", "1"); | ||
| } |
There was a problem hiding this comment.
getWrapperUrl should already be appending permitMysqlScheme to the wrapper url, why is this if-clause required?
| LOGGER.finest("Connecting to " + url); | ||
|
|
||
| if (TestEnvironment.getCurrent().getCurrentDriver() == TestDriver.PG) { | ||
| AuroraPgDialect dialect = new AuroraPgDialect(); |
There was a problem hiding this comment.
I think you could move this if-else to the beginning of the test after List<String> expectedTypes;, before establishing a connection, since you don't need any connections here and you are just setting up for the test.
Summary
Add integration tests to check for timestamp and types in the return values of topology queries
Description
Additional Reviewers
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.