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

add explicit dependency on JUnit Platform Launcher #3525

Closed
wants to merge 2 commits into from

Conversation

adam-enko
Copy link
Contributor

@adam-enko adam-enko commented Mar 8, 2024

Avoid warning regarding "The automatic loading of test framework implementation dependencies has been deprecated."

https://ge.jetbrains.com/s/hnoer7sb3saqa/deprecations?expanded=WyIyMTEiXQ

https://docs.gradle.org/8.4/userguide/upgrading_version_8.html#test_framework_implementation_dependencies

Result

A build scan for this build has other warnings, but there's no more 'automatic loading of test framework' warning https://ge.jetbrains.com/s/olrscf2gfuuke/deprecations

@adam-enko adam-enko self-assigned this Mar 8, 2024
@adam-enko adam-enko added the infrastructure Everything related to builds tools, CI configurations and project tooling label Mar 8, 2024
@adam-enko adam-enko marked this pull request as ready for review March 8, 2024 17:40
@adam-enko adam-enko marked this pull request as draft March 8, 2024 17:41
@adam-enko adam-enko marked this pull request as ready for review March 11, 2024 08:30
@@ -33,6 +33,7 @@ tasks.withType<Test>().configureEach {

dependencies {
testImplementation(platform(libs.junit.bom))
testImplementation(libs.junit.platformLauncher)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just add here testImplementation(kotlin("test")) and remove all other usages of it in other modules?
Or it will not work as intended, because there is no kotlin plugin applied here?

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 given it a quick test and it looks like it works (build scan), but I'd prefer not to.

  • Not all subprojects have kotlin("test") at the moment.
  • Adding dependencies in convention plugins makes reasoning about the available dependencies in the subprojects harder, as you can't just look in the build.gradle.kts of the projects. (Adding testImplementation(platform(libs.junit.bom)) was a compromise when updating Dokka to use convention plugins). And they can't easily be overridden in subprojects.
  • Adding test dependencies for Test Suite tests is also completely separate dependency, so it's misleading adding these dependencies as a 'default' for tests, but then for subprojects that use Test Suites it's not.

Maybe I'm missing some positives. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding dependencies in convention plugins makes reasoning about the available dependencies in the subprojects harder

If we have tests, then most likely we need kotlin-test :) So, by default, I would even expect that kotlin-test will be available everywhere, instead of declaring it dependency in every project. Though, I've rechecked that we have both junit, junit5 and kotest tests - so may be it's really not a good idea here.

Adding test dependencies for Test Suite tests is also completely separate dependency, so it's misleading adding these dependencies as a 'default' for tests, but then for subprojects that use Test Suites it's not.

But, isn't there is the same issue when we add testImplementation(libs.junit.platformLauncher), like in this PR?

Maybe I'm missing some positives.

Yeah, I thought about just making tests sources consistent (use kotlin-test everywhere, use same framewok, etc), but looks like it will not work here, at least not now.

In the end, I see, that there are a lot of other test-related warnings in report, so I think, that may be some more robust cleanup is needed to make tests behaviour/configuration consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding test dependencies for Test Suite tests is also completely separate dependency, so it's misleading adding these dependencies as a 'default' for tests, but then for subprojects that use Test Suites it's not.

But, isn't there is the same issue when we add testImplementation(libs.junit.platformLauncher), like in this PR?

Yeah, good point. I've updated the dependencies to be explicitly added in each subproject.

In the end, I see, that there are a lot of other test-related warnings in report, so I think, that may be some more robust cleanup is needed to make tests behaviour/configuration consistent?

Yes, there are other warnings.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, I see your points, thank you :)

Last question: we have useJUnitPlatform() declared in dokkabuild.java convention plugin, but dependency for junit-platform-launcher test runner is now declared in each subproject.

So, may be, let's do nothing for now?
As for me, those changes are making current tests setup even more confusing :(

And separately, IMO, it's fine to have a separate convention plugin which will configure test tasks/dependencies for java/kotlin/test-suties in one place, so that in the end there will be a single place for it, as now it's copy pasted a lot, dependencies are sometimes different (and I have no idea why). May be I'm wrong or missing something, but I believe we should move in direction to similar setup / configuration reuse for subproject, not otherwise.
If there is a big difference for intergration tests vs subprojects tests vs runners configuration - I think it will be fine to create separate convention plugins or something else - but it's not that required now, just my thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Last question: we have useJUnitPlatform() declared in dokkabuild.java convention plugin, but dependency for junit-platform-launcher test runner is now declared in each subproject.

Yeah, that's confusing to think about, but it's basically the point of convention plugins. In theory, a subproject could decide it needs to use another test framework, and so it could override the convention. However, the same isn't true for dependencies, so it would be hard for that subproject to remove the existing dependency.

But that's theoretical best practice. With Dokka it's not the case at the moment, and even if it does change the build config can be refactored in the future.

* `kotlin.test` in 2.0 will include dependency on it - https://youtrack.jetbrains.com/issue/KT-63397/kotlin-test-should-declare-runtime-dependency-on-org.junit.platformjunit-platform-launcher

So, may be, let's do nothing for now? As for me, those changes are making current tests setup even more confusing :(

Ah, good find about kotlin.test and the platform launcher! I would still like to squish the warning though. Hmm. Well, it's not urgent, so I can put this PR back into a draft and we can sit on it for a bit.

I really agree that dependency management can be confusing. There are too many options!

Another option we could do is to use a libs.versions.toml dependency bundle for JVM testing, that would include kotlin-test and platform-launcher (with a remove this when updating to Kotlin 2.0 comment), and any other common JUnit test dependency.

And separately, IMO, it's fine to have a separate convention plugin which will configure test tasks/dependencies for java/kotlin/test-suties in one place, so that in the end there will be a single place for it, as now it's copy pasted a lot, dependencies are sometimes different (and I have no idea why). May be I'm wrong or missing something, but I believe we should move in direction to similar setup / configuration reuse for subproject, not otherwise. If there is a big difference for intergration tests vs subprojects tests vs runners configuration - I think it will be fine to create separate convention plugins or something else - but it's not that required now, just my thoughts.

We could certainly cut down on the repetition, and some of the dependency messiness.

I'd still prefer to avoid using convention plugins for dependency management, but we could standardise things a bit more:

  • create a java-platform project for dependency alignment, like the JUnit BOM dependency.
  • Update the projects to use test-fixtures, and expose test dependencies like kotlin-test with testApi(...)

However, that could be more magical and confusing, so, pick your poison :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, let's go for now with explicit dependencies in subprojects and decide on what to do next later if needed, when we will see other issues with tests or if we will start migrating more to test-suites/test-fixtures, cause I really don't know what will be better here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

* `kotlin.test` in 2.0 will include dependency on it - https://youtrack.jetbrains.com/issue/KT-63397/kotlin-test-should-declare-runtime-dependency-on-org.junit.platformjunit-platform-launcher

So, may be, let's do nothing for now?

I've had a think about it, and I agree with you now :)

The "The automatic loading of test framework implementation dependencies has been deprecated" warnings are only visible in build scans, they don't affect day-to-day development, and the problem will resolve itself in Kotlin 2.0.

So, I propose closing this PR, unmerged. If anyone disagrees, we can reopen it!

@adam-enko adam-enko requested a review from whyoleg March 11, 2024 10:16
@adam-enko adam-enko closed this Mar 18, 2024
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.

None yet

3 participants