[GitHub] [drill] laurentgo commented on a change in pull request #2244: DRILL-7941 Update JUnit from 4.13.2 to 5.7.2

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org