Move JDBC warnings to Statement #1640
Conversation
presto-jdbc/src/test/java/io/prestosql/jdbc/TestJdbcWarnings.java
Outdated
Show resolved
Hide resolved
| assertEquals(currentWarnings.size(), 100); | ||
|
|
||
| Set<WarningEntry> currentWarnings = getStatementWarnings(statement, queryCreationFuture); | ||
| assertThat(currentWarnings).size().isGreaterThanOrEqualTo(PRELOADED_WARNINGS + 1); |
There was a problem hiding this comment.
assertThat(currentWarnings).size().isGreaterThan(PRELOADED_WARNINGS);
There was a problem hiding this comment.
Previously we were checking there were 100 warnings, not we're checking there are at least 6.
I don't know where this 100 came from, but in any case, it would be best to check actual messages of these warnings.
There was a problem hiding this comment.
The messages are not useful, as they are generated in TestingWarningCollector by rendering the warning code. However, the code and message are compared for uniqueness via the WarningEntry wrapper which is then inserted into a Set.
The previous test code was confusing and didn't really test what we wanted, which is to ensure that warnings show up as the query is running. I rewrote the code to directly test that.
presto-jdbc/src/test/java/io/prestosql/jdbc/TestJdbcWarnings.java
Outdated
Show resolved
Hide resolved
presto-jdbc/src/test/java/io/prestosql/jdbc/TestJdbcWarnings.java
Outdated
Show resolved
Hide resolved
electrum
force-pushed
the
jdbc-warnings
branch
from
2c809e3 to
607b42d
Compare
|
@findepi thanks for the review. I updated/rewrote the PR |
The warnings are not specific to the ResultSet.