Display nice type names consistently for validation warnings

Also use the new expectDeprecationWarning(String) to check for emitted runtime validation warnings.
dmvk · Sep 25, 2019 · dfbf9b3 · dfbf9b3
1 parent b892365
commit dfbf9b3
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 23 deletions.
diff --git a/subprojects/execution/src/main/java/org/gradle/internal/execution/steps/ValidateStep.java b/subprojects/execution/src/main/java/org/gradle/internal/execution/steps/ValidateStep.java
@@ -17,6 +17,7 @@
 package org.gradle.internal.execution.steps;
 
 import com.google.common.collect.ImmutableCollection;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMultimap;
 import org.gradle.api.InvalidUserDataException;
 import org.gradle.internal.execution.Context;
@@ -28,7 +29,6 @@
 import org.gradle.internal.reflect.TypeValidationContext;
 import org.gradle.internal.reflect.TypeValidationContext.Severity;
 
-import java.util.List;
 import java.util.stream.Collectors;
 
 public class ValidateStep<C extends Context, R extends Result> implements Step<C, R> {
@@ -55,29 +55,41 @@ public R execute(C context) {
         warnings.forEach(warningReporter::reportValidationWarning);
 
         if (!errors.isEmpty()) {
-            String displayName = context.getWork().getDisplayName();
-            String errorMessage = errors.size() == 1
-                ? String.format("A problem was found with the configuration of %s.", displayName)
-                : String.format("Some problems were found with the configuration of %s.", displayName);
-            List<InvalidUserDataException> causes = errors.stream()
-                .limit(5)
-                .sorted()
-                .map(InvalidUserDataException::new)
-                .collect(Collectors.toList());
-            throw new WorkValidationException(errorMessage, causes);
+            throw new WorkValidationException(
+                String.format("%s found with the configuration of %s (%s).",
+                    errors.size() == 1
+                        ? "A problem was"
+                        : "Some problems were",
+                    context.getWork().getDisplayName(),
+                    describeTypesChecked(validationContext.getTypes())
+                ),
+                errors.stream()
+                    .limit(5)
+                    .sorted()
+                    .map(InvalidUserDataException::new)
+                    .collect(Collectors.toList())
+            );
         }
         return delegate.execute(context);
     }
 
+    private static String describeTypesChecked(ImmutableList<Class<?>> types) {
+        return types.size() == 1
+            ? "type '" + types.get(0).getName() + "'"
+            : "types '" + types.stream().map(Class::getName).collect(Collectors.joining("', '")) + "'";
+    }
+
     public interface ValidationWarningReporter {
         void reportValidationWarning(String warning);
     }
 
     private static class DefaultWorkValidationContext implements UnitOfWork.WorkValidationContext {
         private final ImmutableMultimap.Builder<Severity, String> problems = ImmutableMultimap.builder();
+        private final ImmutableList.Builder<Class<?>> types = ImmutableList.builder();
 
         @Override
         public TypeValidationContext createContextFor(Class<?> type, boolean cacheable) {
+            types.add(type);
             return new MessageFormattingTypeValidationContext(null) {
                 @Override
                 protected void recordProblem(Severity severity, String message) {
@@ -92,5 +104,9 @@ protected void recordProblem(Severity severity, String message) {
         public ImmutableMultimap<Severity, String> getProblems() {
             return problems.build();
         }
+
+        public ImmutableList<Class<?>> getTypes() {
+            return types.build();
+        }
     }
 }
diff --git a/...ore/src/main/java/org/gradle/internal/reflect/MessageFormattingTypeValidationContext.java b/...ore/src/main/java/org/gradle/internal/reflect/MessageFormattingTypeValidationContext.java
@@ -16,6 +16,8 @@
 
 package org.gradle.internal.reflect;
 
+import org.gradle.model.internal.type.ModelType;
+
 import javax.annotation.Nullable;
 
 abstract public class MessageFormattingTypeValidationContext implements TypeValidationContext {
@@ -30,7 +32,7 @@ public void visitTypeProblem(Severity severity, Class<?> type, String message) {
         @SuppressWarnings("StringBufferReplaceableByString")
         StringBuilder builder = new StringBuilder();
         builder.append("Type '");
-        builder.append(type.getTypeName());
+        builder.append(ModelType.of(type).getDisplayName());
         builder.append("': ");
         builder.append(message);
         builder.append('.');
@@ -42,7 +44,7 @@ public void visitPropertyProblem(Severity severity, @Nullable String parentPrope
         StringBuilder builder = new StringBuilder();
         if (rootType != null) {
             builder.append("Type '");
-            builder.append(rootType.getTypeName());
+            builder.append(ModelType.of(rootType).getDisplayName());
             builder.append("': ");
         }
         if (property != null) {

diff --git a/...gTest/groovy/org/gradle/plugin/devel/tasks/AbstractPluginValidationIntegrationSpec.groovy b/...gTest/groovy/org/gradle/plugin/devel/tasks/AbstractPluginValidationIntegrationSpec.groovy
@@ -206,9 +206,9 @@ abstract class AbstractPluginValidationIntegrationSpec extends AbstractIntegrati
 
         expect:
         assertValidationFailsWith(
-            "Type 'MyTask\$Options': Cannot use @CacheableTask on type. This annotation can only be used with Task types.": ERROR,
-            "Type 'MyTask\$Options': Cannot use @CacheableTransform on type. This annotation can only be used with TransformAction types.": ERROR,
             "Type 'MyTask': Cannot use @CacheableTransform on type. This annotation can only be used with TransformAction types.": ERROR,
+            "Type 'MyTask.Options': Cannot use @CacheableTask on type. This annotation can only be used with Task types.": ERROR,
+            "Type 'MyTask.Options': Cannot use @CacheableTransform on type. This annotation can only be used with TransformAction types.": ERROR,
         )
     }
 
@@ -499,8 +499,8 @@ abstract class AbstractPluginValidationIntegrationSpec extends AbstractIntegrati
 
         expect:
         assertValidationFailsWith(
-            "Type 'MyTask\$Options': non-property method 'notANestedGetter()' should not be annotated with: @Input.": WARNING,
             "Type 'MyTask': non-property method 'notAGetter()' should not be annotated with: @Input.": WARNING,
+            "Type 'MyTask.Options': non-property method 'notANestedGetter()' should not be annotated with: @Input.": WARNING,
         )
     }
 
@@ -550,11 +550,11 @@ abstract class AbstractPluginValidationIntegrationSpec extends AbstractIntegrati
 
         expect:
         assertValidationFailsWith(
-            "Type 'MyTask\$Options': setter method 'setReadWrite()' should not be annotated with: @Input.": WARNING,
-            "Type 'MyTask\$Options': setter method 'setWriteOnly()' should not be annotated with: @Input.": WARNING,
             "Type 'MyTask': property 'readWrite' is not annotated with an input or output annotation.": WARNING,
             "Type 'MyTask': setter method 'setReadWrite()' should not be annotated with: @Input.": WARNING,
             "Type 'MyTask': setter method 'setWriteOnly()' should not be annotated with: @Input.": WARNING,
+            "Type 'MyTask.Options': setter method 'setReadWrite()' should not be annotated with: @Input.": WARNING,
+            "Type 'MyTask.Options': setter method 'setWriteOnly()' should not be annotated with: @Input.": WARNING,
         )
     }
 

diff --git a/...egTest/groovy/org/gradle/plugin/devel/tasks/RuntimePluginValidationIntegrationTest.groovy b/...egTest/groovy/org/gradle/plugin/devel/tasks/RuntimePluginValidationIntegrationTest.groovy
@@ -48,9 +48,9 @@ class RuntimePluginValidationIntegrationTest extends AbstractPluginValidationInt
             .keySet()
             .collect { removeTypeForProperties(it) }
 
-        if (expectedWarnings) {
-            executer.expectDeprecationWarnings(expectedWarnings.size())
-            executer.withFullDeprecationStackTraceDisabled()
+        executer.withFullDeprecationStackTraceDisabled()
+        expectedWarnings.forEach { warning ->
+            executer.expectDeprecationWarning("$warning This behaviour has been deprecated and is scheduled to be removed in Gradle 7.0.")
         }
         if (expectedErrors) {
             fails "run"
@@ -59,8 +59,15 @@ class RuntimePluginValidationIntegrationTest extends AbstractPluginValidationInt
         }
         result.assertTaskNotSkipped(":run")
 
-        expectedWarnings.forEach { warning ->
-            assert output.contains("$warning This behaviour has been deprecated and is scheduled to be removed in Gradle 7.0.")
+        switch (expectedErrors.size()) {
+            case 0:
+                break
+            case 1:
+                failure.assertHasDescription("A problem was found with the configuration of task ':run' (type 'MyTask').")
+                break
+            default:
+                failure.assertHasDescription("Some problems were found with the configuration of task ':run' (type 'MyTask').")
+                break
         }
         expectedErrors.forEach { error ->
             failureHasCause(error)