laurentgo commented on a change in pull request #2244:
URL: https://github.com/apache/drill/pull/2244#discussion_r644991563
##########
File path: common/pom.xml
##########
@@ -37,11 +37,21 @@
<artifactId>drill-protocol</artifactId>
<version>${project.version}</version>
</dependency>
+ <dependency>
Review comment:
I'm not sure why the engine has to be included as a dependency vs adding
`org.junit.jupiter:junit-jupiter-api` which contains the JUnit API parts (like
the annotations/assertions).
It seems the surefire should be able to pick up the engine based on the
actual JUnit APIs being used in the codebase (but I guess the surefire JUnit5
page at
https://maven.apache.org/surefire/maven-surefire-plugin/examples/junit-platform.html
can be quite confusing).
On another project I'm part of, we actually had some issues between surefire
plugin and junit engines being added as depedencies were the versions were not
in sync, and it seemed the safest way was to let the surefire plugin decides
which junit5 engine version to use for running the tests
##########
File path: common/pom.xml
##########
@@ -37,11 +37,21 @@
<artifactId>drill-protocol</artifactId>
<version>${project.version}</version>
</dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-engine</artifactId>
+ <version>${junit.version}</version>
Review comment:
Shouldn't we use dependencyManagement for this? (It seems the project
uses it sometimes but not always)?
Looks like JUnit project publishes a BOM artifact (org.junit:junit-bom) to
make sure that all JUnit dependencies are on the same version.
Also, shouldn't this dependencies being provided as test dependencies (since
no main code seems to reference junit5 api). In which case, I guess they could
even be dropped since they are declared with the right scope in the top level
pom.xml this module is a child of?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
