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.

Sign up for GitHub

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

Jump to bottom

Load junit-extensions for surefire tests #394

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Load junit-extensions for surefire tests #394

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e14ab71
Load junit-extensions for surefire tests
findepi Feb 5, 2024
a23bfd8
Update junit to 5.10.2
findepi Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
Airbase 150

* Verify JUnit tests with Airlift `junit-extensions` verifier.

Airbase 149
* Remove bval dependency
* Plugin updates:
Expand Down
15 changes: 15 additions & 0 deletions airbase/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,6 +840,7 @@
<user.timezone>${air.test.timezone}</user.timezone>
<java.awt.headless>true</java.awt.headless>
<java.util.logging.SimpleFormatter.format>%1$tY-%1$tm-%1$tdT%1$tH:%1$tM:%1$tS.%1$tL%1$tz %4$s %5$s%6$s%n</java.util.logging.SimpleFormatter.format>
<junit.jupiter.extensions.autodetection.enabled>true</junit.jupiter.extensions.autodetection.enabled>
</systemPropertyVariables>
<trimStackTrace>false</trimStackTrace>
<runOrder>random</runOrder>
Expand All @@ -862,6 +863,20 @@
${surefireEnablePreview}
${air.test.jvm.additional-arguments}
</argLine>
<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>
Comment on lines +866 to +879
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did i manage to clarify anything here?

</configuration>
</plugin>

Expand Down