[Enchancement] Removed dependency between "test" and "jacocoTestReport" #2935
Conversation
Closes issue opensearch-project#2913. This will allow us to not to exclude the "test" job from every other test. Unfortunately, this will break creating the coverage reports from only starting "jacocoTestReport" task in Gradle. Signed-off-by: Pawel Gudel <[email protected]>
pawel-gudel-eliatra
requested review from
cliu123,
cwperks,
DarshitChanpura,
davidlago,
peternied,
RyanL1997,
stephen-crawford,
reta and
willyborankin
as code owners
Codecov Report
@@ Coverage Diff @@
## main #2935 +/- ##
============================================
- Coverage 62.31% 61.78% -0.53%
+ Complexity 3337 3306 -31
============================================
Files 266 266
Lines 19650 19650
Branches 3329 3329
============================================
- Hits 12244 12141 -103
- Misses 5779 5873 +94
- Partials 1627 1636 +9 |
| @@ -390,8 +390,6 @@ tasks.withType(JavaCompile) { | |||
| } | |||
|
|
|||
| tasks.test.finalizedBy(jacocoTestReport) // report is always generated after tests run | |||
| tasks.jacocoTestReport.dependsOn(test) // tests are required to run before generating the report | |||
There was a problem hiding this comment.
I don't think this is the right thing to do: Gradle uses build cache (see please [1]) which should run test task only once. The build essentially becomes unusable for regular contributors since they have to closely follow whatever the GA workflow does, not just run ./gradlew check and get the code coverage.
[1] https://docs.gradle.org/current/userguide/build_cache.html#sec:task_output_caching_details
There was a problem hiding this comment.
Unfortunately I do not understand the issue here. Running ./gradlew check does not disable creation of code coverage report. Output below:
root@8aadb8ef940c:/app# ./gradlew check --dry-run
=======================================
OpenSearch Build Hamster says Hello!
Gradle Version : 8.1.1
OS Info : Linux 6.3.8-200.fc38.x86_64 (amd64)
JDK Version : 17 (Eclipse Temurin JDK)
JAVA_HOME : /root/.sdkman/candidates/java/17.0.7-tem
Random Testing Seed : 41734A7976B6C119
In FIPS 140 mode : false
=======================================
:compileJava SKIPPED
:processResources SKIPPED
:classes SKIPPED
:compileIntegrationTestJava SKIPPED
:processIntegrationTestResources SKIPPED
:integrationTestClasses SKIPPED
:checkstyleIntegrationTest SKIPPED
:checkstyleMain SKIPPED
:compileTestJava SKIPPED
:copyPluginPropertiesTemplate SKIPPED
:pluginProperties SKIPPED
:processTestResources SKIPPED
:testClasses SKIPPED
:checkstyleTest SKIPPED
:forbiddenApisResources SKIPPED
:forbiddenApisIntegrationTest SKIPPED
:forbiddenApisMain SKIPPED
:forbiddenApisTest SKIPPED
:forbiddenApis SKIPPED
:dependencyLicenses SKIPPED
:filepermissions SKIPPED
:forbiddenPatterns SKIPPED
:jarHell SKIPPED
:licenseHeaders SKIPPED
:loggerUsageCheck SKIPPED
:testingConventions SKIPPED
:thirdPartyAuditResources SKIPPED
:thirdPartyAudit SKIPPED
:generatePomFileForMavenPublication SKIPPED
:generatePomFileForNebulaPublication SKIPPED
:generatePomFileForPluginZipPublication SKIPPED
:validateMavenPom SKIPPED
:validateNebulaPom SKIPPED
:validatePluginZipPom SKIPPED
:validatePom SKIPPED
:precommit SKIPPED
:integTest SKIPPED
:copyExtraTestResources SKIPPED
:test SKIPPED
:integrationTest SKIPPED
:javadoc SKIPPED
:spotbugsIntegrationTest SKIPPED
:spotbugsMain SKIPPED
:spotbugsTest SKIPPED
:spotlessInternalRegisterDependencies SKIPPED
:spotlessJava SKIPPED
:spotlessJavaCheck SKIPPED
:spotlessMisc SKIPPED
:spotlessMiscCheck SKIPPED
:spotlessCheck SKIPPED
:jacocoTestReport SKIPPED
:check SKIPPED
Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.
You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.
See https://docs.gradle.org/8.1.1/userguide/command_line_interface.html#sec:command_line_warnings
BUILD SUCCESSFUL in 1s
When you start test the code coverage ("jacocoTestReport" task) is always started. This PR only removed reverse dependency: a situation when running ./gradlew jacocoTestReport also forces test to be run.
There was a problem hiding this comment.
Running ./gradlew check does not disable creation of code coverage report.
And
When you start test the code coverage ("jacocoTestReport" task) is always started.
That is coming from:
tasks.test.finalizedBy(jacocoTestReport) // report is always generated after tests run
This PR only removed reverse dependency: a situation when running ./gradlew jacocoTestReport also forces test to be run.
Exactly, and this is my question - the jacocoTestReport by definition needs test to be run (otherwise there is no input to build the report), so if one runs ./gradlew jacocoTestReport it would make sense to have tests to be run before
However, if ones run those two in sequence
./gradlew test
./gradlew jacocoTestReport
The jacocoTestReport should not cause test to be rerun
There was a problem hiding this comment.
Unfortunately I still do not quite understand issue here. I'll try to answer one-by-one:
Exactly, and this is my question - the jacocoTestReport by definition needs test to be run (otherwise there is no input to build the report)
Yes, it needs to be run but not as a separate task. It is run always after the "test" task.
so if one runs ./gradlew jacocoTestReport it would make sense to have tests to be run before
What should make sense is to start "test", not "jacocoTestReport".
However, if ones run those two in sequence
./gradlew test
./gradlew jacocoTestReport
The jacocoTestReport should not cause test to be rerun
This is not the scope of this PR. I don't understand why would you like to "jacocoTestReport" as a separate task after running "test" ("jacocoTestReport" is always started after "test" already!). In your execution of two command Gradle runs following things:
- (on current master): "test" -> "jacocoTestReport" -> "test" -> jacocoTestReport"
- (on my PR): "test" -> "jacocoTestReport" -> "jacocoTestReport"
Why would someone like to do that? Just run "./gradlew test" once and get both "test" and "jacocoTestReport" in the same run.
Also, I've verified and "check" job starts "jacocoTestReport" but only as a part of "test" job. This PR does not break creation of code coverage by running "check" task.
There was a problem hiding this comment.
@peternied adding you as discussed
From our point of view
./gradlew test
is the only thing a dev on their local system would need. Not sure if we are missing something.
There was a problem hiding this comment.
This is not the scope of this PR. I don't understand why would you like to "jacocoTestReport" as a separate task after running "test" ("jacocoTestReport" is always started after "test" already!). In your execution of two command Gradle runs following things:
I think this is the core of the issue:
- the
testdoes not needjacocoTestReport(one could run tests without coverage) - the
jacocoTestReportneedstest(one needs tests for the reports)
Why would someone like to do that? Just run "./gradlew test" once and get both "test" and "jacocoTestReport" in the same run.
I think removing this dependency would make sense
tasks.test.finalizedBy(jacocoTestReport)
but not the dependency between test and jacocoTestReport
There was a problem hiding this comment.
is the only thing a dev on their local system would need. Not sure if we are missing something.
@nibix if ./gradlew test assumes tests and coverage report (which at least in OpenSearch core is two separated tasks at the moment), than the change is fine.
There was a problem hiding this comment.
I would recommend that executing tests is separate from the generation of a coverage report. As we have many checks that are only defined by GitHub Actions.
I've created an issue to track this area and to solict more feedback.
|
Hi @pawel-gudel-eliatra, just checking in on the status of this PR. Is there anything you need help with to wrap this up? @reta, did you get a chance to review @pawel-gudel-eliatra's response above? Thank you both. |
Oh ... I totally missed that response, my sincere apologies, let me look into that right now |
|
@reta @peternied I just wanted to double-check: After filing #3045, shall we wait for feedback coming via that issue? Or how shall we proceed here? |
There was a problem hiding this comment.
LGTM. Thank you @pawel-gudel-eliatra !
From what I understand, this PR prevents circular dependency between jacocoTestReport and test task allowing test to not be skipped in Build and Test step of CI.
Description
As a part of discussions in #2861 there's been determined that CI workflow in Github Actions is excluding a lot of unnecessary task. @nibix created #2913 to address this problem. Most of the unneeded tasks has been already removed in #2861. This PR removes last existing part. @peternied's confirmed removal of dependency in his comment.
This change removes the dependency between "jacocoTestReport" and "test" tasks. Specifically the dependency that forces to start "test" task always when "jacocoTestReport" is called. This allows us to always generate coverage report in the end of every task run in "CI" workflow without having to exclude "test" task in every single one of them. Unfortunately this will break functionality to create code coverage by starting only "jacocoTestReport" task. It looks like this was not used widely and was certainly not used in any of Github Actions Workflows. This will not break creating coverage reports in the end of "test" task.
Issues Resolved
#2913
Testing
Manual tests been done to verify that output is in line with expectations. No tests are being added nor are needed.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.