stefanbirkner / system-rules

A collection of JUnit rules for testing code which uses java.lang.System.
http://stefanbirkner.github.io/system-rules
Other
546 stars 71 forks source link

pom: Use "junit" instead of the deprecated "junit-dep" artifact name #60

Closed sschuberth closed 6 years ago

sschuberth commented 6 years ago

"junit-dep" was moved to "junit", see the notice in the header of

https://mvnrepository.com/artifact/junit/junit-dep

stefanbirkner commented 6 years ago

Hello Sebastian,

thank you for your interest in System Rules and for creating the pull request.

I have chosen the dependency junit:junit-dep with care. JUnit 4.9 is the minimal version of JUnit that is needed by System Rules because it needs the TestRule interface. For JUnit 4.9 it is recommended to use junit-dep because it does not contain Hamcrest classes. By declaring the dependency

    <dependency>
        <groupId>junit</groupId>
        <artifactId>junit-dep</artifactId>
        <version>[4.9,)</version>
    </dependency>

I make it possible to use System Rules with every JUnit version >= 4.9.

You already noticed that this means that people run their tests with junit:junit:jar:4.11-beta-1 if they have no explicit dependency to JUnit. I think it is not the responsibility of System Rules to provide an up-to-date JUnit. Developers can do this by themselves. E.g they can declare the dependency to JUnit 4.12 explicitly in their project

    <dependency>
        <groupId>com.github.stefanbirkner</groupId>
        <artifactId>system-rules</artifactId>
        <version>1.17.1</version>
    </dependency>
    <dependency>
        <groupId>junit</groupId>
        <artifactId>junit</artifactId>
        <version>4.12</version>
    </dependency>
sschuberth commented 6 years ago

Hi Stefan,

thanks for the explanation, however some questions remain:

For JUnit 4.9 it is recommended to use junit-dep because it does not contain Hamcrest classes.

But why is it beneficial to use a version that does not contain Hamcrest classes? Does it hurt to simply leave them unused?

I think it is not the responsibility of System Rules to provide an up-to-date JUnit. Developers can do this by themselves.

I agree. If you depend on JUnit, you should declare that dependency yourself instead of relying on a transitive dependency, which might change any time with updates. But the point is that nowadays people usually use a recent version of JUnit with the junit artifact id. So unless you do some build system magic, you end up having both junit-dep and junit, which is not so nice.

stefanbirkner commented 6 years ago

But why is it beneficial to use a version that does not contain Hamcrest classes? Does it hurt to simply leave them unused?

You may run into classloading issues. See java.lang.NoClassDefFoundError: org/hamcrest/SelfDescribing

I agree. If you depend on JUnit, you should declare that dependency yourself instead of relying on a transitive dependency, which might change any time with updates. But the point is that nowadays people usually use a recent version of JUnit with the junit artifact id. So unless you do some build system magic, you end up having both junit-dep and junit, which is not so nice.

That's not true for Maven and I expect other build tools like Gradle handle this correctly to. The build tool knows that junit-dep has been moved to junit and therefore finds out that junit replaces junit-dep.

sschuberth commented 6 years ago

You may get classloading issues.

That should not be the case anymore as JUnit does not bundle (an obsolete version of) Hamcrest anymore. See how this says "The artifact junit:junit does not contain Hamcrest anymore but declares a dependency to Hamcrest. Thus, junit:junit-dep has become obsolete.".

I expect other build tools like Gradle handle this correctly to.

At least in my tests Gradle was not handling this automatically. I had to explicitly use dependency substitution like

configurations.all {
    resolutionStrategy.eachDependency { DependencyResolveDetails details ->
        if (details.requested.name == 'junit-dep') {
            // Prefer more recent versions published under the new 'junit' artifact name instead of under the
            // deprecated 'junit-dep' artifact name.
            details.useTarget group: details.requested.group, name: 'junit', version: details.requested.version
        }
    }
}

How would Maven know about this automatically?

stefanbirkner commented 6 years ago

How would Maven know about this automatically?

Have a look at the node project/distributionManagement/relocation in pom.xml of junit-dep 4.11

sschuberth commented 6 years ago

Funny, I was quoting basically the same line above about junit-dep being obsolete, but did not realize the relocation 😆 Anyway, if even the maintains of JUnit say junit-dep is obsolete, I'm clueless why to stick to it.

stefanbirkner commented 6 years ago

I just tried with Gradle and everything works fine, too. I have this build.gradle

apply plugin: 'java'

repositories {
  mavenCentral()
}

dependencies {
  testCompile 'com.github.stefanbirkner:system-rules:1.17.0'
  testCompile 'junit:junit:4.12'
}

and gradle -q dependencies shows me

testRuntime - Runtime dependencies for source set 'test'.

+--- com.github.stefanbirkner:system-rules:1.17.0
|    \--- junit:junit-dep:[4.9,) -> 4.11
|         \--- junit:junit:4.11 -> 4.12
|              \--- org.hamcrest:hamcrest-core:1.3
\--- junit:junit:4.12 (*)

I use Gradle 3.2.1 for the example.

sschuberth commented 6 years ago

Interesting, thanks for trying. I need to dig out then what went wrong in my case then...

stefanbirkner commented 6 years ago

You're welcome.

sschuberth commented 6 years ago

For the record: In my case the issue was that junit:junit-dep:[4.9,) gets resolved to junit-dep:4.11.20120805.1225 which is considered newer than junit-dep:4.11, so the relocation defined in the POM of junit-dep:4.11 does not kick in.

stefanbirkner commented 6 years ago

Just because I'm curious. Which repository provides this artifact? Are you using snapshot repositories?

sschuberth commented 6 years ago

It's from a cache for some legacy Nexus repository inside an internal virtual Artifactory repository. Not sure how the artifact actually got there.

stefanbirkner commented 6 years ago

Thanks for the information.