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

Fix test runtime Java versions #2918

Merged
merged 17 commits into from Mar 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 8 additions & 4 deletions .github/workflows/gradle-test.pr.yml
Expand Up @@ -10,32 +10,36 @@ jobs:
test-ubuntu:
strategy:
matrix:
version: [ 8, 11, 17 ]
javaVersion: [ 8, 11, 17 ]
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/setup-java@v3
with:
distribution: 'zulu'
java-version: ${{ matrix.version }}
java-version: ${{ matrix.javaVersion }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems we can stick with Java 17 here.

Copy link
Contributor

@Goooler Goooler Mar 22, 2023

Choose a reason for hiding this comment

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

Addressing the fix to #2938.

cache: 'maven'
- uses: gradle/gradle-build-action@v2
env:
ORG_GRADLE_PROJECT_org.jetbrains.dokka.javaToolchain.test: ${{ matrix.javaVersion }}
with:
arguments: clean test --stacktrace

test-windows:
strategy:
matrix:
version: [ 11, 17 ]
javaVersion: [ 11, 17 ]
fail-fast: false
runs-on: windows-latest
steps:
- uses: actions/checkout@v3
- uses: actions/setup-java@v3
with:
distribution: 'zulu'
java-version: ${{ matrix.version }}
java-version: ${{ matrix.javaVersion }}
cache: 'maven'
- uses: gradle/gradle-build-action@v2
env:
ORG_GRADLE_PROJECT_org.jetbrains.dokka.javaToolchain.test: ${{ matrix.javaVersion }}
with:
arguments: clean test --stacktrace --no-daemon --parallel --max-workers=1
54 changes: 54 additions & 0 deletions build-logic/src/main/kotlin/org/jetbrains/DokkaBuildProperties.kt
@@ -0,0 +1,54 @@
package org.jetbrains

import org.gradle.api.provider.Provider
import org.gradle.api.provider.ProviderFactory
import org.gradle.jvm.toolchain.JavaLanguageVersion
import org.jetbrains.kotlin.gradle.dsl.KotlinVersion
import javax.inject.Inject

/**
* Common build properties used to build Dokka subprojects.
*
* This is an extension created by the [org.jetbrains.conventions.Base_gradle] convention plugin.
*
* Default values are set in the root `gradle.properties`, and can be overridden via
* [CLI args, system properties, and environment variables](https://docs.gradle.org/current/userguide/build_environment.html#sec:project_properties)
*/
abstract class DokkaBuildProperties @Inject constructor(
private val providers: ProviderFactory,
) {

/**
* The main version of Java that should be used to build Dokka source code.
*
* Updating the Java target is a breaking change.
*/
val mainJavaVersion: Provider<JavaLanguageVersion> =
dokkaProperty("javaToolchain.mainCompiler", JavaLanguageVersion::of)

/**
* The version of Java that should be used to run Dokka tests.
*
* This value is set in CI/CD environments to make sure that Dokka still works with different
* versions of Java.
*/
val testJavaLauncherVersion: Provider<JavaLanguageVersion> =
dokkaProperty("javaToolchain.testLauncher", JavaLanguageVersion::of)
.orElse(mainJavaVersion)

/**
* The Kotlin language level that Dokka artifacts are compiled to support.
*
* Updating the language level is a breaking change.
*/
val kotlinLanguageLevel: Provider<KotlinVersion> =
dokkaProperty("kotlinLanguageLevel", KotlinVersion::fromVersion)


private fun <T : Any> dokkaProperty(name: String, convert: (String) -> T) =
providers.gradleProperty("org.jetbrains.dokka.$name").map(convert)

companion object {
const val EXTENSION_NAME = "dokkaBuild"
}
}
Expand Up @@ -8,15 +8,39 @@ package org.jetbrains.conventions
*/

plugins {
id("org.jetbrains.conventions.base")
java
}

java {
toolchain {
languageVersion.set(JavaLanguageVersion.of(8))
languageVersion.set(dokkaBuild.mainJavaVersion)
}
}

java {
withSourcesJar()
}

tasks.withType<Test>().configureEach {
useJUnitPlatform()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2918 (comment)

I think that link doesn't work, it just links to the whole PR. Which bit needs to be fixed?

Oh, sorry. In the now deleted plugins/build.gradle.kts there used to be configuration for tasks.test with useJUnitPlatform() - I think that's what made the tests run

Very good catch! useJUnitPlatform() should have been moved to a convention plugin in #2704, but it was missed.

Hopefully this fix 'just works'. Some subprojects might need to override this with useJUnit().

The only thing I didn't understand was how tests were run in other modules like core - found no configuration for tests whatsoever

I guess that useJUnit() is the default, and most projects just use plain kotlin.test.

I think all tests should be updated to use JUnit 5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nm, looks like useJUnitPlatform() isn't a good fit, a lot of subprojects need it, but should be revisited later. I'll set useJUnit() as the default for now.

Copy link
Member

Choose a reason for hiding this comment

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

I think all tests should be updated to use JUnit 5.

As a side note: I really do want all the tests in Dokka to be revisited and made consistent: use the same version of JUnit, and use the same or a similar set of asserts, etc. Right now it's all over the place with kotlin.test, junit 4 and junit 5 asserts, stdlib's check and so on.

So it can definitely wait until then, as long as it works as before now 👍

Copy link
Member

Choose a reason for hiding this comment

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

Teamcity still reports that only 104 tests were run :(

It looks like both useJUnitPlatform() and useJUnit() need to be used, depending on which version of JUnit is used within a project. I don't mind putting a temporary hack into plugins/build.gradle.kts with useJUnitPlatform() as before. This could be revisited when the versions of JUnit are aligned in a separate PR

Something like that, if it works

// plugins/gradle.build.kts
subprojects {
    tasks.withType<Test>().configureEach {
        useJUnitPlatform()
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Created an issue for longer term refactoring: #2924

For now it'd be enough to just run all 1097 tests as before, doesn't matter much how, it's not ideal either way

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 added the necessary JUnit runtime dependencies so JUnit Platform can run both JUnit 4 and 5 tests https://junit.org/junit5/docs/current/user-guide/#running-tests-build-gradle-engines-configure

Locally I get more than 1130 test running


javaLauncher.set(javaToolchains.launcherFor {
languageVersion.set(dokkaBuild.testJavaLauncherVersion)
})
}

dependencies {
testImplementation(platform("org.junit:junit-bom:5.9.2"))
// TODO Upgrade all subprojects to use JUnit Jupiter https://github.com/Kotlin/dokka/issues/2924
// Replace these dependencies with `testImplementation("org.junit.jupiter:junit-jupiter:5.9.2")`
// See https://junit.org/junit5/docs/current/user-guide/#running-tests-build-gradle-engines-configure
// Ideally convention plugins should only provide sensible defaults that can be overridden by subprojects.
// If a convention plugin defines dependencies, these cannot be easily overridden by subprojects, and so
// this should be avoided. However, for now , both JUnit 4 and 5 must be supported, and since these are test
// runtime-only dependencies they are not going to have a significant impact subprojects.
// These dependencies should be revisited in #2924, and (for example) moved to each subproject (which is more
// repetitive, but more declarative and clear), or some other solution.
testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine")
testRuntimeOnly("org.junit.vintage:junit-vintage-engine")
}
@@ -1,10 +1,19 @@
package org.jetbrains.conventions

import org.jetbrains.DokkaBuildProperties

/**
* A convention plugin that sets up common config and sensible defaults for all subprojects.
*
* It provides the [DokkaBuildProperties] extension, for accessing common build properties.
*/

plugins {
base
}

// common Gradle configuration that should be applied to all projects
val dokkaBuildProperties: DokkaBuildProperties = extensions.create(DokkaBuildProperties.EXTENSION_NAME)


if (project != rootProject) {
project.group = rootProject.group
Expand Down
@@ -1,7 +1,6 @@
package org.jetbrains.conventions

import org.jetbrains.configureDokkaVersion
import org.jetbrains.kotlin.gradle.dsl.KotlinVersion
import org.jetbrains.kotlin.gradle.tasks.KotlinCompile

plugins {
Expand All @@ -12,8 +11,6 @@ plugins {

configureDokkaVersion()

val language_version: String by project

tasks.withType<KotlinCompile>().configureEach {
compilerOptions {
freeCompilerArgs.addAll(
Expand All @@ -26,7 +23,7 @@ tasks.withType<KotlinCompile>().configureEach {
)
)
allWarningsAsErrors.set(true)
languageVersion.set(KotlinVersion.fromVersion(language_version))
apiVersion.set(KotlinVersion.fromVersion(language_version))
languageVersion.set(dokkaBuild.kotlinLanguageLevel)
apiVersion.set(dokkaBuild.kotlinLanguageLevel)
}
}
@@ -0,0 +1,27 @@
@file:Suppress("PackageDirectoryMismatch")

package org.gradle.kotlin.dsl // for convenience use a default package for gradle.kts scripts

import org.gradle.api.Project
import org.jetbrains.DokkaBuildProperties

/*
* Utility functions for accessing Gradle extensions that are created by convention plugins.
*
* (Gradle can't generate the nice DSL accessors for the project that defines them)
*
* These functions are not needed outside the convention plugins project and should be marked as
* `internal`
*/

/**
* Retrieves the [dokkaBuild][org.jetbrains.DokkaBuildProperties] extension.
*/
internal val Project.dokkaBuild: DokkaBuildProperties
get() = extensions.getByType()

/**
* Configures the [dokkaBuild][org.jetbrains.DokkaBuildProperties] extension.
*/
internal fun Project.dokkaBuild(configure: DokkaBuildProperties.() -> Unit) =
extensions.configure(configure)
4 changes: 3 additions & 1 deletion gradle.properties
@@ -1,5 +1,8 @@
# Project Settings
dokka_version=1.8.20-SNAPSHOT
org.jetbrains.dokka.javaToolchain.mainCompiler=8
org.jetbrains.dokka.javaToolchain.testLauncher=8
org.jetbrains.dokka.kotlinLanguageLevel=1.4
dokka_integration_test_parallelism=2
# Versions
kotlin_version=1.8.10
Expand All @@ -8,7 +11,6 @@ kotlinx_html_version=0.7.5
kotlin_plugin_version=213-1.8.10-release-430-IJ6777.52
jsoup_version=1.15.3
idea_version=213.6777.52
language_version=1.4
# jackson 2.13.X does not support kotlin language version 1.4, check before updating
jackson_version=2.12.7
# fixes CVE-2022-42003
Expand Down
4 changes: 3 additions & 1 deletion integration-tests/gradle/build.gradle.kts
Expand Up @@ -22,6 +22,8 @@ tasks.integrationTest {

javaLauncher.set(javaToolchains.launcherFor {
// kotlinx.coroutines requires Java 11+
languageVersion.set(JavaLanguageVersion.of(11))
languageVersion.set(dokkaBuild.testJavaLauncherVersion.map {
maxOf(it, JavaLanguageVersion.of(11))
})
})
}
IgnatBeresnev marked this conversation as resolved.
Show resolved Hide resolved