-
Notifications
You must be signed in to change notification settings - Fork 411
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
Fix test runtime Java versions #2918
Conversation
- 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
@Goooler WDYT? |
buildSrc/src/main/kotlin/org/jetbrains/conventions/base-java.gradle.kts
Outdated
Show resolved
Hide resolved
Co-authored-by: Goooler <wangzongler@gmail.com>
buildSrc/src/main/kotlin/org/jetbrains/conventions/base.gradle.kts
Outdated
Show resolved
Hide resolved
buildSrc/src/main/kotlin/org/jetbrains/internal/gradleKotlinDslAccessors.kt
Outdated
Show resolved
Hide resolved
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
build-logic/src/main/kotlin/org/jetbrains/conventions/base.gradle.kts
Outdated
Show resolved
Hide resolved
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.
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
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
Doesn't look like it needs to be updated - they only run the |
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>
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 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() |
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.
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 fortasks.test
withuseJUnitPlatform()
- 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.
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.
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.
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.
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 👍
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.
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()
}
}
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.
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 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
…g the tests would be a big change that should be done in another PR
Looks like unit tests have been fixed, thanks 🎉 All 1097 of them |
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 }} |
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.
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 Dokkaorg.jetbrains.dokka.javaToolchain.testLauncher
- the version of Java used to run testsIt also renames the existing
language_level
property toorg.jetbrains.dokka.kotlinLanguageLevel
, for consistency.When developing locally, these values have defaults set in the root
gradle.properties
. ThetestLauncher
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 launcherupdate GitHub workflow to set the java-version via
DokkaBuildProperties
and theORG_GRADLE_PROJECT_
overrideTODO
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