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

Conversation

aSemy
Copy link
Contributor

@aSemy aSemy commented Mar 13, 2023

fix #2917

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
Copy link
Contributor Author

aSemy commented Mar 13, 2023

@Goooler WDYT?

aSemy and others added 2 commits March 13, 2023 12:14
Co-authored-by: Goooler <wangzongler@gmail.com>
@IgnatBeresnev IgnatBeresnev self-requested a review March 13, 2023 12:25
@IgnatBeresnev IgnatBeresnev added the infrastructure Everything related to builds tools, CI configurations and project tooling label Mar 13, 2023
aSemy and others added 4 commits March 14, 2023 16:05
Co-authored-by: Goooler <wangzongler@gmail.com>
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
Copy link
Member

@IgnatBeresnev IgnatBeresnev left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

IgnatBeresnev commented Mar 16, 2023

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
Copy link
Member

IgnatBeresnev commented Mar 16, 2023

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

2023-03-16_03-34-39

@aSemy
Copy link
Contributor Author

aSemy commented Mar 16, 2023

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
Copy link
Member

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

}
}

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

@IgnatBeresnev IgnatBeresnev Mar 16, 2023

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

@IgnatBeresnev IgnatBeresnev Mar 16, 2023

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

@IgnatBeresnev
Copy link
Member

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

@IgnatBeresnev IgnatBeresnev merged commit 18d01bf into Kotlin:master Mar 16, 2023
@aSemy aSemy deleted the fix/test_runtime_java_version branch March 16, 2023 19:37
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Everything related to builds tools, CI configurations and project tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: All versions of Java are not tested in CI/CD
3 participants