Skip to content

[Enchancement] Removed dependency between "test" and "jacocoTestReport" #2935

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DarshitChanpura merged 1 commit into from
Aug 17, 2023
Merged

[Enchancement] Removed dependency between "test" and "jacocoTestReport" #2935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ jobs:
with:
arguments: |
${{ matrix.gradle_task }} -Dbuild.snapshot=false
-x test

- name: Coverage
uses: codecov/codecov-action@v1
Expand Down
2 changes: 0 additions & 2 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

@reta reta Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@pawel-gudel-eliatra pawel-gudel-eliatra Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@reta reta Jul 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@pawel-gudel-eliatra pawel-gudel-eliatra Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. (on current master): "test" -> "jacocoTestReport" -> "test" -> jacocoTestReport"
  2. (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.

Copy link
Collaborator

@nibix nibix Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@reta reta Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 test does not need jacocoTestReport (one could run tests without coverage)
  • the jacocoTestReport needs test (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

Copy link
Collaborator

@reta reta Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@peternied peternied Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



allprojects {
tasks.withType(Javadoc).all { enabled = false }
Expand Down