Skip to content
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-42193: [Java] Update dependency to maintain JUnit 5 only #42206

Merged
merged 4 commits into from
Jun 21, 2024

Conversation

llama90
Copy link
Contributor

@llama90 llama90 commented Jun 19, 2024

Rationale for this change

We have completed migrating all unit test code from JUnit 4 to JUnit 5 for all Java modules. Now, we are removing the compatibility library junit-vintage-engine to maintain JUnit 5 only.

What changes are included in this PR?

  • Removing JUnit 4 dependencies
  • Updating remaining junit.framework.TestCase imports
  • Updating excludes to prevent JUnit 4 dependencies

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

@llama90
Copy link
Contributor Author

llama90 commented Jun 19, 2024

@vibhatha Is it sufficient to just remove the junit-vintage-engine dependency?

Also, I encountered an error after merging my Flight Module commit as below. However, the current machine was not used to migrate the Flight Module. The machine has a tricky environment, so I cannot be sure if the error is occurring for everyone.

However, the CIs seem to be working well.

[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.026 s <<< FAILURE! -- in org.apache.arrow.driver.jdbc.ITDriverJarValidation
[ERROR] org.apache.arrow.driver.jdbc.ITDriverJarValidation.validateShadedJar -- Time elapsed: 0.011 s <<< FAILURE!
org.opentest4j.AssertionFailedError: jar ==> expected: <Driver jar was not detected in the classpath> but was: <jar>
        at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
        at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
        at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
        at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
        at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1156)
        at org.apache.arrow.driver.jdbc.ITDriverJarValidation.getJdbcJarFile(ITDriverJarValidation.java:72)
        at org.apache.arrow.driver.jdbc.ITDriverJarValidation.validateShadedJar(ITDriverJarValidation.java:84)
        at java.base/java.lang.reflect.Method.invoke(Method.java:580)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1597)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1597)

[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   ITDriverJarValidation.validateShadedJar:84->getJdbcJarFile:72 jar ==> expected: <Driver jar was not detected in the classpath> but was: <jar>
[INFO] 
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0

@vibhatha
Copy link
Collaborator

I also got the same error locally when I was testing some other feature. We may need to take a look. Could you file an issue?

@vibhatha
Copy link
Collaborator

Also I am not sure if that would be the only dependency. Can we try intellisense and see if we still get the Junit4?

@llama90
Copy link
Contributor Author

llama90 commented Jun 19, 2024

Also I am not sure if that would be the only dependency. Can we try intellisense and see if we still get the Junit4?

We can check the dependencies using mvn dependency.

There are no JUnit 4.x dependencies.

So, I believe there is no need for further checks.

I attached result.

 mvn dependency:tree | grep junit 
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test
[INFO] +- org.junit.jupiter:junit-jupiter-engine:jar:5.10.2:test
[INFO] |  +- org.junit.platform:junit-platform-engine:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.10.2:test
[INFO] |  \- org.junit.platform:junit-platform-commons:jar:1.10.2:test
[INFO] +- org.junit.jupiter:junit-jupiter-params:jar:5.10.2:test
[INFO] +- org.mockito:mockito-junit-jupiter:jar:2.25.1:test

@llama90
Copy link
Contributor Author

llama90 commented Jun 19, 2024

I also got the same error locally when I was testing some other feature. We may need to take a look. Could you file an issue?

That's right. The issue appears intermittently. I'll summarize the details and create an issue.

@lidavidm
Copy link
Member

I believe we have a way to configure disallowed dependencies for a stronger check

@lidavidm
Copy link
Member

Ah, the root POM has maven-enforcer-plugin which can ban junit4 entirely

@vibhatha
Copy link
Collaborator

maven-enforcer-plugin

I took a look, we already listed a few such dependencies. That's perfect. Let's use that.

@@ -659,6 +646,9 @@ under the License.
<!-- Do not include annotation processors. Use the annotations-only artifacts -->
<exclude>org.immutables:value</exclude>
<exclude>org.checkerframework:checker</exclude>
<!-- Exclude all JUnit 4 dependencies -->
<exclude>junit:junit:4.*</exclude>
<exclude>org.junit.vintage:junit-vintage-engine</exclude>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated it. Thank you for review!

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jun 19, 2024
@lidavidm
Copy link
Member

@github-actions crossbow submit -g java

Copy link

Revision: be5e51a

Submitted crossbow builds: ursacomputing/crossbow @ actions-cc74f066f5

Task Status
java-jars GitHub Actions
test-conda-python-3.10-spark-v3.5.0 GitHub Actions
test-conda-python-3.11-spark-master GitHub Actions
test-conda-python-3.8-spark-v3.5.0 GitHub Actions
verify-rc-source-java-linux-almalinux-8-amd64 GitHub Actions
verify-rc-source-java-linux-conda-latest-amd64 GitHub Actions
verify-rc-source-java-linux-ubuntu-20.04-amd64 GitHub Actions
verify-rc-source-java-linux-ubuntu-22.04-amd64 GitHub Actions
verify-rc-source-java-macos-amd64 GitHub Actions

@llama90 llama90 changed the title GH-42193: [Java] Remove junit-vintage-engine dependency to maintain JUnit 5 only GH-42193: [Java] Update dependency to maintain JUnit 5 only Jun 19, 2024
@llama90
Copy link
Contributor Author

llama90 commented Jun 19, 2024

I also got the same error locally when I was testing some other feature. We may need to take a look. Could you file an issue?

I created issue for comment

@lidavidm
Copy link
Member

just making sure java-jars passes now

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jun 19, 2024
@vibhatha
Copy link
Collaborator

Did we re-run?

@lidavidm
Copy link
Member

@vibhatha can someone take a look at the issue? I don't think this will go away on re-run

@vibhatha
Copy link
Collaborator

@lidavidm I think @kou resolved this issue here: #42197
Probably need a rebase?

@llama90
Copy link
Contributor Author

llama90 commented Jun 20, 2024

That was rebased about two hours ago. Perhaps we could try running it again?

@vibhatha
Copy link
Collaborator

@github-actions crossbow submit java-jars

Copy link

Revision: f41f9b7

Submitted crossbow builds: ursacomputing/crossbow @ actions-5074156199

Task Status
java-jars GitHub Actions

@vibhatha
Copy link
Collaborator

@llama90 I think now we are failing at the issue we recently filed: #42208

@lidavidm
Copy link
Member

@llama90 if you want to rebase here I can re-run Crossbow

@llama90
Copy link
Contributor Author

llama90 commented Jun 21, 2024

@lidavidm Done!

@lidavidm
Copy link
Member

@github-actions crossbow submit -g java

Copy link

Revision: 4db3a9f

Submitted crossbow builds: ursacomputing/crossbow @ actions-4efa92c4b0

Task Status
java-jars GitHub Actions
test-conda-python-3.10-spark-v3.5.0 GitHub Actions
test-conda-python-3.11-spark-master GitHub Actions
test-conda-python-3.8-spark-v3.5.0 GitHub Actions
verify-rc-source-java-linux-almalinux-8-amd64 GitHub Actions
verify-rc-source-java-linux-conda-latest-amd64 GitHub Actions
verify-rc-source-java-linux-ubuntu-20.04-amd64 GitHub Actions
verify-rc-source-java-linux-ubuntu-22.04-amd64 GitHub Actions
verify-rc-source-java-macos-amd64 GitHub Actions

@lidavidm lidavidm merged commit f07d442 into apache:main Jun 21, 2024
16 checks passed
@lidavidm lidavidm removed the awaiting merge Awaiting merge label Jun 21, 2024
@github-actions github-actions bot added the awaiting merge Awaiting merge label Jun 21, 2024
Copy link

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

There were no benchmark performance regressions. 🎉

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants