gortiz commented on code in PR #13854:
URL: https://github.com/apache/pinot/pull/13854#discussion_r1741500704


##########
.github/workflows/scripts/pr-tests/.pinot_tests_unit.sh:
##########
@@ -29,24 +29,24 @@ netstat -i
 #   - TEST_SET#1 runs install and test together so the module list must ensure 
no additional modules were tested
 #     due to the -am flag (include dependency modules)
 if [ "$RUN_TEST_SET" == "1" ]; then
-  mvn test -T 16 \
+  mvn test jacoco:report-aggregate -T 16 \
       -pl 'pinot-spi' \
       -pl 'pinot-segment-spi' \
       -pl 'pinot-common' \
       -pl ':pinot-yammer' \
       -pl 'pinot-core' \
       -pl 'pinot-query-planner' \
       -pl 'pinot-query-runtime' \
-      -P github-actions,no-integration-tests || exit 1
+      -P github-actions,codecoverage,no-integration-tests || exit 1

Review Comment:
   I don't think this is working as expected. Given we are providing a 
inclusive list of projects and `-pl reports` is not included, we are not 
actually executing the aggregate defined in reports/pom.xml. What we are doing 
is to directly run `jacoco:report-aggregate` on each project, which I don't 
think it is super useful.
   
   We should also apply this change in the integration tests file 
(`.pinot_tests_integration.sh`)
   
   Am I right?



-- 
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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to