drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [drill] laurentgo commented on a change in pull request #2244: DRILL-7941 Update JUnit from 4.13.2 to 5.7.2
Date Fri, 04 Jun 2021 08:03:23 GMT

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



Mime
View raw message