Skip to content
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.

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

Support the build cache and parallel test execution with the JaCoCo plugin applied #5269

Closed
JLLeitschuh opened this issue May 4, 2018 · 19 comments · Fixed by #6419
Closed
Assignees
Labels
Milestone

Comments

@JLLeitschuh
Copy link
Contributor

JLLeitschuh commented May 4, 2018

Update

The wider implications of this bug is captured well here:
#5269 (comment)

Expected Behavior

If you use the Jacoco plugin with it's default configuration, it should not break the Test task's ability to be cached.

Current Behavior

Applying the Jacoco plugin to your project makes the Test task not support caching.

task.getOutputs().doNotCacheIf("JaCoCo agent configured with `append = true`", new Spec<Task>() {
@Override
public boolean isSatisfiedBy(Task element) {
return extension.isEnabled() && extension.isAppend();
}
});

https://docs.gradle.org/current/dsl/org.gradle.testing.jacoco.plugins.JacocoTaskExtension.html#org.gradle.testing.jacoco.plugins.JacocoTaskExtension:append

Whether or not data should be appended if the destinationFile already exists. Defaults to true.

Context

It makes sense that if you have custom configuration for the Jacoco plugin you might prevent Build Caching from working, but in it's default configuration it should work.

Steps to Reproduce (for bugs)

Add the Jacoco plugin to a project and be using the Build Cache.

Workaround

Using the Kotlin DSL:

tasks.withType<Test> {
    extensions.configure(typeOf<JacocoTaskExtension>()) {
        isAppend = false
    }
}

Your Environment

------------------------------------------------------------
Gradle 4.7
------------------------------------------------------------

Build time:   2018-04-18 09:09:12 UTC
Revision:     b9a962bf70638332300e7f810689cb2febbd4a6c

Groovy:       2.4.12
Ant:          Apache Ant(TM) version 1.9.9 compiled on February 2 2017
JVM:          1.8.0_92 (Oracle Corporation 25.92-b14)
OS:           Mac OS X 10.13.3 x86_64

@wolfs
Copy link
Member

wolfs commented May 15, 2018

We would love to change the default for append in the Jacoco plugin, but that would be a breaking change :(
So we need to wait for Gradle 5.0 to change the default to append = false, so caching would be enabled by default for the Test task with JaCoCo enabled.

@wolfs wolfs added this to the 5.0 RC1 milestone May 15, 2018
@wolfs wolfs added in:jacoco-plugin a:feature A new functionality labels May 15, 2018
@wolfs
Copy link
Member

wolfs commented May 15, 2018

There is also another problem: When using parallel test execution. append needs to be true. So we actually would need to make sure to delete the analysis file at the beginning of the Test task and then set append = true for the actual Jacoco agent.

We don't know if append = true works well with parallel access to the coverage file, maybe you have some better insights.

@wolfs wolfs removed this from the 5.0 RC1 milestone May 15, 2018
@wolfs
Copy link
Member

wolfs commented May 15, 2018

If you are willing to work on a solution in the JaCoCo plugin to support parallel test execution with caching and the Jacoco plugin enabled, then we would be happy to assist you in building the corresponding PR.

I think the basic approach would be to interpret append = false in the JaCoCo extension to mean:

  • clean up the coverage file when the Test task starts
  • pass append = true to the JaCoCo agent

We would need some test coverage making sure that parallel test execution works well with the Jacoco plugin and append = true.

When this works, we would set append = false in Gradle 5.0 and thereby enable caching of Test tasks with JaCoCo coverage by default.

@big-guy
Copy link
Member

big-guy commented May 15, 2018

@wolfs If we're making a breaking change in 5.0, we may also just want to remove configuration options that don't make sense or make it difficult to do the right thing.

Even though there's a JacocoMerge task that can take multiple JaCoCo reports and generate a single execution file, some builds accomplish the same thing by using the same output file. This could be from multiple Test tasks or multiple executions of the same Test task with different parameters.

@JLLeitschuh
Copy link
Contributor Author

JLLeitschuh commented May 15, 2018

@wolfs What about simply having each worker generate their own JaCoCo report?
As the Jacoco task is today, it already supports merging multiple JaCoCo execution report outputs into one.

We do that here at Plexxi to create our uber coverage report from all of our sub-projects.

val jacocoRootReport = task<JacocoReport>("jacocoRootReport") {
    group = LifecycleBasePlugin.VERIFICATION_GROUP
    description = "Generates code coverage report for all sub-projects."

    val jacocoReportTasks =
        subprojects
            .filter {
                // Filter out source sets that don't have tests in them
                // Otherwise, Jacoco tries to generate coverage data for tests that don't exist
                !it.java.sourceSets["test"].allSource.isEmpty
            }
            .map { it.tasks[jacocoTestResultTaskName] as JacocoReport }
    dependsOn(jacocoReportTasks)

    val allExecutionData = jacocoReportTasks.map { it.executionData }
    executionData(*allExecutionData.toTypedArray())

    // Pre-initialize these to empty collections to prevent NPE on += call below.
    additionalSourceDirs = files()
    sourceDirectories = files()
    classDirectories = files()

    subprojects.forEach { testedProject ->
        val sourceSets = testedProject.java.sourceSets
        this@task.additionalSourceDirs = this@task.additionalSourceDirs?.plus(files(sourceSets["main"].allSource.srcDirs))
        this@task.sourceDirectories += files(sourceSets["main"].allSource.srcDirs)
        this@task.classDirectories += files(sourceSets["main"].output)
    }

    reports {
        html.isEnabled = true
        xml.isEnabled = true
        csv.isEnabled = false
    }
}

@JLLeitschuh
Copy link
Contributor Author

Is there a release/5.0 branch where a PR of this nature could be pointed?

@wolfs
Copy link
Member

wolfs commented May 15, 2018

I did some digging and it seems like the JaCoCo agent locks the exec file before writing to it. It writes the execution data to the file when the JVM shuts down. See jacoco/jacoco#52.

As a consequence, I think the plan I described above would be good. We could also deprecate setting append to true with 5.0, pointing to the solution to create a report from different execution data files.

@big-guy Do you suggest removing the append option altogether in 5.0? We cannot do a real deprecation cycle given that we would prompt users setting append = false to enable caching now that they are using a deprecated method...

@JLLeitschuh There is no 5.0 branch but I think the changes I described above can be implemented in master. That would not be a change in behaviour given that append = false deletes the output file anyway. 5.0 would then only be about setting append = false per default.

@JLLeitschuh
Copy link
Contributor Author

@big-guy @w4tson The implications of this bug are a bit wider reaching than originally captured by this bug.

Gathering from what you've stated here, it seems that if you choose to have multiple workers for your tests, and you want to capture Jacoco code-coverage data, there is no way to also get build cache support.

So your options are, build cache support, or tests on multiple workers, but not both.

Does this merit another issue being opened as this implication seems to be of slightly wider concern than this issue originally tried to capture?

@wolfs wolfs changed the title Adding the Jacoco plugin with default config breaks Build Cache support for Test task Support the build cache and parallel test execution with the JaCoCo plugin applied May 20, 2018
@wolfs
Copy link
Member

wolfs commented May 20, 2018

@JLLeitschuh You are exactly right.

When using the java and jacoco plugins, users have the complicated and unclear choice of...

  • Accepting the defaults and not being able to cache their Test execution at all.
  • Reconfiguring jacoco.append=false so that Test will be cacheable. But this comes at the cost of not using parallel test execution.

It would be better if someone could have jacoco analysis, parallel test execution, Test cacheability -- all by default or easily configurable.

I wouldn't open a new issue but change the name of the current issue. I gave the rename a shot. WDYT?

@JLLeitschuh
Copy link
Contributor Author

I think that the new names captures it quite well, I updated the top comment on the issue to link to your comment @wolfs.
Thanks!

@JLLeitschuh
Copy link
Contributor Author

@wolfs Unfortunately, I don't think I'm going to have the time to fix this myself. Is there any possibility that someone from the team will get the chance to tackle this before 5.0?

@cal101
Copy link

cal101 commented Jun 19, 2018

Hi!

Talking about gradle 4.7:

From reading the above I got the expression that

  1. jacoco with append left alone (I expect the default "true" to be used)
    1. parallel test execution: maxParallelForks = 4

should work if I don't care for the test cache, right?
Thats because "append=true" disables the test cache, right?

How can I verify that the test cache is disabled?
My problem is that running jacoco with parallel forks is highly unreliable.
More often than not a lot of tests are not executed.
If you tell me that no known issues are open regarding that setting I can start seriously locating the issue.
Up to now I thought it to be an unsupported combination.

Is writing to different report files per agent supported yet?
Or is that something to be done?

Cal

@wolfs
Copy link
Member

wolfs commented Jun 19, 2018

According to this issue append = true should work.

The build cache is disabled if you didn't explicitly enable it and it will be disabled for all Test tasks with the JaCoCo agent configured with append=true.

I don't know about any issue regarding using parallel forks and append = true.

Writing different report files per test worker is not supported, yet.

The build cache shouldn't have any effect on whether tests are executed or not. Are you talking about single tests within the same Test task or whole Test tasks not being executed?

@cal101
Copy link

cal101 commented Jun 19, 2018

Thanks for your answer.

It's a test job of about 8000 tests. Last time less then 2000 tests were executed. So whole testsuites in junit jargon are missing.

@wolfs
Copy link
Member

wolfs commented Jun 20, 2018

@cal101 That sounds like a bug. Could you open another issue with some instructions how to reproduce the problem?

@cal101
Copy link

cal101 commented Jun 20, 2018

@wolfs I will try to make a reproducible case but that's difficult. It's on a closed source legacy project. It may be easier for me trying to debug the issue. I will start trying to debug.

wolfs added a commit that referenced this issue Aug 17, 2018
Jacoco code coverage should work well with the build cache out of the
box. Since appending to a coverage file works with parallel test
execution, see jacoco/jacoco#52, we set
`append=true` and delete the coverage data just before the test task
starts to execute.

Note that this is a breaking change: separate tasks now cannot use the
same coverage file, since each of the tasks will delete it.

Issue: #5269
@wolfs wolfs added this to the 5.0 RC1 milestone Aug 24, 2018
@wolfs wolfs self-assigned this Aug 24, 2018
@wolfs
Copy link
Member

wolfs commented Aug 24, 2018

Fixed via #6419.

@JLLeitschuh
Copy link
Contributor Author

@wolfs Thank you for fixing this!!! Is append now simply ignored?

vpbhargav added a commit to vpbhargav/jabref that referenced this issue Oct 12, 2019
Jacoco append property is deprecated starting gradle 5.0.
From my understandind the propery is currently ignored.
[Issue Ref](gradle/gradle#5269)

Signed-off-by: Bhargava Varadharajan <[email protected]>
@rawasthi11
Copy link

Hi Every one ,
I have a scenario, where i want to execute multiple test cases on same code in parallel , issue in this if i have execute multiples test cases at a same , generated exec file don't have proper mapping of test case with java class.(as in background other java files also get loaded which are evoked by other test cases).

i want for one test case , only single exec file is generated having mapping of java classes which are touched by this tests case only. (this is possible , if i execute test cases in serial). In order to save time, i want to execute multiple test cases at same time, and in exec only those java classes covered which is touched by test case specific to it.

Is there any way to achieve this ?

sschuberth added a commit to oss-review-toolkit/ort that referenced this issue Feb 20, 2022
The work-around introduced in b182fc3 is not required anymore as of
Gradle 5.0, see [1].

[1]: gradle/gradle#5269

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit to oss-review-toolkit/ort that referenced this issue Feb 21, 2022
The work-around introduced in b182fc3 is not required anymore as of
Gradle 5.0, see [1].

[1]: gradle/gradle#5269

Signed-off-by: Sebastian Schuberth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants