Kotlin / kotlinx-kover

Apache License 2.0
1.28k stars 48 forks source link

Multiple issues when migrating from custom plugin to kotlinx-kover #398

Closed realdadfish closed 1 year ago

realdadfish commented 1 year ago

Describe the bug I have had a custom build convention plugin with which I set up code coverage for my Gradle apps. Upon getting the news that kotlinx-kover exists I decided to replace my custom implementation with one that internally configures / uses kotlinx-kover.

Upon this I encountered several issues:

  1. In Android projects, the non-variant-aware tasks koverVerify, koverHtmlReport, ... are created, though they're useless and result in being skipped when executed, because there are no tests executed (naturally):

    > Task :mymodule:koverHtmlReport SKIPPED
    Task 'koverHtmlReport' will be skipped because no tests were executed

    I think it would be better if the creation of these tasks would either be skipped or if they would act as aggegration tasks, i.e. koverHtmlReport would execute all koverHtmlReport tasks of all active / configured build variants

  2. When I look at the HTML report after all tests have been executed and coverage has been calculated, the Jacoco report tells me only 22% of my example module are covered, whereas previously I had more than 80% coverage. When I drill down into the details I see that especially Robolectric-based UI tests get no coverage reported at all. Are there any special settings needed to get the coverage for those tests as well?

  3. koverXmlReport / koverVerify seems to stall indefinitely. When I connect with the Gradle debugger I see that there are dozens of "Unconstrained build operation" threads that are all in parked state. In smaller modules it eventually seems to finish the tasks after some time, but then the configured thresholds (upper and lower) don't seem to let the build fail; instead the verification task always runs green and is even cached afterwards when I try to execute it again.

This is my configuration (stripped to the basic minimum):

import com.android.build.gradle.api.AndroidBasePlugin
import kotlinx.kover.gradle.plugin.dsl.AggregationType
import kotlinx.kover.gradle.plugin.dsl.GroupingEntityType
import kotlinx.kover.gradle.plugin.dsl.MetricType

plugins {
    id("org.jetbrains.kotlinx.kover")
}

val coverageExtension = project.extensions.create(
    "codeCoverage", CoverageExtension::class.java
)

plugins.withId("org.jetbrains.kotlin.jvm") {
    configureProject(coverageExtension)
}
plugins.withType(AndroidBasePlugin::class.java) {
    configureProject(coverageExtension)
    configureAndroidProject(coverageExtension)
}

fun configureProject(extension: CoverageExtension) {
    kover {
        useJacoco("0.8.8")
    }
    koverReport {
        filters {
            excludes {
                classes(extension.classFilter)
            }
        }
        defaults {
            verify {
                onCheck = true
                rule {
                    isEnabled = true
                    entity = GroupingEntityType.APPLICATION
                    bound {
                        minValue = extension.minCoveragePercentage
                        maxValue = extension.maxCoveragePercentage
                        metric = MetricType.LINE
                        aggregation = AggregationType.COVERED_PERCENTAGE
                    }
                }
            }
        }
    }
}

fun configureAndroidProject(extension: CoverageExtension) {
    koverReport {
        androidReports(extension.androidVariant) {}
    }
}

// alias for bc
tasks.register("createCoverageReport") {
    dependsOn(tasks.matching { it.name.startsWith("koverXmlReport") })
    dependsOn(tasks.matching { it.name.startsWith("koverHtmlReport") })
}

tasks.register("checkCodeCoverage") {
    dependsOn(tasks.matching { it.name.startsWith("koverVerify") })
}

This is CoverageExtension.kt:


open class CoverageExtension {
    /**
     * The name of the Android variant / source set to use for coverage calculation
     *
     * Note that only a single variant can be configured here, because multiple variants might
     * share common classes that will otherwise cause conflicts when eventually merging the
     * coverage results.
     */
    var androidVariant: String = "release"

    /**
     * Expected module coverage, never decrease, only increase!
     */
    var expectedCoverage: Float = 0.80f

    /**
     * Coverage threshold in which a calculated coverage value is valid
     * minCoverage = expectedCoverage - minCoverageThreshold
     */
    var minCoverageThreshold: Float = 0.01f

    /**
     * coverage threshold in which a calculated coverage value is valid
     * maxCoverage = expectedCoverage + maxCoverageThreshold
     */
    var maxCoverageThreshold: Float = 0.10f

    /**
     *  Jacoco works with class (bytecode) filters, so we filter out things that we can't / don't want to cover
     *
     *  Add your own filters via
     *  ```
     *  classFilter += listOf("MyClass.*")
     *  ```
     */
    var classFilter: List<String> = listOf(
        // Generated Android classes
        "**/R.class",
        "**/BR.class",
        "**/R$*.class",
        "**/BuildConfig.*",
        "**/Manifest*.*",
        "**/*Test*.*",
        // Generated data binding classes
        "**/databinding/*Binding*.class",
        "**/DataBinderMapperImpl.class",
        // Generated room classes
        "**/*_Impl.class",
        "**/*_Impl$*.class",
        // Generated Navigation library classes
        "**/*Args.class",
        "**/*Args\$Companion.class",
        "**/*Directions.class",
        "**/*Directions\$Companion.class",
        // Generated Dagger classes and provisions
        "**/*Module.*",
        "**/generated/**/*.*",
        "**/*_MembersInjector.*",
        "**/Dagger*",
        "**/Hilt_*",
        "**/hilt_aggregated_deps/**/*.*",
        "**/*_Factory.*",
        "**/*Factory.*",
        "**/*Module$*.*",
        // Other classes
        "**/*JsonAdapter.*"
    )

    internal val minCoveragePercentage: Int
        get() = (max(0f, expectedCoverage - minCoverageThreshold) * 100).toInt()

    internal val maxCoveragePercentage: Int
        get() = (min(1f, expectedCoverage + maxCoverageThreshold) * 100).toInt()
}

This is the configuration of an example module that reported around 90% coverage:

plugins {
   id("my.coverage.plugin")
}

// ...

codeCoverage {
    expectedCoverage = 0.90f
    minCoverageThreshold = 0.03f
    maxCoverageThreshold = 0.02f
}

Environment

realdadfish commented 1 year ago

I figured that this seemed to be the hot path for issue 3, when I print "$file" in the debugger I see that the project's class files are printed multiple times (like 20 or 30 times), not just once.

Bildschirmfoto 2023-06-02 um 11 34 38
shanshin commented 1 year ago

I figured that this seemed to be the hot path for issue 3, when I print "$file" in the debugger I see that the project's class files are printed multiple times (like 20 or 30 times), not just once.

Hi, unfortunately, filtering when working with JaCoCo can be organized only in this way. This line will be executed frequently because it is called for each class file.

However, the filtering code should not work very long.

shanshin commented 1 year ago

In Android projects, the non-variant-aware tasks koverVerify, koverHtmlReport, ... are created, though they're useless and result in being skipped when executed, because there are no tests executed (naturally):

This is the expected behavior, Kover default tasks are always created.

For Android, they are empty so that the user himself can add a report of interesting build variants to them with mergeWith(...).

In general, it can take a long time to build a report for all build variants and often causes errors, because some classes with the same name may be duplicated in different variants.

realdadfish commented 1 year ago

I figured that this seemed to be the hot path for issue 3, when I print "$file" in the debugger I see that the project's class files are printed multiple times (like 20 or 30 times), not just once.

Hi, unfortunately, filtering when working with JaCoCo can be organized only in this way. This line will be executed frequently because it is called for each class file.

However, the filtering code should not work very long.

I'm checking the execution with --profile now to see if there is anything noticable.

shanshin commented 1 year ago

When I drill down into the details I see that especially Robolectric-based UI tests get no coverage reported at all. Are there any special settings needed to get the coverage for those tests as well?

At the moment, tests executed on Android devices or in the emulator are not supported.

realdadfish commented 1 year ago

In general, it can take a long time to build a report for all build variants and often causes errors, because some classes with the same name may be duplicated in different variants.

I have only a single build variant, release, for all but my app module, for performance reasons.

realdadfish commented 1 year ago

When I drill down into the details I see that especially Robolectric-based UI tests get no coverage reported at all. Are there any special settings needed to get the coverage for those tests as well?

At the moment, tests executed on Android devices or in the emulator are not supported.

No, these are pure JVM tests, Robolectric does not need a device or emulator. Still, it is noticable that even though the JVM UI tests run, I get no coverage reported for any UI-related classes.

shanshin commented 1 year ago

I have only a single build variant, release, for all but my app module, for performance reasons.

For your case, you may specify mergeWith("release"), (like in example)[https://kotlin.github.io/kotlinx-kover/gradle-plugin/#kotlin-multiplatform-project]

realdadfish commented 1 year ago

I'm a bit reluctant to switch to kover instrumentation / coverage, because a) I have a partially mixed source with legacy code being written in Java (and IIRC this is only supported with Jacoco, right?) and b) the coverage results will probably vary quite a bit when the tool is changed (at least I got completely different coverage values when I switched between IntelliJ coverage and Jacoco in the IDE in the past), but I guess I'll try anyway.

The reason why I also stick to Jacoco 0.8.8 is because I also saw coverage deviations between different Jacoco versions and for now I kind of want a "reproducible" coverage result. But I'll also try 0.8.10 which is the newest IIRC.

shanshin commented 1 year ago

Is your project open source ? A small project reproducer would help to deal with the slowdown

realdadfish commented 1 year ago

I'm checking the execution with --profile now to see if there is anything noticable.

Ok, this has very low resolution, it just tells me that the koverVerifyRelease task executed for 6m17s out of 7m16s total (cached test execution).

realdadfish commented 1 year ago

Is your project open source ? A small project reproducer would help to deal with the slowdown

Unfortunately not, all closed source. Have you experience with https://github.com/gradle/gradle-profiler-plugin, would this be helpful to create profile data with this?

shanshin commented 1 year ago

Ok, this has very low resolution, it just tells me that the koverVerifyRelease task executed for 6m17s out of 7m16s total (cached test execution).

A large number of classes in the project?

Could you remove the useJacoco("0.8.8") and compare how long the validation will take using Kover tool?

realdadfish commented 1 year ago

A large number of classes in the project?

find . -name "*.class" | grep -v Test | grep -v test | wc -l tells me 2725. It's one of the bigger modules, but not huge.

Could you remove the useJacoco("0.8.8") and compare how long the validation will take using Kover tool?

Yes, running now.

shanshin commented 1 year ago

Have you experience with https://github.com/gradle/gradle-profiler-plugin, would this be helpful to create profile data with this?

Not yet, but it will be useful to understand whether the slowdown occurs during filtering or during validation by JaCoCo itself.

For comparison, you may try to remove all report filters and compare whether the build time will speed up (in this case, the code from here will not be executed).

realdadfish commented 1 year ago

Could you remove the useJacoco("0.8.8") and compare how long the validation will take using Kover tool?

Yes, running now.

koverVerifyRelease with Kover instead of Jacoco only took 2,7s. Trying without class filters now.

realdadfish commented 1 year ago

koverVerifyRelease with Kover instead of Jacoco only took 2,7s. Trying without class filters now.

The same task with Jacoco, but without class filters, took 1,9s, so the filters are definitely the culprit.

shanshin commented 1 year ago

How many class-files do you have in the project?

realdadfish commented 1 year ago

FYI - koverReport { defaults { mergeWith("release") } androidReports("release") {} } needed to be wrapped into project.afterEvaluate {} because of #362. This was not needed when I just configured koverReport { androidReports("release") {} }.

Also, with the configured mergeWith above I still get

> Task :my-module:koverHtmlReport SKIPPED
Task 'koverHtmlReport' will be skipped because no tests were executed

so this does not seem to work either. So I skip this for now and only call / work with the Android variants of the specific tasks for now.

realdadfish commented 1 year ago

How many class-files do you have in the project?

See above, about 2700 prod class files in the build/ directory. Because this is Kotlin code many of these are lambdas, inlines, etc.

realdadfish commented 1 year ago

The creation of the Kover (non Jacoco) HTML report took almost 40s, not exactly fast either. What's good is that this reports a somewhat correct line coverage of 81% for the module. Will check the verification task now.

shanshin commented 1 year ago

The creation of the Kover (non Jacoco) HTML report took almost 40s, not exactly fast either.

To generate the report, Kover analyzes all class-files and all sources, so it can work slowly on large projects.

realdadfish commented 1 year ago

Also, I configured a minValue of 47 and a maxValue of 52, but when I execute koverVerify on the said module that has a line coverage of 81%, the task does not fail. Isn't it supposed to do that because the upper limit is hit?

shanshin commented 1 year ago

Isn't it supposed to do that because the upper limit is hit?

Yes, verification should fail.

For more info, please add rule minValue=100 and maxValue=0 to see verification error

realdadfish commented 1 year ago

Will do that.

Btw... thanks for your / JetBrains work on this, even if it is not yet perfect. A proper coverage solution for JVM/Android/multiplatform projects was long overdue. So thanks again, also for your instant support!

realdadfish commented 1 year ago

Hah, nice that my custom ShowPackageCoverage task still works - apparently kover is reusing the Jacoco XML format!

> Task :my-module:showPackageCoverageRelease
my.app.mymodule.databinding: 74,89%
                                      .di: 51,20%
                                      .domain.filter: 63,77%
                                             .messagenotification: 100,00%
                                             .model: 99,71%
                                      Sum: 85,25%
                                      .io.db.converter: 96,15%
                                            .migrations: 100,00%
                                            .model: 100,00%
                                         Sum: 68,20%
                                         .mqtt.model: 57,91%
                                      Sum: 63,41%
                                      .tech: 71,15%
                                      .view.messagenotification: 100,00%
                                           .screens.actionchippanel.internal: 100,00%
                                                                   .mapper: 100,00%
                                                                   .model: 100,00%
                                                   Sum: 92,16%
                                                   .bla.model: 100,00%
                                                                .state: 88,41%
                                                   Sum: 90,57%
                                                   .common.mapper: 88,78%
                                                          .model: 91,32%
                                                   Sum: 82,42%
                                                   .main.model: 93,48%
                                                   .start.model: 80,00%
                                                   .foo.checkin.mapper: 100,00%
                                                                           .bar.model: 89,74%
                                                                           .baz.model: 99,25%
                                                                   Sum: 87,86%
                                                                   .mapper: 83,56%
                                                                   .model: 100,00%
                                                   Sum: 86,84%
                                                   .topbarinformation.internal: 100,00%
                                                   .bazz.internal: 76,53%
                                                                .model: 100,00%
                                                   Sum: 87,76%
                                                   .buzz.mapping: 100,00%
                                                           .model: 100,00%
                                                           .fizz.internal: 100,00%
                                                                              .model: 100,00%
                                                           Sum: 87,04%
                                                           .state: 96,67%
                                                   Sum: 87,58%
                                                   .bazz.model: 100,00%
                                           Sum: 87,84%
                                      Sum: 88,04%
                              Sum: 82,21%
hilt_aggregated_deps: No coverage
realdadfish commented 1 year ago

Last post from me here today (will be back on Monday), but I can't get verification to work. I execute it like this

gradle -p my-component my-module:koverVerifyRelease --rerun --profile

and use the output of --profile to check if the task really runs (without --rerun it is marked as UP-TO-DATE, not sure thats right, because it does not output anything, and even though the inputs haven't changed I think it should rather always run when executed).

Problem is that the task succeeds, even with this configuration:

  koverReport {
        androidReports("release") {}
        filters {
            excludes {
                classes(/* my class filter */)
            }
        }
        defaults {
            verify {
                onCheck = true
                rule {
                    isEnabled = true
                    entity = GroupingEntityType.APPLICATION
                    bound {
                        minValue = 100
                        maxValue = 0
                        metric = MetricType.LINE
                        aggregation = AggregationType.COVERED_PERCENTAGE
                    }
                }
            }
        }
    }
shanshin commented 1 year ago

Hi, in your configuration, you configure verification for the task koverVerify. For Android projects, these tasks do nothing by default.

To configure validation for the release build variant, you must specify

koverReport {
        androidReports("release") {
            filters {
                excludes {
                    classes(/* my class filter */)
                }
            }
            verify {
                onCheck = true
                rule {
                    bound {
                        minValue = 100
                        maxValue = 0
                    }
                }
            }
        }
}
realdadfish commented 1 year ago

Ah I see, I thought this was common configuration that was shared.

realdadfish commented 1 year ago

I simply don't get it - it will just not execute the verification rule:

// since the plugin does not support lazy configuration and our coverage extension does neither, we need to
// wait until the module's build gradle file is evaluated and has the proper settings filled in
afterEvaluate {
    // We can't use Jacoco because of slow class filtering, see https://github.com/Kotlin/kotlinx-kover/issues/398
    // kover {
    //  useJacoco("0.8.8")
    // }
    plugins.withId("org.jetbrains.kotlin.jvm") {
        configureJvmProject(coverageExtension)
    }
    plugins.withType(AndroidBasePlugin::class.java) {
        configureAndroidProject(coverageExtension)
    }
}

fun configureJvmProject(extension: CoverageExtension) {
    koverReport {
        filters {
            configure(extension)
        }
        defaults {
            configure(extension)
        }
    }
}

fun configureAndroidProject(extension: CoverageExtension) {
    koverReport {
        androidReports(extension.androidVariant) {
            filters {
                configure(extension)
            }
            defaults {
                configure(extension)
            }
        }
    }
}

fun KoverDefaultReportsConfig.configure(extension: CoverageExtension) {
    verify {
        onCheck = true
        rule {
            isEnabled = true
            entity = GroupingEntityType.APPLICATION
            bound {
                minValue = 100 //extension.minCoveragePercentage
                maxValue = 0 // extension.maxCoveragePercentage
                metric = MetricType.LINE
                aggregation = AggregationType.COVERED_PERCENTAGE
            }
            filters {
                configure(extension)
            }
        }
    }
}

fun KoverReportFilters.configure(extension: CoverageExtension) {
    excludes {
        classes(extension.classFilter)
    }
}

I try to make a small example project now. Can't seem to get this working like this.

shanshin commented 1 year ago

Please, for checking, write exactly as in the example above (for example, for a variant release)

koverReport {
        androidReports("release") {
            filters {
                excludes {
                    classes(/* my class filter */)
                }
            }
            verify {
                onCheck = true
                rule {
                    bound {
                        minValue = 100
                        maxValue = 0
                    }
                }
            }
        }
}

and execute koverVerifyRelease.

Configuring inside afterEvaluate may lead to unpredictable behavior.

shanshin commented 1 year ago

It might be better to postpone the migration until issue #362 is fixed.

Current configuration issues complicate the stable reproduction of complex configuration cases.

realdadfish commented 1 year ago

The problem I'm facing right now is that I can't lazy-populate the plugin with settings from my build convention extension, without relying onafterEvaluate - and this breaks the plugin right now. I guess I'll postpone my migration here indeed, sadly. I've pushed my test project here: https://github.com/realdadfish/kover-issue-398/

It's also not entirely clear to me how the filters configurations on KoverReportsConfig, KoverVerifyRule, KoverReportExtension and KoverDefaultReportsConfig work altogether. There might be use cases why one wants all these different filter configurations everywhere, but at least for the novel user these all complicate matters a lot.

Thanks so far for your time and effort!

shanshin commented 1 year ago

It's also not entirely clear to me how the filters configurations on KoverReportsConfig, KoverVerifyRule, KoverReportExtension and KoverDefaultReportsConfig work altogether.

In most cases, it is enough to use

koverReport {
    filters {
        // these filters are applied to all reports and verifications for all build variants
    }
}

Only if individual filters are needed for any particular build variant, then these filters can be overridden:

koverReport {
    androidReports("specialBuildVariant") {
       filters {
           // the filters specified here will only apply to reports for variant `specialBuildVariant` built by  tasks `koverXmlReportSpecialBuildVariant`, `koverHtmlReportSpecialBuildVariant`, `koverVerifySpecialBuildVariant`
        }
    }
}

However, this is a very rare case.

All filters are inherited from the level above and can overload each other, see