Skip to content
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

Merged
merged 4 commits into from Aug 27, 2022

Conversation

aSemy
Copy link
Contributor

@aSemy aSemy commented Aug 26, 2022

  • 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

This is an initial step in fixing #222 and #221, and improving compatibility with Gradle config cache

- 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
import kotlin.test.assertTrue


class KoverPluginTest {
Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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 from mavenLocal() 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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Comment on lines 3 to 9
pluginManagement {
val kotlinVersion: String by settings

plugins {
kotlin("jvm") version kotlinVersion
}
}
Copy link
Contributor Author

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

Copy link
Collaborator

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
), then we will have to wait a long time for the release of Gradle, which will use the expected Kotlin.

Copy link
Contributor Author

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).

Copy link
Contributor Author

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).

@shanshin shanshin marked this pull request as ready for review August 26, 2022 20:51
@shanshin shanshin self-requested a review August 27, 2022 09:55
Copy link
Collaborator

@shanshin shanshin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, thanks!

@shanshin shanshin merged commit 3e67be5 into Kotlin:main Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants