Kotlin / kotlinx-kover

Apache License 2.0
1.28k stars 48 forks source link

Implement Gradle configurations for sharing cross-project aggregation #226

Closed aSemy closed 1 year ago

aSemy commented 1 year ago

Here's an initial pass at implementing aggregation. This will improve compatibility with Gradle, including the configuration cache and avoiding workarounds like in #224.

When finished users will be able to manually define which projects to aggregate. For example

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

dependencies {
  koverAggregate(project(":services"))
  koverAggregate(project(":api"))
  koverAggregate(project(":model"))
}

Or Kover can set a default, equivalent to

dependencies {
  subprojects.forEach { sub -> koverAggregate(sub) }
}

Summary

The approach is based on https://docs.gradle.org/7.3/samples/sample_jvm_multi_project_with_code_coverage.html

Essentially

  1. created some 'producer' and 'consumer' Configurations for providing/fetching Kover source files, and and binary report files.
  2. Created a 'parent' consumer, koverAggregate, that the other two consumers will consume from.
  3. Created some 'variant attributes' for distinguishing between the different configurations.
  4. The appropriate tasks will either push artifacts into them, or consume artifacts from them. This eliminates the need for scanning projects for the Kover extension.

It still needs some work!

shanshin commented 1 year ago

Thanks for the great example! However, I propose to merge it in to separate branch so that I can finalize it, because now many API contracts are violated, project isolation is not fully implemented and these changes are incompatible with older versions of Gradle (at least 7.4 is required and Incubating features are used wich may be incompatible with Android)

aSemy commented 1 year ago

No problem, it's a good challenge.

What do you mean about API contracts being violated? Do you mean because of the changes I've made? Because the extensions are still the same, and although I commented out the code that implements some of the filters, that's only because it's WIP. Once finished, there wouldn't need to be any API changes. The incubating feature I removed, because it didn't work as expected.

However I've come across some bugs that make progressing quite difficult - particularly this one https://github.com/gradle/gradle/issues/6619. Basically adding an output to an extension doesn't work correctly. I think this might need to change, so the Test tasks no longer have an extension that provides output. Instead Kover can register a new task that processes the output of the Test tasks... but I'm just throwing out ideas.

shanshin commented 1 year ago

Yes, I also faced with an error https://github.com/gradle/gradle/issues/6619 , we cannot rely on its fixing, because the fix will only work with the most recent versions of Gradle.

What do you mean about API contracts being violated?

For example, there is a rather non-obvious case with binary reports:

  1. you add an explicit dependency in binaryReportFilesProducer to all Kover project tasks, which can lead to an unexpected launch of all of them when calling merge tasks.
  2. you fill in the artifacts for binaryReportFilesProducer with Files types (not providers) during its configuration, however, the settings can be changed after this moment (for example, in afterEvaluate) and these changes will not be visible in merge tasks.
  3. although filling in the files property for project tasks is not implemented now, however, if you fill in files from all test tasks without exception, the results of merge reports will begin to depend on the order of test execution (test tasks excluded in Kover can still be performed)

Also provider extensionByProject still violates the isolation of projects (refers internally to other projects). Since the main issue is to fully support the configuration caches, this solution is temporary and it is better to finalize it in a separate branch, because most likely it will be necessary to rewrite most of the code again.

aSemy commented 1 year ago
  1. you add an explicit dependency in binaryReportFilesProducer to all Kover project tasks, which can lead to an unexpected launch of all of them when calling merge tasks.

I don't understand why that is an API contract violation? How does the merge task work at the moment if it doesn't depend on the test tasks?

It's normal Gradle behaviour for a task that depends on other tasks to launch the other tasks.

  1. you fill in the artifacts for binaryReportFilesProducer with Files types (not providers) during its configuration, however, the settings can be changed after this moment (for example, in afterEvaluate) and these changes will not be visible in merge tasks.

binaryReportFilesProducer is created using register(), not create(), so it shouldn't be configured (and add artifacts) until it is consumed. It should only be consumed during the execution stage, not configuration.

This could be improved by creating a specific task for Kover to generate the binary report files, then that one specific task could register an output, and the configuration could depend on the task. That's what the Gradle JaCoCo example does:

configurations.create("coverageDataElements") {
    // This will cause the test task to run if the coverage data is requested by the aggregation task
    outgoing.artifact(tasks.test.map { task ->
        task.extensions.getByType<JacocoTaskExtension>().destinationFile!!
    })
}
  1. although filling in the files property for project tasks is not implemented now, however, if you fill in files from all test tasks without exception, the results of merge reports will begin to depend on the order of test execution (test tasks excluded in Kover can still be performed)

I don't understand... is this related to my previous comment about this being WIP and so I commented out functionality to make it compile?

shanshin commented 1 year ago

I don't understand why that is an API contract violation? How does the merge task work at the moment if it doesn't depend on the test tasks?

Regarding the Kover tasks themselves: when starting the merge report generation, the user may not expect that the project verification task will be executed, which may even fail and prevent the merge report from being generated.

Also if kover tasks will execute all test tasks: For example, the user does not want the results of all tests to get into the merge report and explicitly disables the collection of coverage for some test: to analyze the coverage of specific tests, or if all tests are performed for a very long time, but the author is sure that the excluded tests do not check the application itself. Another illustrative example is an Android application, where it is not always necessary to run tests for release and debug build types at the same time.

binaryReportFilesProducer is created using register(), not create(), so it shouldn't be configured (and add artifacts) until it is consumed. It should only be consumed during the execution stage, not configuration.

In any case, .configure { will be executed earlier than afterEvaluate, and if the author changes the location of the file with a binary report (in the current API it is KoverTaskExtension#reportFile), then it will not get into the artifact, and there will be the old value. As for the example, note that it is not the File that gets into the artifact, but the file provider, which can "monitor" changes even in afterEvaluate. In your PR, you resolve a file from the ConfigurableFileCollection right during configuration, which is too early.

I don't understand... is this related to my previous comment about this being WIP and so I commented out functionality to make it compile?

a friendly reminder that when you try to implement this, it is better to use providers with filters for disabled test-tasks inside )

aSemy commented 1 year ago

I'm not really following your feedback. I'll close this for now. I might delete my fork of the project, so make a copy if you'd like to use it later.

shanshin commented 1 year ago

I'm not really following your feedback.

I'm sorry for the misunderstanding.

The main requirements for merge tasks are that merge task should use the result of only user-allowed test tasks and runs only them.

In your PR, you used builtBy(task) which means that it is expected that all the Kover project tasks must be completed to get the artifact for merge task, which is redundant.

Because changing the settings of the Kover plugin can happen at any place, including in afterEvaluate, in all places it is better to use Provider<File> instead of File.

Code from JaCoCo

    outgoing.artifact(tasks.test.map { task ->
        task.extensions.getByType<JacocoTaskExtension>().destinationFile!!
    })

passes the provider to the artifact by tasks.test.map, thereby solving two problems:

so make a copy if you'd like to use it later.

Yes, I definitely use it with minor changes to support configuration caching.

Thank you again for contribution!

aSemy commented 1 year ago

I'm still not following... you're trying to explain things I didn't ask an explanation for. This was WIP so focusing on unfinished code isn't helpful.