GH-42045: [Java] Update Unit Tests for Flight Module #42158

llama90 · 2024-06-15T13:15:39Z

Rationale for this change

Update package from JUnit 4(org.junit) to JUnit 5(org.junit.jupiter).

What changes are included in this PR?

common for all module
- Step 1. Replacing org.junit with org.junit.jupiter.api
- Step 2. Updating Assertions.assertXXX to assertXXX using static imports
flight-core
- Updating common
- Updating annotations
- Doing self review
flight-integration-tests - nothing to change
flight-sql
- Updating common
- Doing self review
flight-sql-jdbc-core
- Updating common
- Updating annotations
  - Adding annotations different from before:
    - ErrorCollection
    - TestRule for FlightServerTestExtension and RootAllocatorExtension
    - ExpectedException
- Doing self review
flight-sql-jdbc-driver
- Updating common
- Updating annotations
- Doing self review

Are these changes tested?

Yes, existing tests have passed.

Are there any user-facing changes?

No.

GitHub Issue: [Java] Update Unit Tests for Flight Module #42045
GitHub Issue: [Java] Update Unit Tests for Flight Module #42045

- Step 1. update import statements - from `org.junit.Assert` to `org.junit.jupiter.api.Assertions` - Step 2. update annotations - `@After` -> `@AfterEach` - `@AfterClass` -> `@AfterAll` - `@Before` -> `@BeforeEach` - `@BeforeClass` -> `@BeforeAll` - `@Test` -> `@Test` with `org.junit.jupiter`

github-actions · 2024-06-15T13:16:05Z

⚠️ GitHub issue #42045 has been automatically assigned in GitHub to PR creator.

…lightServerTestExtension

…ootAllocatorTestExtension

...ight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/FlightServerTestExtension.java

...l-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/RootAllocatorTestExtension.java

...ache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcIntervalVectorAccessorTest.java

llama90 · 2024-06-16T12:33:06Z

...light-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/ResultSetTestUtils.java

+          assertAll(
+              "Errors occurred while fetching data from the ResultSet",
+              errors.stream()
+                  .map(
+                      error ->
+                          () -> {
+                            throw error;
+                          }));
+


ETC. Is this change appropriate?

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.

Fixed it.

arrow/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/ResultSetTestUtils.java

Lines 73 to 77 in 323c709

try {

columns.add((T) resultSet.getObject(columnName));

} catch (final SQLException e) {

throw new RuntimeException(e);

}

arrow/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/ResultSetTestUtils.java

Lines 103 to 107 in 323c709

try {

columns.add((T) resultSet.getObject(columnIndex));

} catch (final SQLException e) {

throw new RuntimeException(e);

}

llama90 · 2024-06-16T12:35:18Z

.../apache/arrow/driver/jdbc/accessor/impl/numeric/ArrowFlightJdbcFloat4VectorAccessorTest.java

+            assertThrows(
+                SQLException.class,
+                () -> {
+                  throw new SQLException();
+                });
+          } else {
+            assertThat(accessor.getBigDecimal(), is(BigDecimal.valueOf(value)));


ETC. Is this change appropriate?

I'm not sure this is right...

I think it should be

if (isInfinite || isNaN) { assertThrows(SQLException.class, accessor::getBigDecimal); } else { assertThat(accessor.getBigDecimal(), is(BigDecimal.valueOf(value)); }

Fixed it.

arrow/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/numeric/ArrowFlightJdbcFloat4VectorAccessorTest.java

Lines 191 to 195 in 323c709

if (Float.isInfinite(value) || Float.isNaN(value)) {

assertThrows(SQLException.class, accessor::getBigDecimal);

} else {

assertThat(accessor.getBigDecimal(), is(BigDecimal.valueOf(value)));

}

arrow/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/numeric/ArrowFlightJdbcFloat4VectorAccessorTest.java

Lines 205 to 211 in 323c709

if (Float.isInfinite(value) || Float.isNaN(value)) {

assertThrows(SQLException.class, () -> accessor.getBigDecimal(9));

} else {

assertThat(

accessor.getBigDecimal(9),

is(BigDecimal.valueOf(value).setScale(9, RoundingMode.HALF_UP)));

}

arrow/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/numeric/ArrowFlightJdbcFloat8VectorAccessorTest.java

Lines 144 to 148 in 323c709

if (Double.isInfinite(value) || Double.isNaN(value)) {

assertThrows(SQLException.class, accessor::getBigDecimal);

} else {

assertThat(accessor.getBigDecimal(), is(BigDecimal.valueOf(value)));

}

arrow/java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/numeric/ArrowFlightJdbcFloat8VectorAccessorTest.java

Lines 184 to 190 in 323c709

if (Double.isInfinite(value) || Double.isNaN(value)) {

assertThrows(SQLException.class, () -> accessor.getBigDecimal(9));

} else {

assertThat(

accessor.getBigDecimal(9),

is(BigDecimal.valueOf(value).setScale(9, RoundingMode.HALF_UP)));

}

llama90 · 2024-06-16T12:38:53Z

...light-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/ResultSetTestUtils.java

Point 3. Removing ErrorCollector.

I removed some methods to remove ErrorCollector. Is this an appropriate change?

llama90

@lidavidm @vibhatha

I have just completed this PR. Overall, it looks good to me, but there are some points I would like to review.

The PR contains many changes, so I have listed the review points below.

Point 1. from TestRule to Extension
Point 2. ParameterizedTest
Point 3. Removing ErrorCollector
ETC.

Thank you always for your review.

lidavidm · 2024-06-17T01:19:31Z

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);


nit: assertEquals?

Fixed it.

arrow/java/flight/flight-core/src/test/java/org/apache/arrow/flight/auth/TestBasicAuth.java

Line 64 in 323c709

assertEquals(0, ImmutableList.copyOf(client.listFlights(Criteria.ALL)).size());

...ight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/FlightServerTestExtension.java

...ache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcIntervalVectorAccessorTest.java

lidavidm · 2024-06-17T02:04:05Z

.../apache/arrow/driver/jdbc/accessor/impl/numeric/ArrowFlightJdbcFloat4VectorAccessorTest.java

+            assertThrows(
+                SQLException.class,
+                () -> {
+                  throw new SQLException();
+                });
+          } else {
+            assertThat(accessor.getBigDecimal(), is(BigDecimal.valueOf(value)));


I'm not sure this is right...

lidavidm · 2024-06-17T02:04:57Z

.../apache/arrow/driver/jdbc/accessor/impl/numeric/ArrowFlightJdbcFloat4VectorAccessorTest.java

+            assertThrows(
+                SQLException.class,
+                () -> {
+                  throw new SQLException();
+                });
+          } else {
+            assertThat(accessor.getBigDecimal(), is(BigDecimal.valueOf(value)));


I think it should be

if (isInfinite || isNaN) { assertThrows(SQLException.class, accessor::getBigDecimal); } else { assertThat(accessor.getBigDecimal(), is(BigDecimal.valueOf(value)); }

lidavidm · 2024-06-17T02:16:41Z

...light-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/ResultSetTestUtils.java

+          assertAll(
+              "Errors occurred while fetching data from the ResultSet",
+              errors.stream()
+                  .map(
+                      error ->
+                          () -> {
+                            throw error;
+                          }));
+


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.

llama90 · 2024-06-18T10:33:40Z

@lidavidm I have reflected your review. Thank you for your feedback!

vibhatha · 2024-06-18T13:24:53Z

...g/apache/arrow/driver/jdbc/accessor/impl/binary/ArrowFlightJdbcBinaryVectorAccessorTest.java

  }

-  @Before
-  public void setup() {
+  public void setup(Supplier<ValueVector> vectorSupplier) {


can't use @BeforeEach?

This logic is similar to initializeDatabase in the adapter module. Unfortunately, I haven't found a better way yet. 🥲 If I find a better solution in the future, I'll propose an improvement.

GH-42025: [Java] Update Unit Tests for Algorithm Module #42029 (comment)

GH-42030: [Java] Update Unit Tests for Adapter Module #42038 (comment)

No I think that's fine. Just wanted to make sure. I can recall the discussion we had.

By the way, should we create an issue to track this in case we get this feature from Junit?

That's a good idea. I know which methods need to be updated, so I created an issue for tracking.

https://github.com/apache/arrow/issues/42196

Thanks. Let's keep this as a future task, we don't need to update our TODO list as we don't know exactly when we can attempt this.

vibhatha · 2024-06-18T13:25:41Z

...g/apache/arrow/driver/jdbc/accessor/impl/binary/ArrowFlightJdbcBinaryVectorAccessorTest.java

+  @MethodSource("data")
+  public void testShouldGetStringReturnExpectedString(Supplier<ValueVector> vectorSupplier)
+      throws Exception {
+    setup(vectorSupplier);


So we have to call this function in each test method?

Based on the information provided by @lidavidm, it will eventually be improved in the future.

It appears junit-team/junit5#878 is the feature request.

GH-42025: [Java] Update Unit Tests for Algorithm Module #42029 (comment)

vibhatha · 2024-06-18T15:06:25Z

...t-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/CoreMockedSqlProducers.java

-    collector.checkThat(actualHireDates, is(expectedHireDates));
-    collector.checkThat(actualLastSales, is(expectedLastSales));
+
+    final int finalActualRowCount = actualRowCount;


is this required to be final?

I meant do we need a separate variable?

If we don't use final, we'll see the error: "Variable used in lambda expression should be final or effectively final."

vibhatha · 2024-06-19T00:04:42Z

...flight-sql-jdbc-driver/src/test/java/org/apache/arrow/driver/jdbc/ITDriverJarValidation.java

@@ -113,7 +105,7 @@ public void validateShadedJar() throws IOException {
   * allowed prefixes
   *
   * @param name the jar entry name
-   * @throws AssertionException if the entry is not allowed
+   * @throws AssertionError if the entry is not allowed


Just curious, how this change comes from? I assume the Jupiter API is using this?

The AssertionError is a built-in class in Java.

AssertionError

The previous code had a typo; it actually used AssertionError, but the comment described it as AssertionException. That's why I corrected it.

arrow/java/flight/flight-sql-jdbc-driver/src/test/java/org/apache/arrow/driver/jdbc/ITDriverJarValidation.java

Lines 117 to 140 in 9f0101e

/**

* Check if a jar entry is allowed.

*

* <p>

* A jar entry is allowed if either it is part of the allowed files or it

* matches one of the allowed prefixes

*

* @param name the jar entry name

* @throws AssertionException if the entry is not allowed

*/

private void checkEntryAllowed(String name) {

// Check if there's a matching file entry first

if (ALLOWED_FILES.contains(name)) {

return;

}

for (String prefix : ALLOWED_PREFIXES) {

if (name.startsWith(prefix)) {

return;

}

}

throw new AssertionError("'" + name + "' is not an allowed jar entry");

}

conbench-apache-arrow · 2024-06-19T08:40:21Z

After merging your PR, Conbench analyzed the 8 benchmarking runs that have been run so far on merge-commit 9d93287.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.

llama90 added 3 commits June 15, 2024 11:27

update org.junit.Assert to org.junit.jupiter.api.Assertions

e6a1ea9

apply lint using 'mvn spotless:apply'

3d8d257

llama90 requested a review from lidavidm as a code owner June 15, 2024 13:15

github-actions bot added Component: Java awaiting review Awaiting review labels Jun 15, 2024

llama90 added 8 commits June 15, 2024 22:23

update unit test from junit 4 to junit 5 for flight-sql

5079faf

update unit test from junit 4 to junit 5 for flight-sql-jdbc-driver

b4bc2fd

feat(flight-sql-jdbc-core): update org.junit.jupiter.api.Assertions

7752861

feat(flight-sql-jdbc-core): update ErrorCollector to assert statements

4b64204

feat(flight-sql-jdbc-core): update @RuleClass to ExtensionClass for F…

b8913b7

…lightServerTestExtension

feat(flight-sql-jdbc-core): update @test only

983b85c

feat(flight-sql-jdbc-core): update @RuleClass to ExtensionClass for R…

bb04df9

…ootAllocatorTestExtension

feat(flight-sql-jdbc-core): update the remaining parts

410fcea

llama90 commented Jun 16, 2024

View reviewed changes

...ight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/FlightServerTestExtension.java Outdated Show resolved Hide resolved

llama90 commented Jun 16, 2024

View reviewed changes

...l-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/RootAllocatorTestExtension.java Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jun 16, 2024

llama90 commented Jun 16, 2024

View reviewed changes

...ache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcIntervalVectorAccessorTest.java Outdated Show resolved Hide resolved

llama90 commented Jun 16, 2024

View reviewed changes

feat(flight-sql-jdbc-core): update parameterizedTest for Test methods

8490578

lidavidm reviewed Jun 17, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jun 17, 2024

llama90 added 2 commits June 18, 2024 19:00

reflect review feedback on exception handling

0b6573d

reflect review feedback by changing assertTrue to assertEquals

4ea9178

reflect review feedback on exception handling

323c709

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 18, 2024

vibhatha reviewed Jun 18, 2024

View reviewed changes

llama90 mentioned this pull request Jun 18, 2024

[Java] Enhance Parameterized tests following JUnit improvements apache/arrow-java#73

Open

lidavidm approved these changes Jun 18, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jun 18, 2024

vibhatha reviewed Jun 19, 2024

View reviewed changes

vibhatha approved these changes Jun 19, 2024

View reviewed changes

lidavidm merged commit 9d93287 into apache:main Jun 19, 2024
32 checks passed

lidavidm removed the awaiting merge Awaiting merge label Jun 19, 2024

github-actions bot added the awaiting committer review Awaiting committer review label Jun 19, 2024

llama90 mentioned this pull request Jun 19, 2024

[Java] Update Unit Tests for Flight Module #42045

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-42045: [Java] Update Unit Tests for Flight Module #42158

GH-42045: [Java] Update Unit Tests for Flight Module #42158

llama90 commented Jun 15, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Jun 15, 2024

llama90 Jun 16, 2024

lidavidm Jun 17, 2024

llama90 Jun 18, 2024

llama90 Jun 16, 2024

lidavidm Jun 17, 2024

lidavidm Jun 17, 2024

llama90 Jun 18, 2024

llama90 Jun 16, 2024

llama90 left a comment •

edited

Loading

lidavidm Jun 17, 2024

llama90 Jun 18, 2024 •

edited

Loading

lidavidm Jun 17, 2024

lidavidm Jun 17, 2024

lidavidm Jun 17, 2024

llama90 commented Jun 18, 2024

vibhatha Jun 18, 2024

llama90 Jun 18, 2024

vibhatha Jun 18, 2024

vibhatha Jun 18, 2024

llama90 Jun 18, 2024

vibhatha Jun 18, 2024

vibhatha Jun 18, 2024

llama90 Jun 18, 2024

vibhatha Jun 18, 2024

vibhatha Jun 18, 2024

llama90 Jun 18, 2024

vibhatha Jun 19, 2024

llama90 Jun 19, 2024

conbench-apache-arrow bot commented Jun 19, 2024

	try {
	columns.add((T) resultSet.getObject(columnName));
	} catch (final SQLException e) {
	throw new RuntimeException(e);
	}

	if (Float.isInfinite(value) \|\| Float.isNaN(value)) {
	assertThrows(SQLException.class, accessor::getBigDecimal);
	} else {
	assertThat(accessor.getBigDecimal(), is(BigDecimal.valueOf(value)));
	}

	if (Float.isInfinite(value) \|\| Float.isNaN(value)) {
	assertThrows(SQLException.class, () -> accessor.getBigDecimal(9));
	} else {
	assertThat(
	accessor.getBigDecimal(9),
	is(BigDecimal.valueOf(value).setScale(9, RoundingMode.HALF_UP)));
	}

	if (Double.isInfinite(value) \|\| Double.isNaN(value)) {
	assertThrows(SQLException.class, accessor::getBigDecimal);
	} else {
	assertThat(accessor.getBigDecimal(), is(BigDecimal.valueOf(value)));
	}

	/**
	* Check if a jar entry is allowed.
	*
	* <p>
	* A jar entry is allowed if either it is part of the allowed files or it
	* matches one of the allowed prefixes
	*
	* @param name the jar entry name
	* @throws AssertionException if the entry is not allowed
	*/
	private void checkEntryAllowed(String name) {
	// Check if there's a matching file entry first
	if (ALLOWED_FILES.contains(name)) {
	return;
	}

	for (String prefix : ALLOWED_PREFIXES) {
	if (name.startsWith(prefix)) {
	return;
	}
	}

	throw new AssertionError("'" + name + "' is not an allowed jar entry");
	}

GH-42045: [Java] Update Unit Tests for Flight Module #42158

GH-42045: [Java] Update Unit Tests for Flight Module #42158

Conversation

llama90 commented Jun 15, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Jun 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llama90 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llama90 Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llama90 commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Jun 19, 2024

llama90 commented Jun 15, 2024 •

edited by github-actions bot

Loading

llama90 left a comment •

edited

Loading

llama90 Jun 18, 2024 •

edited

Loading