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

Integration tests #345

Merged
merged 31 commits into from
Nov 13, 2023
Merged

Integration tests #345

merged 31 commits into from
Nov 13, 2023

Conversation

mvicsokolova
Copy link
Collaborator

@mvicsokolova mvicsokolova commented Sep 11, 2023

This PR introduces the framework for atomicfu-gradle-plugin integration tests.
To run functional tests:

cd integration-testing
./gradlew functionalTest

Current problems:

  • Do not copy versions in build scripts of sample projects #gradle

  • Add tests for the JS IR backend

  • DependenciesChecker: should also check apiElements and runtimeElements configurations

  • DependenciesChecker: it'd be nice to check .module or .pom files after publication of the project for "consumable" dependencies that will not be present in the dependencies output

  • AtomicfuGradlePlugin: configuration of dependencies for MPP project should be reviewed, currently it causes Mpp dependency test to fail

  • klib dump-ir checks are not enabled for now because of this known problem KT-61143, looking for WA

@mvicsokolova mvicsokolova changed the title WIP: introduced simple JVM-only project in integration tests WIP: Integration tests Oct 4, 2023
@mvicsokolova mvicsokolova marked this pull request as ready for review October 12, 2023 16:13
@@ -0,0 +1,28 @@
buildscript {
val atomicfu_version = rootProject.properties["atomicfu_version"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And what is the proper way to get atomicfu_version here?)

Copy link
Contributor

Choose a reason for hiding this comment

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

You can override the plugin version in settings.gradle.kts file. In this case, it will be possible to apply the plugin in the plugins { } block

pluginManagement {
    
    resolutionStrategy {
        eachPlugin {
            val fuVersion = gradle.rootProject.properties["atomicfu_version"]?.toString()
            if (fuVersion != null && requested.id.id != "kotlinx-atomicfu") {
                useVersion(fuVersion)
            }
        }
    }
    
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The error is: The root project is not yet available for build.
And I get the same error for version catalogs, so, I only managed to do it like this:

dependencyResolutionManagement {
    versionCatalogs {
        create("libs") {
            version("atomicfu", "0.22.0-SNAPSHOT")
            version("kotlin", "1.9.20")
        }
    }
}

But it's still versions hardcoding 👀

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess it's fine to leave like it is for now then

val config = config
withPluginWhenEvaluated("kotlin") {
if (config.transformJvm) configureJvmTransformation()
private fun Project.configureMultiplatformPluginDependencies(version: String) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only conceptual change in the AtomicfuGradlePlugin is in configureMultiplatformPluginDependencies, all other changes are just refactoring: changing the order of declarations.

val config = config
withPluginWhenEvaluated("kotlin") {
if (config.transformJvm) configureJvmTransformation()
private fun Project.configureMultiplatformPluginDependencies(version: String) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only conceptual change in the AtomicfuGradlePlugin is in configureMultiplatformPluginDependencies, all other changes are just refactoring: changing the order of declarations.

@mvicsokolova mvicsokolova changed the title WIP: Integration tests Integration tests Oct 23, 2023
@mvicsokolova mvicsokolova requested a review from fzhinkin October 23, 2023 11:52
Copy link
Contributor

@fzhinkin fzhinkin 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! Overall looks good, there's only a few minor issues to address.

}

plugins {
kotlin("jvm") version "${project.properties["kotlin_version"]}" // todo: is there a more consise way to get kotlin_version from gradle.properties?
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I configure version catalog like this, I get "The root project is not yet available for build." 👀

dependencyResolutionManagement {
    versionCatalogs {
        create("libs") {
            version("atomicfu", gradle.rootProject.properties["atomicfu_version"])
        }
   }
}

integration-testing/examples/mpp-sample/build.gradle.kts Outdated Show resolved Hide resolved
integration-testing/build.gradle.kts Show resolved Hide resolved
return BuildResult(exitCode, logFile)
}

private fun buildSystemCommand(projectDir: File, commands: List<String>, properties: List<String>): List<String> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to use TestKit instead of a custom GradleRunner - because supporting it in the future requires additional labor.

TextKit has some problems with the plugin's classpath, however, if configured correctly, they should not appear.


internal fun GradleBuild.clean(): BuildResult = runGradle(listOf("clean"))

internal fun GradleBuild.build(): BuildResult = runGradle(listOf("clean", "build"))
Copy link
Contributor

Choose a reason for hiding this comment

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

cleanAndBuild?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

val output: String by lazy { logFile.readText() }

// Gets the list of dependencies for the given configuration
fun getDependenciesForConfig(configuration: String): List<String> {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to use regular expressions

@@ -0,0 +1,28 @@
buildscript {
val atomicfu_version = rootProject.properties["atomicfu_version"]
Copy link
Contributor

Choose a reason for hiding this comment

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

You can override the plugin version in settings.gradle.kts file. In this case, it will be possible to apply the plugin in the plugins { } block

pluginManagement {
    
    resolutionStrategy {
        eachPlugin {
            val fuVersion = gradle.rootProject.properties["atomicfu_version"]?.toString()
            if (fuVersion != null && requested.id.id != "kotlinx-atomicfu") {
                useVersion(fuVersion)
            }
        }
    }
    
}

integration-testing/build.gradle.kts Show resolved Hide resolved
integration-testing/build.gradle.kts Outdated Show resolved Hide resolved
private fun Project.configureMultiplatformPluginDependencies(version: String) {
val multiplatformExtension = kotlinExtension as? KotlinMultiplatformExtension ?: error("Expected kotlin multiplatform extension")
val atomicfuDependency = "org.jetbrains.kotlinx:atomicfu:$version"
multiplatformExtension.sourceSets.getByName("commonMain").dependencies {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can there be a situation when there are no commonMain or commonTest source sets in the project?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess, commonMain и commonTest always exist in the mpp project acording to the kotlin-gradle-plugin structure

integration-testing/examples/jvm-sample/gradle.properties Outdated Show resolved Hide resolved
integration-testing/examples/mpp-sample/build.gradle.kts Outdated Show resolved Hide resolved
@mvicsokolova
Copy link
Collaborator Author

mvicsokolova commented Nov 7, 2023

The reason of the current build failure: mavenTest depends on atomicfu-gradle-plugin:publishToMavenLocal now and the locally published atomicfu-jvm:0.22.0-SNAPSHOT.jar should be added to it's classpath.

@fzhinkin

This comment was marked as resolved.

@fzhinkin
Copy link
Contributor

fzhinkin commented Nov 8, 2023

As an extra effort to keep things nice and clean, it might be worth adding a separate set of publishToLocal tasks that will write artifacts to a temporary repo in the build directory (like int tests do).
In that case, we can be sure that atomicfu-artifacts won't be published even to the local maven repo if something is broken.
Not sure how difficult it would be to add such "publishToBuildLocal" publication tasks though.

@mvicsokolova
Copy link
Collaborator Author

mvicsokolova commented Nov 8, 2023

I fixed mavenTest task by depending directly to the :atomicfu:publishToMavenLocal task, which should be avoided, but I guess we can stick to this solution for now. I've created an issue to collect improvements for integration tests, including adding separate tasks to publish artifacts to the custom local repo. (#362)

mvicsokolova and others added 18 commits November 11, 2023 14:21
- made integration-testing part of the rootProject
- removed duplicated gradle.properties
- integration-testing depends on atomicfu-gradle-plugin publication
Pass atomicfu and kotlin versions to sample projects as parameters
…by the time of atomicfu-gradle-plugin application
…IntArithmeticTest.kt

Co-authored-by: Filipp Zhinkin <filipp.zhinkin@gmail.com>
@mvicsokolova
Copy link
Collaborator Author

Rebased on the latest develop with Kotlin updated to 1.9.20

@mvicsokolova
Copy link
Collaborator Author

mvicsokolova commented Nov 13, 2023

Well, something happened (after rebase to develop with updated Kotlin 1.9.20) and mpp-sample build is failing with this error again 🪦
Probably, because of some kotlin-stdlib conflicts on the classpath and locally this error is fixed with cleaning local kotlin repo: ~/.m2/org/jetbrains/kotlin/

> Task :compileKotlinJs FAILED
e: java.lang.AssertionError: Built-in class kotlin.Any is not found
  at org.jetbrains.kotlin.builtins.KotlinBuiltIns$3.invoke(KotlinBuiltIns.java:93)
  at org.jetbrains.kotlin.builtins.KotlinBuiltIns$3.invoke(KotlinBuiltIns.java:88)

@@ -25,6 +25,7 @@ apply(plugin = "kotlinx-atomicfu")

repositories {
mavenCentral()
maven("https://maven.pkg.jetbrains.space/kotlin/p/kotlin/dev")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to have these repos ordered differently in jvm- and mpp-sample projects?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem with unresolved classes from kotlin-stdlib may've been caused by the maven local repository being corrupted by some other process, and it's priority caused gradle to search for stdlib classes there first. As gradle can not work properly with mavenLocal, it's better not to set it first in the repo list .

@mvicsokolova mvicsokolova merged commit 9d2a3e4 into develop Nov 13, 2023
@mvicsokolova mvicsokolova deleted the integration-tests branch November 13, 2023 12:28
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.

3 participants