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
update project to use kotlin-dsl plugin #223
Conversation
- update methods to use Gradle Kotlin DSL - use embeddedKotlinVersion to help align with Gradle embedded Kotlin version - add simple smoke test for testing if the plugin works
src/test/kotlin/KoverPluginTest.kt
Outdated
import kotlin.test.assertTrue | ||
|
||
|
||
class KoverPluginTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the existing tests pretty difficult to understand and unpick. I created this small test so I could more easily adjust the test to fetch the output build log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this test does not test the functionality of the plugin and is used for local debugging, I think it is better to leave it in the local branch.
In addition, it slightly smears the testing levels across two modules - in this project there are partial unit-tests, and not for the entire plugin, the dependency in it on gradleTestKit()
is redundant.
In addition, remember that the functional tests run by Gradle itself are not entirely honest, because there are minor differences in the environment and dependencies. Therefore, for verification, I recommend publishing to the local repository gradlew publishToMavenLocal
and already from mavenLocal()
use the plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It tests whether the Kover plugin can be applied, that's functionality :)
Therefore, for verification, I recommend publishing to the local repository
gradlew publishToMavenLocal
and already frommavenLocal()
use the plugin.
That's a lot more work than running a single unit test!
I found this test really useful when trying to track down why the other tests weren't working. The existing tests didn't give enough detail. I did try adjusting them, but I didn't really understand how they're set up and created. I find it much easier to see the build and settings files under test, and adjust the command and output log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each of the available functional tests checks that the plugin is being applied, a separate test for this is useless.
In any case, example project launches will be added soon (see #219), so there is no need for a separate such test, especially in another module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best way is to improve the error messages of the existing functional tests (adding the error log and the contents of the build script to the message, I will do it after merge #219), rather than adding a completely duplicate test.
settings.gradle.kts
Outdated
pluginManagement { | ||
val kotlinVersion: String by settings | ||
|
||
plugins { | ||
kotlin("jvm") version kotlinVersion | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is removed because the version is now provided by Gradle's embeddedKotlinVersion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to return the previous version, because Gradle uses old versions.
And if some bug is found in the compiler (for example
// TODO `onlyIf` block moved out from config lambda because of bug in Kotlin compiler - it implicitly adds closure on `Project` inside onlyIf's lambda |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted it but it does lead to this warning from Gradle
> Configure project :
WARNING: Unsupported Kotlin plugin version.
The `embedded-kotlin` and `kotlin-dsl` plugins rely on features of Kotlin `1.5.31` that might work differently than in the requested version `1.7.10`.
I've never seen any problems come out of this warning, so I think it can be ignored.
I lean more towards using the same version as Gradle, because at least the compiler bug are known problems. It's never been clear to me what the problems are if the project is compiled with a different version (even if the Kotlin language level is 1.4).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted it but it does lead to this warning from Gradle
> Configure project :
WARNING: Unsupported Kotlin plugin version.
The `embedded-kotlin` and `kotlin-dsl` plugins rely on features of Kotlin `1.5.31` that might work differently than in the requested version `1.7.10`.
I've never seen any problems come out of this warning, so I think it can be ignored.
I lean more towards using the same version as Gradle, because at least the compiler bug are known problems. It's never been clear to me what the problems are if the project is compiled with a different version (even if the Kotlin language level is 1.4).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, thanks!
This is an initial step in fixing #222 and #221, and improving compatibility with Gradle config cache