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

Bundles don't work for test dependencies, nor when there are multiple steps to transitive dependencies #425

Closed
gabrielittner opened this issue Jun 20, 2021 · 5 comments · Fixed by #441
Labels
bug Something isn't working
Milestone

Comments

@gabrielittner
Copy link
Contributor

Build scan link

Reproducer test-bundles.zip

Plugin version

0.74.0

Gradle version

7.0

(Optional) Android Gradle Plugin (AGP) version

4.2.1

Describe the bug

I'm depending on io.kotest:kotest-assertions-core-jvm in testImplementation but only use things from the transitive io.kotest:kotest-assertions-shared-jvm dependency.

I've declared a bundle like this:

dependencyAnalysis {
    dependencies {
        bundle("kotest-assertions") {
            includeDependency("io.kotest:kotest-assertions-core-jvm")
            includeDependency("io.kotest:kotest-assertions-shared-jvm")
        }
    }
}

When running buildHealth I get this advice

Advice for project :settings
Unused dependencies which should be removed:
- testImplementation("io.kotest:kotest-assertions-core-jvm:4.6.0")

Transitively used dependencies that should be declared directly as indicated:
- implementation("io.kotest:kotest-assertions-shared-jvm:4.6.0")

To Reproduce
Steps to reproduce the behavior:

  1. have a used transitive test dependency
  2. put that dependency into a bundle

Expected behavior

No advice is printed because both dependencies are part of a bundle.

@autonomousapps
Copy link
Owner

Thanks for the report. It looks like there's still work to be done supporting the test source set. It also shouldn't be saying to add that transitive to implementation, but instead testImplementation (I imagine).

@autonomousapps
Copy link
Owner

autonomousapps commented Jul 20, 2021

I am working on this right now. There are actually three bugs.

  1. Dependency bundles don't take into account multiple steps between dependencies. As currently implemented, they only look at immediate transitive dependencies (which is not how I envisioned the feature to work). In between your two dependencies is io.kotest:kotest-assertions-shared. There is no workaround right now because of the next reason.
  2. Frankly, the test-dependency support is a bit half-baked at the moment, and I'm working to get it to the same level as the "main" dependency analysis. So even if you were to add io.kotest:kotest-assertions-shared, it wouldn't be good enough because I'm not currently walking the test compile classpath. I have a WIP that adds this.
  3. The reason the advice is to add that test dependency to implementation instead of testImplementation is that the test class you have in your reproducer has the same name as a class in the main source set. One of the factors in how the plugin decides which configuration a dependency is on is the source set of the associated source code that uses the dependency. Having the same class name in main and test source sets confuses it. The workaround here is to change one of the two names. The canonical approach is to add the suffix Test to the test class. Fixing this bug is not a priority at this time, since frankly it seems like a pain in the butt :)

I expect to have a fix for the first two bugs in the near-ish future.

@autonomousapps autonomousapps changed the title Bundles don't seem to work for test dependencies Bundles don't work for test dependencies, nor when there are multiple steps to transitive dependencies Jul 20, 2021
@gabrielittner
Copy link
Contributor Author

  1. It sounds fine to me to keep it like it currently is. I think it's only using the same class name because I copy pasted the main class to create the test class for the reproducer.

@autonomousapps
Copy link
Owner

Fixed. You'll be able to use the snapshot once it gets published, but if you want to wait, this will be in the next release.

@gabrielittner
Copy link
Contributor Author

Thanks, this is great to hear, looking forward to the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants