Don't suggest adding a (test) project dependency on the project itself (since 1.23.1)

[gabrielittner]

trafficstars

Build scan link

https://scans.gradle.com/s/qoumnifidl3h6

Project is open source, you can see the failure on this pr https://github.com/freeletics/khonshu/pull/554

Plugin version

1.23.1

Gradle version

8.3

(Optional) Android Gradle Plugin (AGP) version

8.1.2

reason output for bugs relating to incorrect advice

./gradlew :codegen-compiler:reason --id=:codegen-compiler
----------------------------------------
You asked about the dependency ':codegen-compiler'.
You have been advised to add this dependency to 'testImplementation'.
----------------------------------------

Shortest path from :codegen-compiler to :codegen-compiler for compileClasspath:
:codegen-compiler

Shortest path from :codegen-compiler to :codegen-compiler for runtimeClasspath:
:codegen-compiler

Shortest path from :codegen-compiler to :codegen-compiler for testCompileClasspath:
:codegen-compiler

Shortest path from :codegen-compiler to :codegen-compiler for testRuntimeClasspath:
:codegen-compiler

Shortest path from :codegen-compiler to :codegen-compiler for testFixturesCompileClasspath:
:codegen-compiler

Shortest path from :codegen-compiler to :codegen-compiler for testFixturesRuntimeClasspath:
:codegen-compiler

Source: main
------------
(no usages)

Source: test
------------
(no usages)

Source: testFixtures
--------------------
(no usages)

Describe the bug

Execution failed for task ':buildHealth'.
> Advice for :codegen-compiler
  These transitive dependencies should be declared directly:
    testImplementation(project(":codegen-compiler"))

I believe this happens if the tests of a project are using the testFixtures defined in the same project which then leads the plugin to believe that the shortest path from the test source set to the main source set goes through the test fixture source set even though no dependency is needed.

To Reproduce

1. Define test fixtures in a project
2. Use the test fixture in the test source set

Expected behavior

No advice is given

[autonomousapps]

Thanks for the issue.

[autonomousapps]

Can confirm that I can repro this locally, having checked out the branch on your repo.

[gabrielittner]

Thanks for taking a look, I just noticed that this doesn't happen anymore with 1.24.0 https://github.com/freeletics/khonshu/pull/558

[autonomousapps]

Oh, great! Can we close this then?

[gabrielittner]

Yes, just clicked the wrong comment button

[gabrielittner]

It actually still happens with 1.24.0. Renovate bot was weird and only updated the sample app in the project which didn't have the issue. Here is the pr that updates the main part to 1.24 https://github.com/freeletics/khonshu/pull/560

[jjohannes]

Technically, this advice is most likely correct. When you apply the test-fixtures plugin, Gradle rewires the setup (I assume AGP does it similarly). You now have something like:

test --depends-on--> test-fixtures --depends-on--> main

If we look at these things as different variants of one project, as soon as you directly refer to classes in "main" from "test" you should declare a dependency. If we strictly follow the rules.

I can however understand that this seems unintuitive. Especially, because there is special handling of "test" already (#900), which is most likely the reason why this advice was not seen before the support for "test-fixtures" was added. So there could be a special handling for this case added to the plugin. (But should there be even more special handlings?)

You can ignore this kind of advices in your project. For example by doing this:

dependencyAnalysis.issues { onAny { exclude(project.path) } }

I wonder if it would be enough to document that.

[autonomousapps]

Jendrik, thanks for that detailed explanation. I like to balance strictness with ergonomics, and since Gradle is unlikely to ever require users to add a dependency from the test source to the main source, I think this plugin should respect that as well. It might be enough to add that exclusion globally as you suggested in your comment, or maybe we can do something "deeper". I'm happy to take this on with the help of the reproducer provided.

[autonomousapps]

@gabrielittner can you re-test with 1.26.0?

[gabrielittner]

It's still reproducible. You can test by removing this line here https://github.com/freeletics/khonshu/blob/main/codegen-compiler/codegen-compiler.gradle.kts#L36-L37

[ianbrandt]

With 1.27.0 as well.

  Advice for :tdd-in-kotlin
  These transitive dependencies should be declared directly:
    testImplementation(project(":tdd-in-kotlin"))

https://github.com/sdkotlin/sd-kotlin-talks/tree/a7571fc464831fc23124e52cd8ce0fc445339326

[ianbrandt]

I'd vote for special handling. Otherwise it's, "add the testImplementation dependency from the project to itself if you use the core Test Fixtures Plugin, but don't if you don't."

Breaking from the convention of the default "test" suite, the JVM Test Suite Plugin does require adding an explicit dependency to project outputs, though I haven't noticed this plugin advising testSuiteNameImplementation(project(":self")) when doing so.

If memory serves, I also don't believe any advise is given when the "test" suite of one project refers to the "testFixtures" of another.

[jjohannes]

The special handling of how the dependency scopes of "test" and "main" are glued together has history – orginating in how the scopes compile and test in Maven behave. There are discussions to change this (since quite some time) in Gradle (https://github.com/gradle/gradle/issues/6483) so that the test test suite becomes less special.

But from a user perspective, I agree that the special handling is probably a good idea. Although ideally I would like to see this being configurable (see #900) to satisfy different use cases.

If memory serves, I also don't believe any advise is given when the "test" suite of one project refers to the "testFixtures" of another.

This should work with the current version (#854).

[ianbrandt]

Just FYI, with 1.30.0 I had a couple projects where even if I added the self references for the advised configurations verbatim, the plugin still listed them as undeclared, transitively used dependencies. I had to apply the exclude(project.path) workaround to get past it. I haven't gotten around to creating a reproducer for the issue, but I just retested with 1.31.0, and I no longer need the exclude. Whatever the cause for that additional issue was, it seems to have been addressed.

[autonomousapps]

I ran into this issue on one of my projects and resolved it here. I'm going to consider this issue closed.