vlsi commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r593039868
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
Review comment:
> I do not see any value in it, since these asserts check not for the
outcome of the DDL operation per se, but whether JDBC command returned a
ResultSet or not
Frankly speaking, that is not really obvious from the code, even though I
admit `statement.execute` is the very basic JDBC API.
I would suggest:
1) If the outcome of `execute` is not needed, then skip the assert. Just
ignore the returned value.
2) If you want to assert something, use `reason` method or add a helper
method like `assertExecutesWithoutResutlSet(s, "create schema if not exists
s")`. Then the test code would be shorter, it would be easier to understand,
and the failure messages could be way more readable. For instance, the helper
method could throw errors like "statement produced resultset with columns ...
while no resultset was expected".
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
