Copilot commented on code in PR #7148:
URL: https://github.com/apache/ignite-3/pull/7148#discussion_r2598504751
##########
modules/jdbc/src/test/java/org/apache/ignite/internal/jdbc/JdbcConnectionSelfTest.java:
##########
@@ -444,18 +452,153 @@ public void wrap() throws SQLException {
}
}
+ @Test
+ void testQueryTimeoutProperty() throws SQLException {
+ String propName = "queryTimeoutSeconds";
+ String urlPrefix = URL + "?" + propName;
+
+ SqlThrowingFunction<String, Number> valueGetter = url -> {
+ try (Connection conn = createConnection(url)) {
+ try (Statement stmt = conn.createStatement();
+ PreparedStatement pstmt =
conn.prepareStatement("SELECT 1")) {
+
+ assertThat(stmt.getQueryTimeout(),
Matchers.is(pstmt.getQueryTimeout()));
+
+ return stmt.getQueryTimeout();
+ }
+ }
+ };
+
+ assertThat(valueGetter.apply(URL), Matchers.is(0));
+ assertThat(valueGetter.apply(urlPrefix + "=2147483647"),
Matchers.is(Integer.MAX_VALUE));
+ assertThat(valueGetter.apply(urlPrefix + "=0"), Matchers.is(0));
+
+ expectConnectionException(urlPrefix + "=A",
+ format("Failed to parse int property [name={}, value=A]",
propName));
+
+ expectConnectionException(urlPrefix + "=-1",
+ format("Property cannot be lower than 0 [name={}, value=-1]",
propName));
+
+ expectConnectionException(urlPrefix + "=2147483648",
+ format("Failed to parse int property [name={},
value=2147483648]", propName));
+
+ // Check deprecated name "connectionTimeout"
+ try (Connection conn = createConnection(URL +
"?connectionTimeout=100")) {
+ assertThat(((JdbcConnection)
conn).properties().getConnectionTimeout(), Matchers.is(100));
+ }
+
+ // If both names are specified, the deprecated property name should be
ignored
+ try (Connection conn = createConnection(URL +
"?connectionTimeoutMillis=50&connectionTimeout=100")) {
+ assertThat(((JdbcConnection)
conn).properties().getConnectionTimeout(), Matchers.is(50));
Review Comment:
The test for deprecated property is incorrect. Lines 485-493 test the
deprecated `connectionTimeout` property, but this is the
`testQueryTimeoutProperty()` method which should test the deprecated
`queryTimeout` property instead.
These tests should be checking the deprecated `queryTimeout` property with
`getQueryTimeout()`, not `getConnectionTimeout()`. For example:
```java
// Check deprecated name "queryTimeout"
try (Connection conn = createConnection(URL + "?queryTimeout=100")) {
try (Statement stmt = conn.createStatement()) {
assertThat(stmt.getQueryTimeout(), Matchers.is(100));
}
}
// If both names are specified, the deprecated property name should be
ignored
try (Connection conn = createConnection(URL +
"?queryTimeoutSeconds=50&queryTimeout=100")) {
try (Statement stmt = conn.createStatement()) {
assertThat(stmt.getQueryTimeout(), Matchers.is(50));
}
}
```
```suggestion
// Check deprecated name "queryTimeout"
try (Connection conn = createConnection(URL + "?queryTimeout=100")) {
try (Statement stmt = conn.createStatement()) {
assertThat(stmt.getQueryTimeout(), Matchers.is(100));
}
}
// If both names are specified, the deprecated property name should
be ignored
try (Connection conn = createConnection(URL +
"?queryTimeoutSeconds=50&queryTimeout=100")) {
try (Statement stmt = conn.createStatement()) {
assertThat(stmt.getQueryTimeout(), Matchers.is(50));
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
