wip/dnm.

autonomousapps · Oct 15, 2024 · eba7090 · eba7090
1 parent f7a6ce5
commit eba7090
Show file tree

Hide file tree

Showing 13 changed files with 294 additions and 51 deletions.
diff --git a/src/functionalTest/groovy/com/autonomousapps/AbstractProject.groovy b/src/functionalTest/groovy/com/autonomousapps/AbstractProject.groovy
@@ -6,6 +6,7 @@ import com.autonomousapps.kit.AbstractGradleProject
 import com.autonomousapps.kit.GradleProject
 import com.autonomousapps.kit.gradle.GradleProperties
 import com.autonomousapps.kit.gradle.Plugin
+import com.autonomousapps.kit.gradle.dependencies.PluginProvider
 import com.autonomousapps.kit.gradle.dependencies.Plugins
 import com.autonomousapps.utils.DebugAware
 
@@ -24,6 +25,26 @@ abstract class AbstractProject extends AbstractGradleProject {
   /** Applies the 'java-library' and 'com.autonomousapps.dependency-analysis' plugins. */
   protected static final List<Plugin> javaLibrary = [Plugin.javaLibrary, Plugins.dependencyAnalysisNoVersion]
 
+  protected final PluginProvider pluginProvider
+
+  static String getKotlinVersion() {
+    return System.getProperty("com.autonomousapps.test.versions.kotlin")
+  }
+
+  AbstractProject() {
+    this(getKotlinVersion(), null)
+  }
+
+  AbstractProject(
+    String kotlinVersion,
+    String agpVersion
+  ) {
+    pluginProvider = new PluginProvider(
+      kotlinVersion,
+      agpVersion,
+    )
+  }
+
   @Override
   protected GradleProject.Builder newGradleProjectBuilder(
     GradleProject.DslKind dslKind = GradleProject.DslKind.GROOVY

diff --git a/src/functionalTest/groovy/com/autonomousapps/android/projects/AbstractAndroidProject.groovy b/src/functionalTest/groovy/com/autonomousapps/android/projects/AbstractAndroidProject.groovy
@@ -25,15 +25,12 @@ abstract class AbstractAndroidProject extends AbstractProject {
 
   protected final String agpVersion
   protected final AgpVersion version
-  protected final PluginProvider pluginProvider
 
   AbstractAndroidProject(String agpVersion) {
+    super(getKotlinVersion(), agpVersion)
+
     this.agpVersion = agpVersion
     this.version = AgpVersion.version(agpVersion)
-    this.pluginProvider = new PluginProvider(
-      System.getProperty("com.autonomousapps.test.versions.kotlin"), // TODO: inject
-      agpVersion,
-    )
   }
 
   protected AndroidBlock defaultAndroidAppBlock(

diff --git a/src/functionalTest/groovy/com/autonomousapps/jvm/CustomSourceSetSpec.groovy b/src/functionalTest/groovy/com/autonomousapps/jvm/CustomSourceSetSpec.groovy
@@ -191,13 +191,13 @@ final class CustomSourceSetSpec extends AbstractJvmSpec {
     )
   }
 
-  def "don't suggest moving a dependency from one feature variant to another"() {
+  def "don't suggest moving a dependency from one feature variant to another (#gradleVersion)"() {
     given:
     def project = new FeatureVariantInConsumerTestProject()
     gradleProject = project.gradleProject
 
     when:
-    build(gradleVersion, gradleProject.rootDir, 'buildHealth')
+    build(gradleVersion, gradleProject.rootDir, 'consumer:dependencies', 'buildHealth')
 
     then:
     assertThat(project.actualBuildHealth()).containsExactlyElementsIn(project.expectedBuildHealth)

diff --git a/src/functionalTest/groovy/com/autonomousapps/jvm/Kotlin2MigrationSpec.groovy b/src/functionalTest/groovy/com/autonomousapps/jvm/Kotlin2MigrationSpec.groovy
@@ -0,0 +1,24 @@
+package com.autonomousapps.jvm
+
+import com.autonomousapps.jvm.projects.Kotlin2Migration
+
+import static com.autonomousapps.utils.Runner.build
+import static com.google.common.truth.Truth.assertThat
+
+final class Kotlin2MigrationSpec extends AbstractJvmSpec {
+
+  def "suggests declaring testFixtures dependencies (#gradleVersion)"() {
+    given:
+    def project = new Kotlin2Migration.DeclaresTestFixturesDependencies()
+    gradleProject = project.gradleProject
+
+    when:
+    build(gradleVersion, gradleProject.rootDir, 'buildHealth')
+
+    then:
+    assertThat(project.actualBuildHealth()).containsExactlyElementsIn(project.expectedBuildHealth)
+
+    where:
+    gradleVersion << gradleVersions()
+  }
+}
diff --git a/src/functionalTest/groovy/com/autonomousapps/jvm/projects/CustomTestSourceSetProject.groovy b/src/functionalTest/groovy/com/autonomousapps/jvm/projects/CustomTestSourceSetProject.groovy
@@ -31,6 +31,23 @@ final class CustomTestSourceSetProject extends AbstractProject {
 
   private GradleProject build() {
     return newGradleProjectBuilder()
+      .withRootProject { r ->
+        r.withBuildScript { bs ->
+          // TODO(tsr): should we just exclude every source set with the "Test" suffix from ABI analysis?
+          bs.withGroovy(
+            // Kotlin still adds everything to `<custom source set>Api`, annoyingly.
+            '''\
+              dependencyAnalysis {
+                abi {
+                  exclusions {
+                    excludeSourceSets('functionalTest')
+                  }
+                }
+              }
+            '''
+          )
+        }
+      }
       .withSubproject('proj') { s ->
         s.sources = source()
         s.withBuildScript { bs ->

diff --git a/...nalTest/groovy/com/autonomousapps/jvm/projects/FeatureVariantInConsumerTestProject.groovy b/...nalTest/groovy/com/autonomousapps/jvm/projects/FeatureVariantInConsumerTestProject.groovy
@@ -6,7 +6,6 @@ import com.autonomousapps.AbstractProject
 import com.autonomousapps.kit.GradleProject
 import com.autonomousapps.kit.Source
 import com.autonomousapps.kit.SourceType
-import com.autonomousapps.kit.gradle.Feature
 import com.autonomousapps.kit.gradle.Java
 import com.autonomousapps.kit.gradle.Plugin
 import com.autonomousapps.model.Advice
@@ -24,27 +23,26 @@ final class FeatureVariantInConsumerTestProject extends AbstractProject {
   }
 
   private GradleProject build() {
-    def builder = newGradleProjectBuilder()
-    builder.withSubproject('producer') { s ->
-      s.sources = producerSources
-      s.withBuildScript { bs ->
-        bs.plugins = javaLibrary
+    return newGradleProjectBuilder()
+      .withSubproject('producer') { s ->
+        s.sources = producerSources
+        s.withBuildScript { bs ->
+          bs.plugins = javaLibrary
+        }
       }
-    }
-    builder.withSubproject('consumer') { s ->
-      s.sources = consumerSources
-      s.withBuildScript { bs ->
-        bs.plugins = javaLibrary + Plugin.javaTestFixtures
-        bs.java = Java.ofFeatures(Feature.ofName('extra'))
-        bs.dependencies = [
-          project('api', ':producer'),
-          project('testFixturesImplementation', ':producer'),
-          project('extraImplementation', ':consumer')
-        ]
+      .withSubproject('consumer') { s ->
+        s.sources = consumerSources
+        s.withBuildScript { bs ->
+          bs.plugins = javaLibrary + Plugin.javaTestFixtures
+          bs.java = Java.ofFeatures('extra')
+          bs.dependencies = [
+            project('api', ':producer'),
+            project('testFixturesImplementation', ':producer'),
+            project('extraImplementation', ':consumer')
+          ]
+        }
       }
-    }
-
-    return builder.write()
+      .write()
   }
 
   private producerSources = [
@@ -95,19 +93,15 @@ final class FeatureVariantInConsumerTestProject extends AbstractProject {
           private Producer p;
           private Consumer c;
         }""".stripIndent()
-    , "extra")
+      , "extra")
   ]
 
   Set<ProjectAdvice> actualBuildHealth() {
     return actualProjectAdvice(gradleProject)
   }
 
-  private final Set<Advice> expectedConsumerAdvice = [
-    Advice.ofAdd(projectCoordinates(':producer'), 'extraImplementation')
-  ]
-
   final Set<ProjectAdvice> expectedBuildHealth = [
-    projectAdviceForDependencies(':consumer', expectedConsumerAdvice),
-    projectAdviceForDependencies(':producer', [] as Set)
+    emptyProjectAdviceFor(':consumer'),
+    emptyProjectAdviceFor(':producer')
   ]
 }
diff --git a/src/functionalTest/groovy/com/autonomousapps/jvm/projects/Kotlin2Migration.groovy b/src/functionalTest/groovy/com/autonomousapps/jvm/projects/Kotlin2Migration.groovy
@@ -0,0 +1,113 @@
+package com.autonomousapps.jvm.projects
+
+import com.autonomousapps.AbstractProject
+import com.autonomousapps.kit.GradleProject
+import com.autonomousapps.kit.Source
+import com.autonomousapps.model.ProjectAdvice
+
+import static com.autonomousapps.AdviceHelper.actualProjectAdvice
+import static com.autonomousapps.AdviceHelper.emptyProjectAdviceFor
+import static com.autonomousapps.kit.gradle.Dependency.implementation
+import static com.autonomousapps.kit.gradle.Dependency.testFixturesImplementation
+
+abstract class Kotlin2Migration extends AbstractProject {
+
+  Kotlin2Migration() {
+    super()
+  }
+
+  static final class DeclaresTestFixturesDependencies extends Kotlin2Migration {
+
+    final GradleProject gradleProject
+
+    DeclaresTestFixturesDependencies() {
+      super()
+      this.gradleProject = build()
+    }
+
+    private GradleProject build() {
+      return newGradleProjectBuilder()
+        .withSubproject('consumer') { s ->
+          s.sources = sourcesConsumer
+          s.withBuildScript { bs ->
+            bs.plugins = kotlin + pluginProvider.javaTestFixtures
+            bs.dependencies(
+              implementation(':producer'),
+              implementation(':producer').testFixtures(),
+//              testFixturesImplementation(':producer'),
+            )
+          }
+        }
+        .withSubproject('producer') { s ->
+          s.sources = sourcesProducer
+          s.withBuildScript { bs ->
+            bs.plugins = kotlin + pluginProvider.javaTestFixtures
+          }
+        }
+        .write()
+    }
+
+    private List<Source> sourcesConsumer = [
+      Source.kotlin(
+        '''\
+          package com.example.consumer
+          
+          import com.example.producer.Simpsons.HOMER
+
+          class Consumer {
+            private val homer = HOMER
+          }
+        '''
+      )
+        .withPath('com.example.consumer', 'Consumer')
+        .build(),
+      Source.kotlin(
+        '''\
+          package com.example.consumer
+          
+          import com.example.producer.Person
+
+          class TestFixture {
+            val testPerson = Person("Emma", "Goldman")
+          }
+        '''
+      )
+        .withPath('com.example.consumer', 'TestFixture')
+        .withSourceSet('testFixtures')
+        .build(),
+    ]
+
+    private List<Source> sourcesProducer = [
+      Source.kotlin(
+        '''\
+          package com.example.producer
+
+          data class Person(val firstName: String, val lastName: String)
+        '''
+      )
+        .withPath('com.example.producer', 'Person')
+        .build(),
+      Source.kotlin(
+        '''\
+          package com.example.producer
+
+          object Simpsons {
+            val HOMER = Person("Homer", "Simpson")
+          }
+        '''
+      )
+        .withPath('com.example.producer', 'Simpsons')
+        .withSourceSet('testFixtures')
+        .build(),
+    ]
+
+    Set<ProjectAdvice> actualBuildHealth() {
+      return actualProjectAdvice(gradleProject)
+    }
+
+    final Set<ProjectAdvice> expectedBuildHealth = [
+      emptyProjectAdviceFor(':consumer'),
+      emptyProjectAdviceFor(':producer'),
+    ]
+  }
+}
diff --git a/src/functionalTest/kotlin/com/autonomousapps/kit/gradle/dependencies/PluginProvider.kt b/src/functionalTest/kotlin/com/autonomousapps/kit/gradle/dependencies/PluginProvider.kt
@@ -32,4 +32,10 @@ class PluginProvider(
   val kotlinKaptNoVersion: Plugin = Plugin("org.jetbrains.kotlin.kapt")
 
   val springBoot: Plugin = Plugin("org.springframework.boot", springBootVersion)
+
+  /*
+   * Core plugins. This layer exists for ease of writing test fixtures.
+   */
+
+  val javaTestFixtures: Plugin = Plugin.javaTestFixtures
 }
diff --git a/src/main/kotlin/com/autonomousapps/tasks/ComputeAdviceTask.kt b/src/main/kotlin/com/autonomousapps/tasks/ComputeAdviceTask.kt
@@ -159,7 +159,7 @@ abstract class ComputeAdviceTask @Inject constructor(
       val annotationProcessorUsages = usageBuilder.annotationProcessingUsages
       val supportedSourceSets = parameters.supportedSourceSets.get()
       val isKaptApplied = parameters.kapt.get()
-      val nonTransitiveDependencies = computeNonTransitiveDependenciesMap(dependencyGraph)
+      val directDependencies = computeDirectDependenciesMap(dependencyGraph)
 
       val ignoreKtx = parameters.ignoreKtx.get()
 
@@ -178,7 +178,7 @@ abstract class ComputeAdviceTask @Inject constructor(
         dependencyUsages = dependencyUsages,
         annotationProcessorUsages = annotationProcessorUsages,
         declarations = declarations,
-        nonTransitiveDependencies = nonTransitiveDependencies,
+        directDependencies = directDependencies,
         supportedSourceSets = supportedSourceSets,
         isKaptApplied = isKaptApplied,
       )
@@ -206,16 +206,25 @@ abstract class ComputeAdviceTask @Inject constructor(
     /**
      * Returns the set of non-transitive dependencies from [dependencyGraph], associated with the source sets
      * ([Variant.variant][com.autonomousapps.model.declaration.Variant.variant]) they're used by.
+     *
+     * These are _direct_ dependencies that are not _declared_ because they're coming from associated classpaths. For
+     * example, the `test` source set extends from the `main` source set (and also the compile and runtime classpaths).
      */
-    private fun computeNonTransitiveDependenciesMap(
+    private fun computeDirectDependenciesMap(
       dependencyGraph: Map<String, DependencyGraphView>,
     ): SetMultimap<String, Variant> {
       return newSetMultimap<String, Variant>().apply {
         dependencyGraph.values.map { graphView ->
           val root = graphView.graph.root()
-          graphView.graph.children(root).forEach { nonTransitiveDependency ->
-            // TODO: just identifier and not gav()?
-            put(nonTransitiveDependency.identifier, graphView.variant)
+          graphView.graph.children(root).forEach { directDependency ->
+            val identifier = if (directDependency is IncludedBuildCoordinates) {
+              // An attempt to normalize the identifier
+              directDependency.resolvedProject.identifier
+            } else {
+              // TODO: just identifier and not gav()?
+              directDependency.identifier
+            }
+            put(identifier, graphView.variant)
           }
         }
       }
@@ -257,7 +266,7 @@ internal class DependencyAdviceBuilder(
   private val dependencyUsages: Map<Coordinates, Set<Usage>>,
   private val annotationProcessorUsages: Map<Coordinates, Set<Usage>>,
   private val declarations: Set<Declaration>,
-  private val nonTransitiveDependencies: SetMultimap<String, Variant>,
+  private val directDependencies: SetMultimap<String, Variant>,
   private val supportedSourceSets: Set<String>,
   private val isKaptApplied: Boolean,
 ) {
@@ -292,7 +301,7 @@ internal class DependencyAdviceBuilder(
 
     return dependencyUsages.asSequence()
       .flatMap { (coordinates, usages) ->
-        StandardTransform(coordinates, declarations, nonTransitiveDependencies, supportedSourceSets, buildPath)
+        StandardTransform(coordinates, declarations, directDependencies, supportedSourceSets, buildPath)
           .reduce(usages)
           .map { advice -> advice to coordinates }
       }
@@ -348,7 +357,7 @@ internal class DependencyAdviceBuilder(
         StandardTransform(
           coordinates = coordinates,
           declarations = declarations,
-          nonTransitiveDependencies = emptySetMultimap(),
+          directDependencies = emptySetMultimap(),
           supportedSourceSets = supportedSourceSets,
           buildPath = buildPath,
           isKaptApplied = isKaptApplied