Fix test runtime Java versions #2918

aSemy · 2023-03-13T10:29:07Z

This PR sets the Gradle JVM Toolchains used to run the tests.

Additionally it introduces a workaround for the JUnit test runner so all JUnit 4 and 5 tests will run - to be resolved in #2924

New properties.

It introduces two new Gradle Project properties

org.jetbrains.dokka.javaToolchain.mainCompiler - the version of Java used to compile Dokka
org.jetbrains.dokka.javaToolchain.testLauncher - the version of Java used to run tests

It also renames the existing language_level property to org.jetbrains.dokka.kotlinLanguageLevel, for consistency.

When developing locally, these values have defaults set in the root gradle.properties. The testLauncher property is overridden in CI/CD.

Changes

Create DokkaBuildProperties extension for convenient sharing values used to configure the Dokka build (only contains 3 values now, but this will be expanded later).

This will be very convenient for sharing properties when the build is updated to be a composite Migrate buildSrc to composite build #2912.
update base-java convention to set the test Java launcher
update GitHub workflow to set the java-version via DokkaBuildProperties and the ORG_GRADLE_PROJECT_ override

TODO

~~Update TeamCity to use the new variables (if necessary?)~~ Not necessary, as per Fix test runtime Java versions #2918 (comment)
- org.jetbrains.dokka.javaToolchain.mainCompiler
- org.jetbrains.dokka.javaToolchain.testLauncher
- org.jetbrains.dokka.kotlinLanguageLevel

- Create DokkaBuildProperties extension for convenient sharing values used to configure the Dokka build (only contains 3 values now, but this will be expanded later) - update base-java convention to set the test Java launcher - update GitHub workflow to set the java-version via DokkaBuildProperties

aSemy · 2023-03-13T10:42:03Z

@Goooler WDYT?

buildSrc/src/main/kotlin/org/jetbrains/DokkaBuildProperties.kt

buildSrc/src/main/kotlin/org/jetbrains/conventions/base-java.gradle.kts

Co-authored-by: Goooler <wangzongler@gmail.com>

buildSrc/src/main/kotlin/org/jetbrains/DokkaBuildProperties.kt

buildSrc/src/main/kotlin/org/jetbrains/conventions/base.gradle.kts

buildSrc/src/main/kotlin/org/jetbrains/internal/gradleKotlinDslAccessors.kt

Co-authored-by: Goooler <wangzongler@gmail.com>

…va_version # Conflicts: # build-logic/src/main/kotlin/org/jetbrains/DokkaBuildProperties.kt # build-logic/src/main/kotlin/org/jetbrains/conventions/base-java.gradle.kts # build-logic/src/main/kotlin/org/jetbrains/internal/gradleKotlinDslAccessors.kt

build-logic/src/main/kotlin/org/jetbrains/conventions/base.gradle.kts

IgnatBeresnev

Nice solution! Having build properties should come in handy 👍 And thanks to @Goooler for taking a look as well

I'll start teamcity-based integration tests, and merge it tomorrow during the day

IgnatBeresnev · 2023-03-16T02:35:47Z

Non-blocking nitpick, will merge without it, but do let me know if you want to address it in this PR: maybe the fix we put in for the integration tests could be revisited? Is there a better option after current changes, like setting the new build property value to 11 and letting the convention plugin do the configuration part? GitHub actions configuration for them could be corrected as well it seems

Update TeamCity to use the new variables (if necessary?)

Doesn't look like it needs to be updated - they only run the test task, and have the same JVM version as the default one, so it should be good. The matrix thing is GitHub Actions only

IgnatBeresnev · 2023-03-16T03:27:24Z

Oops, it looks like the majority of unit tests are not run at all 😅

Most likely this configuration was forgotten to be moved to the convention plugins.

Could you fix it in this or in a separate PR, please? There shouldn't be any problems with the tests themselves since we didn't merge anything that could break them

…dle.kts Co-authored-by: Goooler <wangzongler@gmail.com>

aSemy · 2023-03-16T12:37:48Z

Oops, it looks like the majority of unit tests are not run at all 😅

Most likely this configuration was forgotten to be moved to the convention plugins.

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

IgnatBeresnev · 2023-03-16T12:47:13Z

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

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

integration-tests/gradle/build.gradle.kts

aSemy · 2023-03-16T12:55:11Z

build-logic/src/main/kotlin/org/jetbrains/conventions/base-java.gradle.kts

    }
 }

 java {
    withSourcesJar()
 }
+
+tasks.withType<Test>().configureEach {
+    useJUnitPlatform()


#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.

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.

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 👍

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() } }

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

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

…g the tests would be a big change that should be done in another PR

IgnatBeresnev · 2023-03-16T19:17:22Z

Looks like unit tests have been fixed, thanks 🎉 All 1097 of them

Goooler · 2023-03-20T04:06:37Z

.github/workflows/gradle-test.pr.yml

    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 }}


Seems we can stick with Java 17 here.

Addressing the fix to #2938.

Goooler reviewed Mar 13, 2023

View reviewed changes

buildSrc/src/main/kotlin/org/jetbrains/DokkaBuildProperties.kt Outdated Show resolved Hide resolved

aSemy added 2 commits March 13, 2023 12:13

suppress PackageDirectoryMismatch in accessors util

c322173

rename accessors file

fcd57d1

aSemy commented Mar 13, 2023

View reviewed changes

buildSrc/src/main/kotlin/org/jetbrains/conventions/base-java.gradle.kts Outdated Show resolved Hide resolved

aSemy and others added 2 commits March 13, 2023 12:14

fix Version typo

d5ab847

Co-authored-by: Goooler <wangzongler@gmail.com>

default testLauncher to mainJavaVersion

f27fa8c

Goooler reviewed Mar 13, 2023

View reviewed changes

buildSrc/src/main/kotlin/org/jetbrains/DokkaBuildProperties.kt Outdated Show resolved Hide resolved

buildSrc/src/main/kotlin/org/jetbrains/DokkaBuildProperties.kt Outdated Show resolved Hide resolved

IgnatBeresnev self-requested a review March 13, 2023 12:25

IgnatBeresnev added the infrastructure Everything related to builds tools, CI configurations and project tooling label Mar 13, 2023

Goooler reviewed Mar 14, 2023

View reviewed changes

aSemy and others added 4 commits March 14, 2023 16:05

use function references instead of lambdas

66eb243

Co-authored-by: Goooler <wangzongler@gmail.com>

Remove explicit type arguments

bf47fd9

Co-authored-by: Goooler <wangzongler@gmail.com>

tidy up comment

379ab75

Goooler reviewed Mar 14, 2023

View reviewed changes

build-logic/src/main/kotlin/org/jetbrains/conventions/base.gradle.kts Outdated Show resolved Hide resolved

IgnatBeresnev approved these changes Mar 16, 2023

View reviewed changes

aSemy and others added 2 commits March 16, 2023 13:33

Update build-logic/src/main/kotlin/org/jetbrains/conventions/base.gra…

0dc471f

…dle.kts Co-authored-by: Goooler <wangzongler@gmail.com>

Merge branch 'master' into fix/test_runtime_java_version

8337737

tweak the 'kotlinx.coroutines requires Java 11' fix

62a38a1

aSemy commented Mar 16, 2023

View reviewed changes

integration-tests/gradle/build.gradle.kts Show resolved Hide resolved

set useJUnitPlatform() in base-java convention

8f44842

aSemy commented Mar 16, 2023

View reviewed changes

aSemy added 2 commits March 16, 2023 14:11

add JUnit engines for both JUnit 4 and 5

a586e25

replace useJUnitPlatform() with useJUnit() - aligning and updatin…

a7c81de

…g the tests would be a big change that should be done in another PR

This was referenced Mar 16, 2023

Align versions of JUnit and use the same test api across the project #2924

Closed

Integration tests refactoring #2925

Open

aSemy added 2 commits March 16, 2023 17:21

use JUnit Platform & add necessary runtime dependencies

8f0ab24

update TODO comment with more info

bb39de7

IgnatBeresnev merged commit 18d01bf into Kotlin:master Mar 16, 2023
28 of 29 checks passed

aSemy deleted the fix/test_runtime_java_version branch March 16, 2023 19:37

Goooler reviewed Mar 20, 2023

View reviewed changes

Goooler mentioned this pull request Mar 22, 2023

Stick with Java 17 on CI #2938

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix test runtime Java versions #2918

Fix test runtime Java versions #2918

aSemy commented Mar 13, 2023 •

edited

aSemy commented Mar 13, 2023

IgnatBeresnev left a comment

IgnatBeresnev commented Mar 16, 2023 •

edited

IgnatBeresnev commented Mar 16, 2023 •

edited

aSemy commented Mar 16, 2023

IgnatBeresnev commented Mar 16, 2023

aSemy Mar 16, 2023

aSemy Mar 16, 2023

IgnatBeresnev Mar 16, 2023

IgnatBeresnev Mar 16, 2023 •

edited

IgnatBeresnev Mar 16, 2023 •

edited

aSemy Mar 16, 2023

IgnatBeresnev commented Mar 16, 2023

Goooler Mar 20, 2023

Goooler Mar 22, 2023 •

edited

Fix test runtime Java versions #2918

Fix test runtime Java versions #2918

Conversation

aSemy commented Mar 13, 2023 • edited

New properties.

Changes

TODO

aSemy commented Mar 13, 2023

IgnatBeresnev left a comment

Choose a reason for hiding this comment

IgnatBeresnev commented Mar 16, 2023 • edited

IgnatBeresnev commented Mar 16, 2023 • edited

aSemy commented Mar 16, 2023

IgnatBeresnev commented Mar 16, 2023

aSemy Mar 16, 2023

Choose a reason for hiding this comment

aSemy Mar 16, 2023

Choose a reason for hiding this comment

IgnatBeresnev Mar 16, 2023

Choose a reason for hiding this comment

IgnatBeresnev Mar 16, 2023 • edited

Choose a reason for hiding this comment

IgnatBeresnev Mar 16, 2023 • edited

Choose a reason for hiding this comment

aSemy Mar 16, 2023

Choose a reason for hiding this comment

IgnatBeresnev commented Mar 16, 2023

Goooler Mar 20, 2023

Choose a reason for hiding this comment

Goooler Mar 22, 2023 • edited

Choose a reason for hiding this comment

aSemy commented Mar 13, 2023 •

edited

IgnatBeresnev commented Mar 16, 2023 •

edited

IgnatBeresnev commented Mar 16, 2023 •

edited

IgnatBeresnev Mar 16, 2023 •

edited

IgnatBeresnev Mar 16, 2023 •

edited

Goooler Mar 22, 2023 •

edited