Conversation
723b4cd to
64b8279
Compare
It is very useful when running JUnit tests, and harmless otherwise.
JUnit 5.10.1 has this somewhat desired feature of not ignoring overridden tests. This is important behavioral aspect changed back in 5.10.2. Worth updating if only to prevent becoming used to 5.10.1 behavior.
64b8279 to
a23bfd8
Compare
| <additionalClasspathDependencies> | ||
| <additionalClasspathDependency> | ||
| <groupId>io.airlift</groupId> | ||
| <artifactId>junit-extensions</artifactId> | ||
| <version>3</version> | ||
| <exclusions> | ||
| <!-- additionalClasspathDependencies does not support dependency resolution, so dependencies result in duplicate/conflicting classpath elements --> | ||
| <exclusion> | ||
| <groupId>*</groupId> | ||
| <artifactId>*</artifactId> | ||
| </exclusion> | ||
| </exclusions> | ||
| </additionalClasspathDependency> | ||
| </additionalClasspathDependencies> |
There was a problem hiding this comment.
surefire's additionalClasspathDependencies appends requested maven artifact along with all its dependencies onto the test classpath. It does not do any dependency management / conflict resolution, it only prints a warning about conflicting dependencies. As a result, some maven artifacts can be duplicated, and some can exist in copies differing in version.
Exclusions disables this, of course. For exclusions to work, however, junit-extensions needs not to have dependencies. Currently it depends on log-manager for InitializeLogging extension. findepi/airlift-junit-extensions#1 makes this dependency optional and is required for this PR.
There was a problem hiding this comment.
That would be nice, but not by introducing complexity in junit-extensions. Why does additionalClasspathDependencies not do dependency management? Bug or feature? Maybe raise an issue with the maven project, or check it it's in Maven 4?
makes this dependency optional and is required for this PR.
This is problematic. In the future, there may be other dependencies. We can't make each and every one of them optional.
There was a problem hiding this comment.
Why does additionalClasspathDependencies not do dependency management? Bug or feature?
it's documented, so i believe it's by design
https://maven.apache.org/surefire/maven-surefire-plugin/examples/configuring-classpath.html
Since version 3.2.0 the additionalClasspathDependencies parameter can be used to add arbitrary dependencies to your test execution classpath via their regular Maven coordinates. Those are resolved from the repository like regular Maven project dependencies and afterwards added as additional classpath elements to the end of the classpath, so you cannot use these to override project dependencies or resources (except those which are filtered with classpathDependencyExclude). All artifacts of scope compile and runtime scope from the dependency tree rooted in the given dependency are added. The parametrization works like for regular Maven dependencies in a POM. Exlusions are supported as well. Neither the dependency management section from the underlying POM is used nor are the conflicts among the different dependency trees (from the project dependencies or from the additional dependencies) automatically resolved. Conflicts lead to warnings, though, which help you clean up the classpath manually. Only external dependencies (outside the current Maven reactor) are supported.
there may be other dependencies. We can't make each and every one of them optional.
Why can't we?
there is concrete value in junit-extensions being dependency-free, so that it can be very easily pulled into every module/project testing with junit. of course, it adds more burden on junit-extensions itself (eg need to enable features dynamically), but it may be worth it.
There was a problem hiding this comment.
Did i manage to clarify anything here?
|
cc: @ksobolew |
|
Do we still need this or can I close it? @findepi |
|
i still don't mind this being merged. Do you think it shouldn't? If so, I can close. |
4 participants
It is very useful when running JUnit tests, and harmless otherwise.