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

Java 8 CI to prevent regressions #902

Open
TWiStErRob opened this issue Aug 25, 2022 · 11 comments
Open

Java 8 CI to prevent regressions #902

TWiStErRob opened this issue Aug 25, 2022 · 11 comments

Comments

@TWiStErRob
Copy link

TWiStErRob commented Aug 25, 2022

Expected Behavior

Java 8 is tested on CI on every PR, so that breakages like #898 cannot happen.

Current Behavior

Java 11 - 18 on CI.

Fix

You can keep building on whatever Java you want, but in Gradle, specify on what JVM the tests are executed on. Gradle Test tasks fork a separate process by default, so there's no hit here. It should be a simple case of setting the Java launcher for the tests:

tasks.withType<Test>().configureEach {
  it.javaLauncher.set(javaToolchains.launcherFor {
      languageVersion = JavaLanguageVersion.of("8")
  })
}

Going further

You can remove all the builds from CI and just use 1 build that tests all the Javas at once... 🤯:
https://jakewharton.com/build-on-latest-java-test-through-lowest-java/
Jake delivers once again! :)

Failure Logs

I can see (by running the build on Java 8) that AGP is preventing you from that, but that needn't be the case.

* Where:
Build file 'P:\projects\contrib\github-mockk\modules\mockk-agent-android\build.gradle.kts' line: 4

* What went wrong:
An exception occurred applying plugin request [id: 'buildsrc.convention.android-library']
> Failed to apply plugin 'com.android.internal.library'.
   > Android Gradle plugin requires Java 11 to run. You are currently using Java 1.8.
     Your current JDK is located in  P:\tools\lang\java-1.8.0_201-x64-jdk\jre
     You can try some of the following options:
       - changing the IDE settings.
       - changing the JAVA_HOME environment variable.
       - changing `org.gradle.java.home` in `gradle.properties`.

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.
aSemy added a commit to aSemy/mockk that referenced this issue Aug 25, 2022
@aSemy
Copy link
Contributor

aSemy commented Aug 25, 2022

Some context: JDK8 was in the matrix but it was removed here de4448f.

An update: I've tried this out as part of #900, but it causes issues because the Android plugin requires JDK 11
https://github.com/mockk/mockk/runs/8024512958?check_suite_focus=true#step:6:263

@Raibaz
Copy link
Collaborator

Raibaz commented Aug 25, 2022

I guess this leaves us with two options:

  • Not testing Java 8 in CI
  • Not testing Android in CI

And given that Java 8 is no longer being updated by Oracle, I guess it's an easy choice :)

@TWiStErRob
Copy link
Author

Please read the linked article, the JVM running the build and the JVM running the test are very distinct.

(I just learned this myself and introduced it at work, and it makes things much simpler when you understand each, and separate versions. There's one JVM running the Gradle (can be 18), there's the language level of your Java source code (I think it can be high), there's the target version of class file generated (#898), and there's the JVM version of the tests/consumers (#902).)

Eventually, I'm happy to help out here with CI. I'm just traveling these weekends.

Note: until Kotlin drops support for targeting Java 8, and Android catches up (take this seriously, we tried using Java 11 recently on a big app and we were getting crashes at runtime, so we literally cannot compile with Java 11 target), there'll be Java 8 users of Kotlin libraries for more years to come. If you drop support for it, people will be forced to drop Mockk and look for alternatives.

@aSemy
Copy link
Contributor

aSemy commented Aug 26, 2022

MockK does set up Toolchains, but it's different to the article.

https://github.com/mockk/mockk/blob/master/buildSrc/src/main/kotlin/buildsrc/convention/toolchain-jvm.gradle.kts

@jschneider
Copy link

I am not sure about the target JDKs of this project. But the lastest release does not work with JDK 8 anymore:

io/mockk/proxy/jvm/dispatcher/JvmMockKDispatcher has been compiled by a more recent version of the Java Runtime (class file version 55.0), this version of the Java Runtime only recognizes class file versions up to 52.0
java.lang.UnsupportedClassVersionError: io/mockk/proxy/jvm/dispatcher/JvmMockKDispatcher has been compiled by a more recent version of the Java Runtime (class file version 55.0), this version of the Java Runtime only recognizes class file versions up to 52.0

I am unsure if this is a bug or a feature :-)

@aSemy
Copy link
Contributor

aSemy commented Aug 29, 2022

@jschneider I expect that JDK8 compatibility will be restored by #907 :)

@3flex
Copy link

3flex commented Aug 31, 2022

I guess this leaves us with two options:

  • Not testing Java 8 in CI
  • Not testing Android in CI

Another option - use something like @EnabledForJreRange(min = JAVA_11) on the Android tests. I didn't check which test runner you're using (this is for JUnit 5 - but should be possible on others), but this allows full range of Java versions to be used in CI while skipping Android tests on incompatible versions.

@TWiStErRob
Copy link
Author

Nice compromise @3flex! JUnit 4 has assumeTrue/assumeThat which is not that declarative, but has the same result: ignores the test.

I'll try to give splitting the Java versions a go on the weekend and either raise a PR or report my findings.

@Raibaz
Copy link
Collaborator

Raibaz commented Aug 31, 2022

Thanks @TWiStErRob!

I'll hold off publishing a new release until after the weekend then, so if you make any progress on this we can include it.

@TWiStErRob
Copy link
Author

Not necessary @Raibaz, it won't change the artifacts, only the CI. #898 (Java 8 issue) was already fixed by #900 + #907. This is just a followup to prevent it happening again.

@aSemy
Copy link
Contributor

aSemy commented Sep 11, 2022

You can remove all the builds from CI and just use 1 build that tests all the Javas at once... 🤯: https://jakewharton.com/build-on-latest-java-test-through-lowest-java/ Jake delivers once again! :)

I've tried implementing this approach. It wasn't easy because the Kotlin JVM test tasks are named dynamically, and they're created lazily.

Unfortunately, once set up, all the tests pass - including the sealed-class tests that are supposed to be broken :(

Here's the updated Toolchain convention, but because it doesn't work, I don't want to commit it.

It provides these tasks:

  • lifecycle tasks that will run all JDK tests, whether Kotlin or Java
    • checkJdk11
    • checkJdk17
    • checkJdk18
    • checkJdk8
    • checkJdks - runs all checkJdk* tasks
  • Kotlin JVM JDK tests
    • jvmTestJdk11
    • jvmTestJdk17
    • jvmTestJdk18
    • jvmTestJdk8
  • Java JDK tests
    • testJdk11
    • testJdk17
    • testJdk18
    • testJdk8
// buildSrc/src/main/kotlin/buildsrc/convention/toolchain-jvm.gradle.kts

package buildsrc.convention

import buildsrc.config.Deps
import org.jetbrains.kotlin.gradle.tasks.KotlinCompile
import org.jetbrains.kotlin.gradle.tasks.UsesKotlinJavaToolchain


description = "Set JavaToolchain for compiling main and test code"


tasks.withType<JavaCompile>().configureEach {
    options.encoding = "UTF-8"
    sourceCompatibility = Deps.Versions.jvmTarget.toString()
    targetCompatibility = Deps.Versions.jvmTarget.toString()
}

tasks.withType<KotlinCompile>().configureEach {
    kotlinOptions {
        jvmTarget = Deps.Versions.jvmTarget.toString()
    }
}

// Retrieve the JavaToolchainService extension
val javaToolchains: JavaToolchainService = extensions.getByType()

// Add this marker property to each JDK specific task, to differentiate them from the regular test task it is based on
val testTaskJdkMarkerName = "test jdk task"

val testJdkTargets = listOf(8, 11, 17, 18)

val javaToolchainMainVersion: Provider<JavaLanguageVersion> =
    providers.gradleProperty("javaToolchainMainVersion")
        .map { JavaLanguageVersion.of(it) }


tasks.withType<JavaCompile>().configureEach {
    javaCompiler.set(
        javaToolchains.compilerFor { languageVersion.set(javaToolchainMainVersion) }
    )
}

tasks.withType<UsesKotlinJavaToolchain>().configureEach {
    kotlinJavaToolchain.toolchain.use(
        javaToolchains.launcherFor { languageVersion.set(javaToolchainMainVersion) }
    )
}

val checkJdks by tasks.registering {
    group = LifecycleBasePlugin.VERIFICATION_GROUP
    description = "Run all JDK tests"
}

// create Lifecycle tasks for each JDK version
testJdkTargets.forEach { testJavaVersion ->
    val checkJdkVersion = tasks.register(checkJdkTaskName(testJavaVersion)) {
        group = LifecycleBasePlugin.VERIFICATION_GROUP
        description = "Run all JDK $testJavaVersion tests"
    }
    checkJdks.configure { dependsOn(checkJdkVersion) }
}


// for each Test task, create JDK tests
// must use afterEvaluate because the Kotlin plugin adds jvmTest lazily
afterEvaluate {
    tasks.withType<Test>()
        .matching {
            !it.extensions.extraProperties.has(testTaskJdkMarkerName)
        }
        .forEach {
            registerTestJdkTasks(it)
        }
}


fun registerTestJdkTasks(baseTask: Test) {
    testJdkTargets.forEach { testJavaVersion ->
        logger.info("registering test jdk $testJavaVersion task ${baseTask.name} ${baseTask::class}")

        val testJdkVersion = tasks.register<Test>("${baseTask.name}Jdk$testJavaVersion") {
            javaLauncher.set(
                javaToolchains.launcherFor {
                    languageVersion.set(JavaLanguageVersion.of(testJavaVersion))
                }
            )

            description = "Runs the '${baseTask.name}' suite on JDK $testJavaVersion"
            group = LifecycleBasePlugin.VERIFICATION_GROUP

            // marker property to stop recursion
            extensions.extraProperties.set(testTaskJdkMarkerName, testJavaVersion)

            // Copy inputs from base Test task
            classpath = baseTask.classpath
            testClassesDirs = baseTask.testClassesDirs

            useJUnitPlatform()
        }
        tasks.named(checkJdkTaskName(testJavaVersion)).configure { dependsOn(testJdkVersion) }
    }
}


fun checkJdkTaskName(testJavaVersion: Int): String = "checkJdk$testJavaVersion"

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

No branches or pull requests

5 participants