Support testFixtures

[Mahoney]

Build scan link https://scans.gradle.com/s/z6637ifi3x7ha

Plugin version 0.61.0

Gradle version 6.6.1

Android Gradle Plugin (AGP) version N/A

Describe the bug Getting a failure claiming I have an unused dependency that I think is invalid - possibly due to the plugin not knowing about the testFixtures configuration? Or not knowing about how kotlin works with top level functions? Specifically, it reports:

> Task :auction-xmpp-integration-tests:aggregateAdvice
Unused dependencies which should be removed:
- implementation(project(":auction-xmpp"))

and advice-all-variants-pretty.json contains:

{
  "dependencyAdvice": [
    {
      "dependency": {
        "identifier": ":auction-xmpp",
        "configurationName": "implementation"
      },
      "usedTransitiveDependencies": [
        {
          "identifier": "org.jetbrains:annotations",
          "resolvedVersion": "13.0"
        }
      ],
      "fromConfiguration": "implementation"
    }
  ],
  "pluginAdvice": [],
  "shouldFail": true
}

but xmpp-integration-tests doesn't depend on project(":auction-xmpp"), it depends on its testFixtures: xmpp-integration-tests/build.gradle.kts#L13

implementation(testFixtures(project(":auction-xmpp")))

which it definitely uses: xmpp-integration-tests/src/XMPPAuctionTest.kt#L3 To Reproduce Steps to reproduce the behavior:

git clone [email protected]:Mahoney-playground/goos.git && \
cd goos && \
git checkout stable-gradle-version && \
./gradlew build

Expected behavior I think the build should pass

Additional context Info on testFixtures: https://docs.gradle.org/current/userguide/java_testing.html#sec:java_test_fixtures

Output of ./gradlew :auction-xmpp-integration-tests:reason --id :auction-xmpp:

> Task :auction-xmpp-integration-tests:reason
You asked about the dependency :auction-xmpp. You have been advised to remove this dependency.

Shortest path to :auction-xmpp from the current project:
:auction-xmpp-integration-tests
\--- :auction-xmpp

Dependency :auction-xmpp provides the following:
- 5 classes
- 2 public constants

And none of these are used.
Reason DOT: /Users/Robert/Workspaces/goos/appSrc/auction/xmpp-integration-tests/build/reports/dependency-analysis/graph-reason.gv

Contents of graph-reason.gv:

strict digraph DependencyGraph {

  ":auction-xmpp-integration-tests" [style=filled fillcolor="#008080"];

  ":auction-xmpp" [style=filled fillcolor="#008080"];

  ":auction-xmpp-integration-tests" -> "io.kotest:kotest-framework-api-jvm";
  ":auction-xmpp-integration-tests" -> "org.jetbrains.kotlin:kotlin-stdlib";
  ":auction-xmpp-integration-tests" -> "org.jetbrains.kotlinx:kotlinx-coroutines-core";
  ":auction-xmpp-integration-tests" -> ":testlauncher";
  ":auction-xmpp-integration-tests" -> ":auction-xmpp" [style=bold color="#FF6347" weight=8];
  ":auction-xmpp-integration-tests" -> "io.kotest:kotest-framework-api";
  "io.kotest:kotest-framework-api-jvm" -> "org.jetbrains.kotlin:kotlin-stdlib-jdk8";
  "io.kotest:kotest-framework-api-jvm" -> "org.jetbrains.kotlin:kotlin-stdlib-common";
  "org.jetbrains.kotlin:kotlin-stdlib" -> "org.jetbrains.kotlin:kotlin-stdlib-common";
  "org.jetbrains.kotlin:kotlin-stdlib" -> "org.jetbrains:annotations";
  "org.jetbrains.kotlinx:kotlinx-coroutines-core" -> "org.jetbrains.kotlinx:kotlinx-coroutines-core-jvm";
  ":auction-xmpp" -> ":auction-xmpp";
  ":auction-xmpp" -> ":auction-api";
  ":auction-xmpp" -> "org.jetbrains.kotlin:kotlin-stdlib";
  ":auction-xmpp" -> "org.igniterealtime.smack:smack-core";
  ":auction-xmpp" -> "org.igniterealtime.smack:smack-im";
  "io.kotest:kotest-framework-api" -> "io.kotest:kotest-framework-api-jvm";
  "org.jetbrains.kotlin:kotlin-stdlib-jdk8" -> "org.jetbrains.kotlin:kotlin-stdlib";
  "org.jetbrains.kotlin:kotlin-stdlib-jdk8" -> "org.jetbrains.kotlin:kotlin-stdlib-jdk7";
  "org.jetbrains.kotlinx:kotlinx-coroutines-core-jvm" -> "org.jetbrains.kotlin:kotlin-stdlib";
  "org.jetbrains.kotlinx:kotlinx-coroutines-core-jvm" -> "org.jetbrains.kotlin:kotlin-stdlib-common";
  ":auction-api" -> ":auction-api";
  ":auction-api" -> "io.kotest:kotest-framework-api-jvm";
  "org.igniterealtime.smack:smack-core" -> "xpp3:xpp3";
  "org.igniterealtime.smack:smack-core" -> "org.jxmpp:jxmpp-core";
  "org.igniterealtime.smack:smack-core" -> "org.jxmpp:jxmpp-jid";
  "org.igniterealtime.smack:smack-core" -> "org.minidns:minidns-core";
  "org.igniterealtime.smack:smack-im" -> "org.igniterealtime.smack:smack-core";
  "org.jetbrains.kotlin:kotlin-stdlib-jdk7" -> "org.jetbrains.kotlin:kotlin-stdlib";
  "org.jxmpp:jxmpp-core" -> "org.jxmpp:jxmpp-util-cache";
  "org.jxmpp:jxmpp-jid" -> "org.jxmpp:jxmpp-core";
  "org.jxmpp:jxmpp-jid" -> "org.jxmpp:jxmpp-util-cache";
}

[autonomousapps]

You might be right about test fixtures. This plugin currently makes no accommodation for that.

You can get some insight into why the plugin is recommending something by eg:

./gradlew module:reason --id <identifier>

I'd give you an example for <identifier>, but you didn't tell me which dependency had been incorrectly declared unused :)

[Mahoney]

Yes, sorry, that was a rubbish bug report - rushing to get to bed. Added the obvious extra details.

[Mahoney]

I realise I've muddied the waters by using a snapshot version of gradle, but I see exactly the same behaviour reverting to gradle 6.6.1.

[Mahoney]

Updated the bug report to a branch using gradle 6.6.1 - same behaviour.

[autonomousapps]

Thanks, appreciated. I think this must be due to the plugin having no support yet for testFixtures. I will update this ticket to be a "feature request" to support that gradle feature.

For now, I think you can configure the plugin to ignore this "unused" dependency, following guidance in the wiki at https://github.com/autonomousapps/dependency-analysis-android-gradle-plugin/wiki/Customizing-plugin-behavior

[daanschipper]

I've done some more investigation into this issue. The locateDependenciesMain task does not take the capabilities of a project dependency into account when determining all declared dependencies. Some information about a dependency is lost when iterating over all the dependencies and mapping it to a set in ConfigurationsToDependenciesTransformer, only the identifier and configuration of a dependency is saved.

This causes issues when having dependencies declared as follows

project(':a') {
  plugins {
    id 'java-library'
    id 'java-test-fixtures'
  }
}

project(':b') {
  plugins {
     id 'java-library'
  }
  dependencies {
    testImplementation project(":a")    
    testImplementation testFixtures(project(":a"))
  }
}

only one 'location' is saved.

  {
    "identifier": ":a",
    "configurationName": "testImplementation",
    "isInteresting": true
  },

What is interesting, artifactsReportMain is able to locate the correct physical jars, but again the capability information is lost. This causes confusing reports down the line, e.g. the initial issue report.

  {
    "dependency": {
      "identifier": ":a",
      "configurationName": "testImplementation"
    },
    "isTransitive": false,
    "file": "<workspace>/a/.build/libs/a.jar"
  },
    {
    "dependency": {
      "identifier": ":a",
      "configurationName": "testImplementation"
    },
    "isTransitive": false,
    "file": "<workspace>/a/.build/libs/a-test-fixtures.jar"
  },

A potential solution would be to extend the Location data class with an additional field for the requested capabilities. By default this would be an empty list, but in case of testFixtures it would be (as you might expect) a list with one element testFixtures. Before diving further into this and start implementing the change, what do you think about this?

[autonomousapps]

@daanschipper thank you for the detailed note. I have given it some thought and I think you're on the right track, at least to the extent that it certainly seems like the Location class needs to encapsulate more information. That would only be a first step, of course. If you want to pursue this idea, feel free to file a PR and we can discuss further.

[autonomousapps]

FYI I am currently working on a massive redesign of the plugin, and as part of that I'm reworking the "location" code, and it will include an attributes: Set<String> (or something similar) to capture this information.

[autonomousapps]

Hey @jjohannes any thoughts on how we might support testFixtures?

[jjohannes]

@autonomousapps I would have to look into the code again for concrete implementation suggestions (which I can do soonish, if you plan to work on this).

General thoughts:

I would attempt to support arbitrary additional source sets and feature variants. Test fixtures uses this and has nothing 'special' on top from the dependency analysis perspective.

Then there are two aspects:

1. Dependencies from an additional source set - e.g. testFixtureImplementation("commons-io:commons-io:2.6")
2. Dependencies to an additional feature variant - e.g. implementation(testFixtures(project(":module-a")))

Note that implementation(testFixtures(project(":module-a"))) is just a shortcut for

implementation(project(":module-a")) {
  capabilities { requireCapability("my.group:module-a-test-fixtures") }
}

which would be the notation you can use for any feature variant. (For those following this issue and are interested in more details, I explained some of it here: https://youtu.be/XCzyUESaBHQ)

It might probably be a good idea to implement these two aspected one after the other. I think that (1) is easier to implement, because there we do not need more information about which aritfacts (Jars/classes) to select on the "other side". We continue to look at the "main" Jar(s) only. Solving this, would already be a huge benefit. Not only for dependencies you declare from testFixtures, but also from other additional source sets (like integrationTest).

For (2) we would need to make the connection between the capability you select in the dependency declaration and the aritfact(s) that resolves to on the "other side". Which in the test fixture example would not be module-a.jar, but module-a-test-fixtures.jar.

I once added these test cases that supposedly make sure that both cases do not lead to wrong suggestions. They, and the changes made when they were introduced, could be a good starting point: https://github.com/autonomousapps/dependency-analysis-android-gradle-plugin/blob/main/src/functionalTest/groovy/com/autonomousapps/jvm/CustomSourceSetSpec.groovy

[jjohannes]

Additional note: I am not sure if everything I wrote also applies for the Android testFixtures. I haven't looked at how that's done in AGP yet.

[autonomousapps]

This issue feels like the largest remaining gap in what this plugin can do. So, I think it's a good candidate as a next feature. That said, I'm extremely busy at work and don't know when I'll have time to dig into it.

[jjohannes]

This issue feels like the largest remaining gap in what this plugin can do.

I agree and I would also love to contribute. If I find some time, I'll have a closer look and see if I can get started with something.

[Mahoney]

Fantastic, thanks so much @jjohannes and @autonomousapps ! Looking forward to trying it out when a release is cut.