Implemented protype of Kover Aggregated Plugin #644
Conversation
|
|
||
| htmlDir.convention(layout.buildDirectory.dir("reports/kover/html")) | ||
|
|
||
| this.onlyIf { |
There was a problem hiding this comment.
Is there a reason to use onlyIf for logging? Won't doFirst/doLast suit this purpose better?
There was a problem hiding this comment.
Because onlyIf always executed, even when the task does not start and the result is taken from the cache (FROM-CACHE outcome)
There was a problem hiding this comment.
It's worth plunking a comment describing it then
...lugin/src/main/kotlin/kotlinx/kover/gradle/aggregation/settings/KoverSettingsGradlePlugin.kt
Outdated
Show resolved
Hide resolved
|
|
||
| internal object KoverParametersProcessor { | ||
| fun process(settingsExtension: KoverSettingsExtensionImpl, providers: ProviderFactory) { | ||
| if (providers.gradleProperty("kover").isPresent) { |
There was a problem hiding this comment.
Does it make sense to check support values for this property? For instance, suppose someone has a project where coverage was enabled using enableCoverage DSL's call, but a user now want's to explicitly disable it via CLI by passing -Pkover=false.
There was a problem hiding this comment.
In this case, the users can write their own condition, like
kover {
if (providers.gradleProperty("useCoverage").toBoolean() != true) {
enableCoverage()
}
}:)
In general, this is a debatable question in DSL, whether it is worth giving the enable function enableCoverage() or property coverageIsEnabled
There was a problem hiding this comment.
the users can write their own condition
That requires committing some code to a repo, so if someone want to temporary disable code coverage collection in CI, it could be problematic.
There was a problem hiding this comment.
in this case, we can add only -Pkover=false, so that there is -Pkover to turn on and -Pkover=false to turn off
| settingsExtension.coverageIsEnabled.set(true) | ||
| } | ||
|
|
||
| settingsExtension.reports.includedProjects.readAppendableArgument(providers, "kover.classes.from") |
There was a problem hiding this comment.
I guess kover.classes.from should reads as "collect coverage for classes from following projects", but for me options like kover.classes.from.excludes and kover.classes.excludes are not easily distinguishable.
Did you consider naming package-related options as kover.packages.includes and kover.packaged.excludes?
There was a problem hiding this comment.
The names are not final at all and this is the first thing that came to mind.
In any case, you can come up with a lot of spelling options and they are all likely to be similar to each other in one way or another (
There will most likely be no packages - this filter is duplicated classes, and instead of kover.classes.from, we can try kover.projects and kover.projects.excludes
...plugin/src/main/kotlin/kotlinx/kover/gradle/aggregation/settings/KoverParametersProcessor.kt
Outdated
Show resolved
Hide resolved
...-plugin/src/main/kotlin/kotlinx/kover/gradle/aggregation/project/KoverProjectGradlePlugin.kt
Outdated
Show resolved
Hide resolved
kover-gradle-plugin/src/main/kotlin/kotlinx/kover/gradle/aggregation/commons/utils/Dynamic.kt
Outdated
Show resolved
Hide resolved
...-plugin/src/main/kotlin/kotlinx/kover/gradle/aggregation/project/KoverProjectGradlePlugin.kt
Show resolved
Hide resolved
| To use the plugin, just add into a `settings.gradle.kts` file | ||
| ```kotlin | ||
| plugins { | ||
| id("org.jetbrains.kotlinx.kover.settings") version "0.8.1" |
There was a problem hiding this comment.
I'm not sure that kover.settings conveys the meaning of the plugin. Yes, it is applied in settings, but its goals are 1) reduce configuration 2) instrument only test tasks that are currently run. We may want to eventually rename it, but I don't have any good name right now except kover.onthefly
There was a problem hiding this comment.
My option is org.jetbrains.kotlinx.kover.aggregation - because the feature of this plugin is that it can aggregate coverage from all subprojects
There was a problem hiding this comment.
Given that we have merge in regular setup which also does some kind of aggregation and various AggregationTypes, I'm afraid that this word is already overused :(
There was a problem hiding this comment.
In fact, this plugin has nothing to do with old Kover plugin, but it does exactly what merge says - it creates a aggregated reports
There was a problem hiding this comment.
Well, if I run ./gradlew :projectA:jvmTest -Pkover koverHtmlReport, it will show me only projectA's coverage, in that case there's no aggregation
There was a problem hiding this comment.
Not quite. :projectA may depends on :projectB, thus the sources from these two projects will be aggregated in the report
There was a problem hiding this comment.
I can live with aggregation for now, maybe we'll find better name later
| excludedProjects.add(":b") | ||
|
|
||
| // -Pkover.classes.includes=classes.to.exclude.* | ||
| includedClasses.add("classes.to.include.*") |
There was a problem hiding this comment.
I believe there should also be inclusions/exclusions by annotation
|
|
||
| override val reports: ReportsSettings = objects.newInstance<ReportsSettings>() | ||
|
|
||
| // abstract override val variants: NamedDomainObjectContainer<VariantI> |
|
|
||
| package kotlinx.kover.gradle.aggregation.settings.dsl.intern | ||
|
|
||
| internal abstract class KoverProjectExtensionImpl { |
There was a problem hiding this comment.
This class seems unnecessary
kover-gradle-plugin/build.gradle.kts
Outdated
| create("KoverSettingsPlugin") { | ||
| id = "org.jetbrains.kotlinx.kover.settings" | ||
| implementationClass = "kotlinx.kover.gradle.aggregation.settings.KoverSettingsGradlePlugin" | ||
| displayName = "Gradle Plugin for Kotlin Code Coverage Tools" |
There was a problem hiding this comment.
It has the same displayName and description as a regular plugin. Won't it be confusing?
There was a problem hiding this comment.
I changed it a little
Relates #608