lidavidm commented on code in PR #42158:
URL: https://github.com/apache/arrow/pull/42158#discussion_r1642055118
##########
java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/FlightServerTestExtension.java:
##########
Review Comment:
Cool! Thank you for porting this!
##########
java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcIntervalVectorAccessorTest.java:
##########
Review Comment:
That's fine, thank you!
##########
java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/numeric/ArrowFlightJdbcFloat4VectorAccessorTest.java:
##########
@@ -193,9 +189,14 @@ public void
testShouldGetBigDecimalMethodFromFloat4Vector() throws Exception {
(accessor, currentRow) -> {
float value = accessor.getFloat();
if (Float.isInfinite(value) || Float.isNaN(value)) {
- exceptionCollector.expect(SQLException.class);
+ assertThrows(
+ SQLException.class,
+ () -> {
+ throw new SQLException();
+ });
+ } else {
+ assertThat(accessor.getBigDecimal(),
is(BigDecimal.valueOf(value)));
Review Comment:
I think it should be
```
if (isInfinite || isNaN) {
assertThrows(SQLException.class, accessor::getBigDecimal);
} else {
assertThat(accessor.getBigDecimal(), is(BigDecimal.valueOf(value));
}
```
##########
java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/numeric/ArrowFlightJdbcFloat4VectorAccessorTest.java:
##########
@@ -193,9 +189,14 @@ public void
testShouldGetBigDecimalMethodFromFloat4Vector() throws Exception {
(accessor, currentRow) -> {
float value = accessor.getFloat();
if (Float.isInfinite(value) || Float.isNaN(value)) {
- exceptionCollector.expect(SQLException.class);
+ assertThrows(
+ SQLException.class,
+ () -> {
+ throw new SQLException();
+ });
+ } else {
+ assertThat(accessor.getBigDecimal(),
is(BigDecimal.valueOf(value)));
Review Comment:
I'm not sure this is right...
##########
java/flight/flight-core/src/test/java/org/apache/arrow/flight/auth/TestBasicAuth.java:
##########
@@ -60,7 +62,7 @@ public class TestBasicAuth {
@Test
public void validAuth() {
client.authenticateBasic(USERNAME, PASSWORD);
-
Assertions.assertTrue(ImmutableList.copyOf(client.listFlights(Criteria.ALL)).size()
== 0);
+ assertTrue(ImmutableList.copyOf(client.listFlights(Criteria.ALL)).size()
== 0);
Review Comment:
nit: assertEquals?
##########
java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/ResultSetTestUtils.java:
##########
@@ -68,33 +57,39 @@ public static <T> void testData(
* @param columnNames the column names to fetch in the {@code ResultSet} for
comparison.
* @param expectedResults the rows and columns representing the only values
the {@code resultSet}
* is expected to have.
- * @param collector the {@link ErrorCollector} to use for asserting that the
{@code resultSet} has
- * the expected values.
* @param <T> the type to be found in the expected results for the {@code
resultSet}.
* @throws SQLException if querying the {@code ResultSet} fails at some
point unexpectedly.
*/
@SuppressWarnings("unchecked")
public static <T> void testData(
final ResultSet resultSet,
final List<String> columnNames,
- final List<List<T>> expectedResults,
- final ErrorCollector collector)
+ final List<List<T>> expectedResults)
throws SQLException {
testData(
resultSet,
data -> {
+ List<Throwable> errors = new ArrayList<>();
final List<T> columns = new ArrayList<>();
for (final String columnName : columnNames) {
try {
columns.add((T) resultSet.getObject(columnName));
} catch (final SQLException e) {
- collector.addError(e);
+ errors.add(e);
}
}
+ assertAll(
+ "Errors occurred while fetching data from the ResultSet",
+ errors.stream()
+ .map(
+ error ->
+ () -> {
+ throw error;
+ }));
+
Review Comment:
Why would we re-throw the error? I think that would short-circuit the test
unexpectedly.
That said, the version of this with an error collector is never actually
used. I would rather remove the redundant overloads and let any exceptions here
bubble up and terminate the test, instead of try to preserve the original
semantics which aren't used anyways.
--
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]
