-
Notifications
You must be signed in to change notification settings - Fork 408
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
Changes from all commits
e021943
c322173
fcd57d1
d5ab847
f27fa8c
66eb243
bf47fd9
379ab75
ed31e35
0dc471f
8337737
62a38a1
8f44842
a586e25
a7c81de
8f0ab24
bb39de7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Very good catch! Hopefully this fix 'just works'. Some subprojects might need to override this with
I guess that I think all tests should be updated to use JUnit 5. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nm, looks like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 So it can definitely wait until then, as long as it works as before now 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Something like that, if it works // plugins/gradle.build.kts
subprojects {
tasks.withType<Test>().configureEach {
useJUnitPlatform()
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.