Migrate integration-testing module from Groovy to Kts #4341
Conversation
There was a problem hiding this comment.
With the proposed changes, I get the following Gradle error (for example, when invoking ./gradle tasks in the integration-testing directory):
Build file `kotlinx.coroutines/integration-testing/build.gradle.kts' line: 34
'kotlin_snapshot_version' should be defined when building with snapshot compiler
On develop, everything is okay, so there's a clear regression.
When I look at build.gradle and build.gradle.kts, there's a clear change in logic.
Such mistakes will be easier to avoid (and if they do happen, to pinpoint) if the refactoring is split into several commits:
- Rename
*.gradleinto*.gradle.kts(the build is expected to fail due to the incorrect syntax). - Rewrite the syntax to be correct, but stay as faithful as possible to the original (so that all the changes are trivial).
- Introduce the extra structure if you want.
This is just a suggestion, though. I'll accept this PR as is when it's fixed, as it's quite small and easy to verify with any approach.
DmitryTsyvtsyn
force-pushed
the
migrate-integration-testing-to-kts
branch
2 times, most recently
from
50d6d83 to
c7f7463
Compare
|
I updated the pull request |
There was a problem hiding this comment.
Other than the remaining change in behavior, I think this is good to go. There are still minor things that could be improved in terms of style. Would you like me to list them, or should we merge this PR, and then I'll put my fixes on top of it in a separate PR myself?
integration-testing/build.gradle.kts
Outdated
| } | ||
|
|
||
| plugins { | ||
| id("org.jetbrains.kotlin.jvm") version "2.0.0" |
There was a problem hiding this comment.
2.0.0 is incorrect, it must be the configured Kotlin version.
Give me a list of minor things, I'll correct it along with the Kotlin version |
integration-testing/build.gradle.kts
Outdated
| val kotlinVersion = fetchKotlinVersion() | ||
|
|
||
| extra.apply { | ||
| set("native_targets_enabled", isEnabledNativeTargets) |
There was a problem hiding this comment.
This variable is unused, so no need to set or check it.
integration-testing/build.gradle.kts
Outdated
| val isEnabledNativeTargets = rootProject.properties["disable_native_targets"] == null | ||
| val usingSnapshotVersion = checkIsSnapshotVersion() | ||
| val hasSnapshotTrainProperty = checkIsSnapshotTrainProperty() | ||
| val kotlinVersion = fetchKotlinVersion() |
There was a problem hiding this comment.
This variable is only used to set kotlin_version, which is only used in this build script. Both fetchKotlinVersion and this val can be moved outside the buildScript block.
There was a problem hiding this comment.
fetchKotlinVersion() depends on different functions
There was a problem hiding this comment.
It won't work, I checked
There was a problem hiding this comment.
The result of the function being called is available in extra["build_snapshot_train"].
There was a problem hiding this comment.
If you set extra outside the buildscript block in the buildscript block you won't get the value( I checked
integration-testing/build.gradle.kts
Outdated
| } | ||
| } | ||
|
|
||
| tasks.create<Test>("jvmCoreTest") { |
There was a problem hiding this comment.
By giving a name to this, you can refer to the newly-created task without using the "jvmCoreTest" string below. The same goes for the other tasks.
integration-testing/build.gradle.kts
Outdated
| } | ||
| } | ||
|
|
||
| tasks.named<KotlinCompile>("compileDebugAgentTestKotlin") { |
There was a problem hiding this comment.
Starting from this line, all of this could be put into a tasks block, so that there's no need to repeat tasks. everywhere.
integration-testing/build.gradle.kts
Outdated
| } | ||
|
|
||
| // Drop this when node js version become stable | ||
| tasks.withType(org.jetbrains.kotlin.gradle.targets.js.npm.tasks.KotlinNpmInstallTask::class.java).configureEach { |
There was a problem hiding this comment.
| tasks.withType(org.jetbrains.kotlin.gradle.targets.js.npm.tasks.KotlinNpmInstallTask::class.java).configureEach { | |
| tasks.withType<org.jetbrains.kotlin.gradle.targets.js.npm.tasks.KotlinNpmInstallTask>().configureEach { |
By importing the class name, the code becomes even more readable. The same goes for all the other tasks.withType( invocations.
integration-testing/build.gradle.kts
Outdated
| } | ||
|
|
||
| tasks.withType(org.jetbrains.kotlin.gradle.tasks.KotlinJvmCompile::class.java).configureEach { | ||
| jvmTargetValidationMode = org.jetbrains.kotlin.gradle.dsl.jvm.JvmTargetValidationMode.WARNING |
There was a problem hiding this comment.
Would benefit from importing the name.
DmitryTsyvtsyn
force-pushed
the
migrate-integration-testing-to-kts
branch
from
c7f7463 to
3e35437
Compare
|
It's ready! You can check! |
No description provided.