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

Ban the use of junit:junit (4.x) and org.hamcrest:hamcrest-core (1.3) #1796

Merged
merged 1 commit into from
Nov 25, 2021

Conversation

mprins
Copy link
Member

@mprins mprins commented Nov 25, 2021

update al leftover testcases to use org.junit.jupiter.api.Test, either this was leftover from #1387 or these imports snuck back in when adding/updating these test cases

see: #1794 (comment)

@mprins
Copy link
Member Author

mprins commented Nov 25, 2021

I also think we should get rid of guru.nidi:code-assert alltogether as it has a humongous dependency tree and it's only used in one testcase:

[INFO] ---------------------< com.github.oshi:oshi-core >----------------------
[INFO] Building oshi-core 6.0.0-SNAPSHOT                                  [2/5]
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- maven-dependency-plugin:3.2.0:tree (default-cli) @ oshi-core ---
[INFO] com.github.oshi:oshi-core:jar:6.0.0-SNAPSHOT
[INFO] +- net.java.dev.jna:jna:jar:5.10.0:compile
[INFO] +- net.java.dev.jna:jna-platform:jar:5.10.0:compile
[INFO] +- org.slf4j:slf4j-api:jar:1.7.32:compile
[INFO] +- org.slf4j:slf4j-simple:jar:1.7.32:test
[INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.8.1:test
[INFO] |  +- org.opentest4j:opentest4j:jar:1.2.0:test
[INFO] |  +- org.junit.platform:junit-platform-commons:jar:1.8.1:test
[INFO] |  \- org.apiguardian:apiguardian-api:jar:1.1.2:test
[INFO] +- org.hamcrest:hamcrest:jar:2.2:test
[INFO] \- guru.nidi:code-assert:jar:0.9.15:test
[INFO]    +- guru.nidi:code-assert-core:jar:0.9.15:test
[INFO]    +- com.github.spotbugs:spotbugs:jar:4.2.0:test
[INFO]    |  +- org.ow2.asm:asm:jar:9.0:test
[INFO]    |  +- org.ow2.asm:asm-analysis:jar:9.0:test
[INFO]    |  +- org.ow2.asm:asm-commons:jar:9.0:test
[INFO]    |  +- org.ow2.asm:asm-tree:jar:9.0:test
[INFO]    |  +- org.ow2.asm:asm-util:jar:9.0:test
[INFO]    |  +- org.apache.bcel:bcel:jar:6.5.0:test
[INFO]    |  +- net.jcip:jcip-annotations:jar:1.0:test
[INFO]    |  +- org.dom4j:dom4j:jar:2.1.3:test
[INFO]    |  +- org.apache.commons:commons-lang3:jar:3.11:test
[INFO]    |  +- org.apache.commons:commons-text:jar:1.9:test
[INFO]    |  +- com.github.spotbugs:spotbugs-annotations:jar:4.2.0:test
[INFO]    |  |  \- com.google.code.findbugs:jsr305:jar:3.0.2:test
[INFO]    |  +- org.json:json:jar:20200518:test
[INFO]    |  \- jaxen:jaxen:jar:1.2.0:test
[INFO]    +- com.h3xstream.findsecbugs:findsecbugs-plugin:jar:1.11.0:test
[INFO]    +- net.sourceforge.pmd:pmd-java:jar:5.8.1:test
[INFO]    |  +- net.java.dev.javacc:javacc:jar:5.0:test
[INFO]    |  +- net.sourceforge.pmd:pmd-core:jar:5.8.1:test
[INFO]    |  |  +- com.beust:jcommander:jar:1.48:test
[INFO]    |  |  +- commons-io:commons-io:jar:2.4:test
[INFO]    |  |  \- com.google.code.gson:gson:jar:2.5:test
[INFO]    |  +- net.sourceforge.saxon:saxon:jar:9.1.0.8:test
[INFO]    |  \- net.sourceforge.saxon:saxon:jar:dom:9.1.0.8:test
[INFO]    +- com.puppycrawl.tools:checkstyle:jar:6.19:test
[INFO]    |  +- antlr:antlr:jar:2.7.7:test
[INFO]    |  +- org.antlr:antlr4-runtime:jar:4.5.3:test
[INFO]    |  +- commons-beanutils:commons-beanutils:jar:1.9.2:test
[INFO]    |  +- commons-collections:commons-collections:jar:3.2.2:test
[INFO]    |  +- commons-cli:commons-cli:jar:1.3.1:test
[INFO]    |  \- com.google.guava:guava:jar:19.0:test
[INFO]    +- org.slf4j:jcl-over-slf4j:jar:1.7.30:test
[INFO]    +- org.slf4j:jul-to-slf4j:jar:1.7.30:test
[INFO]    +- org.jetbrains.kotlin:kotlin-compiler-embeddable:jar:1.3.71:test
[INFO]    |  +- org.jetbrains.kotlin:kotlin-stdlib:jar:1.3.71:test
[INFO]    |  |  +- org.jetbrains.kotlin:kotlin-stdlib-common:jar:1.3.71:test
[INFO]    |  |  \- org.jetbrains:annotations:jar:13.0:test
[INFO]    |  +- org.jetbrains.kotlin:kotlin-script-runtime:jar:1.3.71:test
[INFO]    |  +- org.jetbrains.kotlin:kotlin-reflect:jar:1.3.71:test
[INFO]    |  +- org.jetbrains.kotlin:kotlin-daemon-embeddable:jar:1.3.71:test
[INFO]    |  \- org.jetbrains.intellij.deps:trove4j:jar:1.0.20181211:test
[INFO]    +- io.gitlab.arturbosch.detekt:detekt-core:jar:1.9.1:test
[INFO]    |  +- io.gitlab.arturbosch.detekt:detekt-api:jar:1.9.1:test
[INFO]    |  |  \- org.yaml:snakeyaml:jar:1.26:test
[INFO]    |  \- io.gitlab.arturbosch.detekt:detekt-metrics:jar:1.9.1:test
[INFO]    +- io.gitlab.arturbosch.detekt:detekt-rules:jar:1.9.1:test
[INFO]    +- com.pinterest.ktlint:ktlint-core:jar:0.36.0:test
[INFO]    |  \- org.ec4j.core:ec4j-core:jar:0.2.0:test
[INFO]    \- com.pinterest.ktlint:ktlint-ruleset-standard:jar:0.36.0:test
[INFO]       \- com.pinterest.ktlint:ktlint-test:jar:0.36.0:test
[INFO]          \- org.assertj:assertj-core:jar:3.12.2:test

@mprins mprins requested a review from dbwiddis November 25, 2021 09:47
…1.3)

update al leftover testcases to use `org.junit.jupiter.api.Test`
@mprins mprins added the maven dependencies An issue with unnecessary dependencies or competing transitive dependencies label Nov 25, 2021
@dbwiddis
Copy link
Member

LGTM. Thanks for this!

Note we don’t use code-assert on the Java 11 branch but that shouldn’t matter since we are mainly just keeping the master branch clean.

@dbwiddis
Copy link
Member

Also:

  • there wern't any leftover problems from Switch tests to JUnit5 and Hamcrest matchers #1387, as they would have appeared in earlier modular releases
  • the primary reason I included code-assert was to catch checkstyle issues in unit testing on my IDE, because previously to doing that, Appveyor was the only test suite really enforcing that, and they were hard to find in the logs due to the early termination of the test, and it's so much more nice to see the exact issues highlighted in PRs with line numbers so committers can easily see and fix the problem.
    • I'm ok with removing code-assert if we actually use a properly configured maven plugin that enforces the same thing. Our current config doesn't.

@dbwiddis dbwiddis merged commit 5a4fe48 into master Nov 25, 2021
@dbwiddis dbwiddis deleted the banned-deps branch November 25, 2021 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maven dependencies An issue with unnecessary dependencies or competing transitive dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants