fix: do not filter out advice to add to testImplementation if there i…

…s conflicting advice to downgrade from implementation.

src/functionalTest/groovy/com/autonomousapps/jvm/RuntimeOnlySpec.groovy

@@ -0,0 +1,34 @@
+package com.autonomousapps.jvm
+
+import com.autonomousapps.jvm.projects.ImplRuntimeTestImplConfusionProject
+import com.autonomousapps.utils.Colors
+
+import static com.autonomousapps.utils.Runner.build
+import static com.google.common.truth.Truth.assertThat
+
+final class RuntimeOnlySpec extends AbstractJvmSpec {
+
+  def "don't suggest implementation to runtimeOnly when used for testImplementation (#gradleVersion)"() {
+    given:
+    def project = new ImplRuntimeTestImplConfusionProject()
+    gradleProject = project.gradleProject
+
+    when:
+    def result = build(
+      gradleVersion, gradleProject.rootDir,
+      'buildHealth',
+      ':lib:reason', '--id', ImplRuntimeTestImplConfusionProject.SPARK_SQL_ID,
+    )
+
+    then: 'advice is correct'
+    assertThat(project.actualBuildHealth()).containsExactlyElementsIn(project.expectedBuildHealth)
+
+    and: 'reason makes sense'
+    def output = Colors.decolorize(result.output)
+    assertThat(output).contains('You have been advised to change this dependency to \'runtimeOnly\' from \'implementation\'.')
+    assertThat(output).contains('There is no advice regarding this dependency.')
+
+    where:
+    gradleVersion << gradleVersions()
+  }
+}

src/functionalTest/groovy/com/autonomousapps/jvm/projects/ImplRuntimeTestImplConfusionProject.groovy

@@ -0,0 +1,89 @@
+package com.autonomousapps.jvm.projects
+
+import com.autonomousapps.AbstractProject
+import com.autonomousapps.kit.GradleProject
+import com.autonomousapps.kit.Source
+import com.autonomousapps.model.Advice
+import com.autonomousapps.model.ProjectAdvice
+
+import static com.autonomousapps.AdviceHelper.actualProjectAdvice
+import static com.autonomousapps.AdviceHelper.emptyProjectAdviceFor
+import static com.autonomousapps.AdviceHelper.moduleCoordinates
+import static com.autonomousapps.AdviceHelper.projectAdviceForDependencies
+import static com.autonomousapps.kit.gradle.Dependency.implementation
+
+/**
+ * A dependency declared on {@code implementation} is visible to {@code testImplementation}, and so we don't typically
+ * advise to add a declaration on the latter. However, in the case where the dependency is not used in main source, and
+ * that dependency also provides runtime capabilities (such as a service loader), the algorithm must not suggest moving
+ * the declaration to {@code runtimeOnly} without also adding it to {@code testImplementation} (since the former is not
+ * visible to the latter). So, this is a case where a single declaration, combined with two usages (runtimeOnly and
+ * testImplementation), must result in two pieces of advice: <strong>change</strong> {@code implementation} ->
+ * {@code runtimeOnly} and <strong>add</strong> to {@code testImplementation}.
+ *
+ * @see <a href="https://docs.gradle.org/current/userguide/java_plugin.html#resolvable_configurations">Java configurations</a>
+ */
+final class ImplRuntimeTestImplConfusionProject extends AbstractProject {
+
+  final GradleProject gradleProject
+
+  static final SPARK_SQL_ID = "org.apache.spark:spark-sql_2.12"
+  private final spark = implementation("$SPARK_SQL_ID:3.5.0")
+
+  ImplRuntimeTestImplConfusionProject() {
+    this.gradleProject = build()
+  }
+
+  private GradleProject build() {
+    return newGradleProjectBuilder()
+      .withSubproject('lib') { s ->
+        s.sources = SOURCES
+        s.withBuildScript { bs ->
+          bs.plugins = javaLibrary
+          bs.dependencies(spark)
+        }
+      }
+      .write()
+  }
+
+  private static final List<Source> SOURCES = [
+    Source.java(
+      '''\
+        package com.example.lib;
+        
+        public class Lib {}
+      '''
+    )
+      .withPath('com.example.lib', 'Lib')
+      .build(),
+    Source.java(
+      '''\
+        package com.example.lib;
+        
+        import org.apache.spark.sql.SparkSession;
+        
+        public class LibTest {
+          private void test() {
+            SparkSession.builder().getOrCreate();
+          }
+        }
+      '''
+    )
+      .withPath('com.example.lib', 'LibTest')
+      .withSourceSet('test')
+      .build(),
+  ]
+
+  Set<ProjectAdvice> actualBuildHealth() {
+    return actualProjectAdvice(gradleProject)
+  }
+
+  private final Set<Advice> libAdvice = [
+    Advice.ofChange(moduleCoordinates(spark), 'implementation', 'runtimeOnly'),
+    Advice.ofAdd(moduleCoordinates(spark), 'testImplementation'),
+  ]
+
+  final Set<ProjectAdvice> expectedBuildHealth = [
+    projectAdviceForDependencies(':lib', libAdvice),
+  ]
+}

src/main/kotlin/com/autonomousapps/model/Advice.kt

@@ -10,7 +10,7 @@ import com.squareup.moshi.JsonClass
  * An "advice" is a kind of _transform_ that users ought to perform to bring their dependency declarations into a more
  * correct state.
  *
- * See also [Usage][com.autonomousapps.model.intermediates.Usage].
+ * See also [Usage][com.autonomousapps.model.internal.intermediates.Usage].
  */
 @JsonClass(generateAdapter = false)
 data class Advice(
@@ -70,8 +70,6 @@ data class Advice(
 
   fun isRuntimeOnly() = toConfiguration?.endsWith("runtimeOnly", ignoreCase = true) == true
 
-  fun isRemoveRuntimeOnly() = isRemove() && fromConfiguration?.endsWith("runtimeOnly", ignoreCase = true) == true
-
   /**
    * An advice is "add-advice" if it is undeclared and used, AND is not `compileOnly`.
    */
@@ -106,11 +104,8 @@ data class Advice(
     it.endsWith("kapt", ignoreCase = true) || it.endsWith("annotationProcessor", ignoreCase = true)
   }.isTrue()
 
-  /** If this is advice to remove or downgrade an api-like dependency. */
-  fun isDowngrade(): Boolean {
-    return (isRemove() || isChange() || isCompileOnly())
-      && fromConfiguration?.endsWith("api", ignoreCase = true) == true
-  }
+  /** If this is advice to remove or downgrade a dependency. */
+  fun isDowngrade(): Boolean = (isRemove() || isCompileOnly() || isRuntimeOnly())
 
   /** If this is advice to add a dependency, or change an existing dependency to make it api-like. */
   fun isUpgrade(): Boolean = isAnyAdd() || (isAnyChange() && isToApiLike())

src/main/kotlin/com/autonomousapps/transform/StandardTransform.kt

@@ -318,7 +318,7 @@ internal class StandardTransform(
     // => we need to remove that advice.
 
     return advice.asSequence()
-      .filterNot { isDeclaredInRelatedSourceSet(it) }
+      .filterNot { isDeclaredInRelatedSourceSet(advice, it) }
       .map { downgradeTestDependencies(it) }
       .toSet()
   }
@@ -334,8 +334,10 @@ internal class StandardTransform(
    *   // functionalTestImplementation will also "inherit" the 'foo:bar:1.0' dependency.
    * }
    * ```
+   *
+   * nb: returning false means "keep this advice."
    */
-  private fun isDeclaredInRelatedSourceSet(advice: Advice): Boolean {
+  private fun isDeclaredInRelatedSourceSet(allAdvice: Set<Advice>, advice: Advice): Boolean {
     if (!advice.isAnyAdd()) return false
 
     val sourceSetName = DependencyScope.sourceSetName(advice.toConfiguration!!)
@@ -349,8 +351,19 @@ internal class StandardTransform(
     // Unless it's api-like on a test source set, which makes no sense.
     if (advice.isToApiLike() && !isTestRelated) return false
 
+    // Instead of attempting a complex algorithm to get it "just right", if we see ANY downgrade advice, we bail out.
+    // This particular function is already an optimization to support a scenario we arguably shouldn't, leading to
+    // increasingly complex and brittle code. That is, it supports the special case that main deps are generally
+    // visible to test. Perhaps we should just stop doing that.
+    val anyDowngrade = allAdvice.any { it.isDowngrade() }
+    if (anyDowngrade) return false
+
     val sourceSets = directDependencies[advice.coordinates.identifier].map { it.variant }
 
+    // There's "no point" in adding a new declaration when that dependency is already available as a direct dependency,
+    // UNLESS we also happen to have some advice that might DOWNGRADE/REMOVE that declaration. For example, we might be
+    // about to advise the user to remove an `implementation` dependency, in which case the advice to also add a
+    // `testImplementation` dependency IS NOT redundant and SHOULD NOT be filtered out.
     return sourceSetName in sourceSets
   }