-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-42030: [Java] Update Unit Tests for Adapter Module #42038
Conversation
|
I currently use explicit calls to Reason
|
/** | ||
* This method creates Connection object and DB table and also populate data into table for test. | ||
* | ||
* @throws SQLException on error | ||
* @throws ClassNotFoundException on error | ||
*/ | ||
@Before | ||
public void setUp() throws SQLException, ClassNotFoundException { | ||
protected void initializeDatabase(Table table) throws SQLException, ClassNotFoundException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vibhatha I'll look into it more, but if you have any suggestions for a better structure, I would really appreciate your advice.
cc @lidavidm
classDiagram
direction LR
class AbstractJdbcToArrowTest {
}
class JdbcToArrowCharSetTest {
}
class JdbcToArrowDataTypesTest {
}
class JdbcToArrowMapDataTypeTest {
}
class JdbcToArrowNullTest {
}
class JdbcToArrowOptionalColumnsTest {
}
class JdbcToArrowTest {
}
class JdbcToArrowTimeZoneTest {
}
class JdbcToArrowVectorIteratorTest {
}
AbstractJdbcToArrowTest <|-- JdbcToArrowCharSetTest
AbstractJdbcToArrowTest <|-- JdbcToArrowDataTypesTest
AbstractJdbcToArrowTest <|-- JdbcToArrowMapDataTypeTest
AbstractJdbcToArrowTest <|-- JdbcToArrowNullTest
AbstractJdbcToArrowTest <|-- JdbcToArrowOptionalColumnsTest
AbstractJdbcToArrowTest <|-- JdbcToArrowTest
AbstractJdbcToArrowTest <|-- JdbcToArrowTimeZoneTest
JdbcToArrowTest <|-- JdbcToArrowVectorIteratorTest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@llama90 I will review this tomorrow. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what you are concerned about is making the database connection for each test case with @BeforeEach
setup method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need to create a database connection with a different .yml
for each test, but @BeforeEach
cannot take arguments for @ParameterizedTest
.
I described something similar here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine
a5a328a
to
64f8b60
Compare
Ummm.... There is a JUnit 5 bug on Windows (Server 2022) with JDK 11, specifically in the CI used by Arrow.
Should we consider a rollback because of this, or is there another solution we could explore? error message
UPDATE: I tried two kinds of approaches Using
|
@llama90 thanks for taking a very thorough assessment on this. I think the issue still persists. Furthermore looking into the JUNIT5 bug report, it seems that there isn't a solid fix yet. So my suggestion would be to fix it once that's resolved. Although, we can create two sub-issues for this ticket, one for functional conversion (orc) and other for non-functional bits. cc @lidavidm Appreciate your feedback. |
@ClassRule | ||
public static final TemporaryFolder TMP = new TemporaryFolder(); | ||
@TempDir | ||
public File TMP; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new JUnit API is different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that in JUnit 5, the alternative for temporary directories is to use @TempDir
. So I modified it accordingly.
@TempDir
can be used to annotate a field in a test class or a parameter in a lifecycle method or test method of type Path or File that should be resolved into a temporary directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that seems to be related, though can we re-check by running CIs?
java/adapter/jdbc/src/test/java/org/apache/arrow/adapter/jdbc/h2/JdbcToArrowCharSetTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a merge might have gone wrong? There are changes from other PRs
@ClassRule | ||
public static final TemporaryFolder TMP = new TemporaryFolder(); | ||
@TempDir | ||
public File TMP; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new JUnit API is different.
/** | ||
* This method creates Connection object and DB table and also populate data into table for test. | ||
* | ||
* @throws SQLException on error | ||
* @throws ClassNotFoundException on error | ||
*/ | ||
@Before | ||
public void setUp() throws SQLException, ClassNotFoundException { | ||
protected void initializeDatabase(Table table) throws SQLException, ClassNotFoundException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine
java/bom/pom.xml
Outdated
<licenseHeader> | ||
<file>${maven.multiModuleProjectDirectory}/dev/license/asf-xml.license</file> | ||
<delimiter>(<configuration|<project)</delimiter> | ||
</licenseHeader> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did this come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, maybe this is a merge artifact...it seems to already be on main?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a rebase and saw that changes from another PR were left out.
What do you think about issues with @TempDir
when using JDK 11 on Windows?
If possible, can I fix the problem with a new PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lidavidm we seems to have a problem here with Windows: https://github.com/apache/arrow/actions/runs/9490384583/job/26153755020?pr=42038#step:5:10625
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to fix it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be helpful?
- initize database within the test methods
35e7777
to
dd84928
Compare
bc536e6
to
fb22cad
Compare
@llama90 the CI failures are due to spotless issue. Could you please try |
Yes, I tried to check ( I can't test the environment for Windows locally, I am repeatedly testing it through CI. |
8fde75e
to
b496322
Compare
Yeah that happens, and after pre-commit that will be warned before committing.
I understand. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was getting an error in Windows saying "failed to delete temp directory" because "The process cannot access the file because it is being used by another process."
To fix this, I found out that using try-with-resources
to properly close resources would help.
But for the convert
method, the FileInputStream
(fis) needs to stay open for the iterator
after the convert
method is called. This caused the error because the fis
wasn't properly closed. So, I made a writeDataToFile
method for the FileOutputStream
and used try-with-resources
for fis
in the same method where the iterator
is used.
Now, the tests run fine on Windows too.
Here are the different approaches I tried:
Attempt 1:
convert method:
- FileOutputStream with
try-with-resources
- FileInputStream with
try-with-resources
Error: Stream Closed
- because the fis
was closed while the iterator
still needed it.
Attempt 2:
convert method:
- FileOutputStream with
try-with-resources
- FileInputStream
Error: failed to delete temp directory
- because the fis
wasn't closed.
Attempt 3:
convert method:
- FileOutputStream with
try-with-resources
- Added
FileInputStream
as a parameter to the convert method and explicitly closed it in the test method.
Didn't work as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pushing this one and evaluating diverse approaches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the changes look okay to me. And using try-with-resources is actually encouraged, may be this upgrade showed an issue in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for figuring it out! So basically, we weren't properly closing the file in all cases and that stopped Windows from deleting the temp files?
File deletion operations differ across operating systems. Specifically, Windows only marks a file for deletion on close. Therefore, it's important to use explicit resource management, such as try-with-resources, to ensure proper release of resources. |
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 870b315. There were 12 benchmark results indicating a performance regression:
The full Conbench report has more details. It also includes information about 59 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
Update package from JUnit 4(
org.junit
) to JUnit 5(org.junit.jupiter
).What changes are included in this PR?
avro
andjdbc
moduleorg.junit
withorg.junit.jupiter.api
.Assertions.assertXXX
toassertXXX
using static imports.@Before
,@After
.@Before
->@BeforeEach
@After
->@AfterEach
@Test
->@Test
withorg.junit.jupiter
@ClassRule
->@TempDir
and@BeforeAll
Parameterized
testjava.io.IOException: Failed to delete temp directory
on Windows with JDK 11ParameterizedTest
in JDBC tests.orc
module@BeforeAll
,@Rule
,@TemporaryFolder
Are these changes tested?
Yes, existing tests have passed.
Are there any user-facing changes?
No.