Implemented Kover Maven Plugin #654
Conversation
Resolves #51
There was a problem hiding this comment.
In general, LGTM. I will take a one more round of more close look later.
Do you have plans to extend test coverage? Not all possible configuration keys are covered, for example HtmReportMojo.title.
Am I right that the parameters you've exposed as parameters are the ones that are conventionally have global single value and most often are changed?
kover-maven-plugin/build.gradle.kts
Outdated
| extensions.configure<Kover_docs_conventions_gradle.KoverDocsExtension> { | ||
| docsDirectory.set("maven-plugin") | ||
| description.set("Kover Maven Plugin") | ||
| callDokkaHtml.set(false) | ||
| } |
There was a problem hiding this comment.
Why not like this? 🤔
| extensions.configure<Kover_docs_conventions_gradle.KoverDocsExtension> { | |
| docsDirectory.set("maven-plugin") | |
| description.set("Kover Maven Plugin") | |
| callDokkaHtml.set(false) | |
| } | |
| koverDocs { | |
| docsDirectory.set("maven-plugin") | |
| description.set("Kover Maven Plugin") | |
| callDokkaHtml.set(false) | |
| } |
There was a problem hiding this comment.
Perhaps, once upon a time, accessors were not generated for this and I copy the same everywhere
| * Binary reports that built in advance, before the start of the project build. | ||
| * | ||
| * It's convenient to use when tests are performed on CI on different nodes. |
There was a problem hiding this comment.
It's not clear by looking only at this doc, how it will be used and why I should provide it. Shall I review the KDocs now or for now you've put them mostly for yourself and going to improve in the future?
There was a problem hiding this comment.
It is worth at least a cursory read KDoc.
Additional files are provided only when necessary. That is, you need to merge the coverage with tests that are run on different machines.
Since the XML or HTML report cannot be merged, only ic files can be used for merging.
| lateinit var binaryReportFile: File | ||
|
|
||
| override fun doExecute() { | ||
| val propertyName = agentPropertyName ?: Constants.AGENT_ARG_PARAMETER |
There was a problem hiding this comment.
Elvis does not have effect on lateinit vars. Access to uninitialized var will throw a kotlin.UninitializedPropertyAccessException.
I see you handle this differently in your code, e.g. IcReportMojo.icFile is not checked at all
or AbstractCoverageTaskMojo.aggregate is not a lateinit var at all
There was a problem hiding this comment.
My bad, it's a dead code
| tasks.collectRepository.get().repositories.forEach { repository -> | ||
| repository.copyRecursively(dir) | ||
| } |
There was a problem hiding this comment.
This looks like a hack that may stop working in the future. It's also incompatible with configuration cache
There was a problem hiding this comment.
Yes, I think in another PR it is necessary to redo the work with local repositories
Right, this was the basic idea, the single values that are most often same for all goals. |
Added extra tests. |
kover-maven-plugin/docs/index.md
Outdated
|
|
||
| ## Current limitations | ||
| - instrumentation of only tests in `test` goal is supported, `it-tests` tests are not supported yet | ||
| - if several Kover JVM agents are specified when running the tests, then only the first one will work |
There was a problem hiding this comment.
if several Kover JVM agents are specified
Is it even possible to configure such thing?
There was a problem hiding this comment.
Yes, now if you use site lifecycle, then initialization happens twice
There was a problem hiding this comment.
I think an example or detailed explanation is in order so users won't accidentally create such a configuration.
There was a problem hiding this comment.
Example is in site directory :)
In case of site lifecycle, it will not be possible to avoid such a configuration.
kover-maven-plugin/src/main/kotlin/kotlinx/kover/maven/plugin/mojo/HtmReportMojo.kt
Outdated
Show resolved
Hide resolved
kover-maven-plugin/src/main/kotlin/kotlinx/kover/maven/plugin/mojo/VerifyMojo.kt
Show resolved
Hide resolved
Co-authored-by: Leonid Startsev <sandwwraith@users.noreply.github.com>
Co-authored-by: Leonid Startsev <sandwwraith@users.noreply.github.com>
Co-authored-by: Leonid Startsev <sandwwraith@users.noreply.github.com>
Resolves #51