Review Feedback Vol. 1

Author: Vampire

docs/release_notes.adoc

@@ -71,7 +71,7 @@ will now require that the spec is annotated with <<parallel_execution.adoc#isola
 
 === Misc
 
-* Fix handling of @Verify and @VerifyAll spockIssue:2150[]
+* Fix handling of `@Verify` and `@VerifyAll` spockIssue:2150[]
 * Fix handling of condition method calls spockPull:2162[]
 * Fix vararg handling in SpyStatic spockIssue:2161[]
 * Fix incompatibility with JUnit 6 in OSGi environment spockIssue:2231[]

spock-core/src/main/java/org/spockframework/compiler/AstNodeCache.java

@@ -48,8 +48,6 @@ public class AstNodeCache {
   public final ClassNode DataVariableMultiplication = ClassHelper.makeWithoutCaching(DataVariableMultiplication.class);
   public final ClassNode DataVariableMultiplicationFactor = ClassHelper.makeWithoutCaching(DataVariableMultiplicationFactor.class);
   public final ClassNode BlockInfo = ClassHelper.makeWithoutCaching(BlockInfo.class);
-  public final ClassNode Verify = ClassHelper.makeWithoutCaching(Verify.class);
-  public final ClassNode VerifyAll = ClassHelper.makeWithoutCaching(VerifyAll.class);
   public final ClassNode Void = ClassHelper.makeWithoutCaching(void.class);
 
   public final MethodNode Specification_GetSpecificationContext =

spock-core/src/main/java/org/spockframework/compiler/DeepBlockRewriter.java

@@ -184,12 +184,17 @@ private boolean handleInteraction(InteractionRewriter rewriter, ExpressionStatem
     // would also want to enforce this for when-blocks, but unfortunately it's not that uncommon
     // for projects to have interactions in when-blocks (Gradle, Tapestry). Before we enforce this,
     // we should at least support multiple setup-blocks.
-    if ((block instanceof ExpectBlock) || (block instanceof VerifyBlock)) {
+    if (block instanceof ExpectBlock) {
       resources.getErrorReporter().error(stat, "Interactions are not allowed in '%s' blocks. " +
           "Put them before the '%s' block or into a 'then' block.", block.getName(), block.getName());
       return true;
     }
 
+    if (block instanceof VerifyBlock) {
+      resources.getErrorReporter().error(stat, "Interactions are not allowed in '@Verify' and '@VerifyAll' methods.");
+      return true;
+    }
+
     replaceVisitedStatementWith(interaction);
     interactionFound = true;
     return true;

spock-core/src/main/java/org/spockframework/compiler/SpecParser.java

@@ -24,7 +24,10 @@
 import org.codehaus.groovy.ast.*;
 import org.codehaus.groovy.ast.expr.ConstantExpression;
 import org.codehaus.groovy.ast.stmt.Statement;
+import spock.lang.Verify;
+import spock.lang.VerifyAll;
 
+import static org.spockframework.compiler.AstUtil.hasAnnotation;
 import static org.spockframework.util.Identifiers.*;
 
 /**
@@ -67,7 +70,7 @@ public void visitField(FieldNode gField) {
     if (gField.isStatic()) return;
 
     Field field = new Field(spec, gField, fieldCount++);
-    field.setShared(AstUtil.hasAnnotation(gField, Shared.class));
+    field.setShared(hasAnnotation(gField, Shared.class));
     field.setOwner(owner);
     spec.getFields().add(field);
   }
@@ -181,26 +184,24 @@ private void buildFeatureMethod(MethodNode method) {
   }
 
   private boolean isVerifyMethod(MethodNode method) {
-    boolean hasVerifyAnnotation = !method.getAnnotations(nodeCache.Verify).isEmpty();
-    boolean hasVerifyAllAnnotation = !method.getAnnotations(nodeCache.VerifyAll).isEmpty();
+    boolean hasVerifyAnnotation = hasAnnotation(method, Verify.class);
+    boolean hasVerifyAllAnnotation = hasAnnotation(method, VerifyAll.class);
     if (hasVerifyAnnotation && hasVerifyAllAnnotation) {
-      errorReporter.error(method, "Method '%s' cannot be annotated with both @Verify and @VerifyAll", method.getName());
+      errorReporter.error(method, "Method '%s' cannot be annotated with both @Verify and @VerifyAll.", method.getName());
       return false;
     }
     return hasVerifyAnnotation || hasVerifyAllAnnotation;
   }
 
   private void buildVerifyMethod(MethodNode method) {
-    if (!method.isVoidMethod() && !method.isDynamicReturnType()) {
-      errorReporter.error("Verification helper method '%s' must have a void or dynamic return type.", method.getName());
-    }
+    VerifyMethod.verifyReturnType(method, errorReporter);
     method.setReturnType(nodeCache.Void);
 
     Method verify;
-    if (method.getAnnotations(nodeCache.Verify).isEmpty()) {
-      verify = new VerifyAllMethod(spec, method);
-    } else {
+    if (hasAnnotation(method, Verify.class)) {
       verify = new VerifyMethod(spec, method);
+    } else {
+      verify = new VerifyAllMethod(spec, method);
     }
     spec.getMethods().add(verify);
   }

spock-core/src/main/java/org/spockframework/compiler/SpecialMethodCall.java

@@ -47,6 +47,10 @@ public class SpecialMethodCall implements ISpecialMethodCall {
   private final boolean conditionBlock;
   private final AstNodeCache nodeCache;
 
+  public SpecialMethodCall(String methodName, AstNodeCache nodeCache) {
+    this(methodName, null, null, null, null, null, false, nodeCache);
+  }
+
   public SpecialMethodCall(String methodName, Expression inferredName, Expression inferredType,
       MethodCallExpression methodCallExpr, @Nullable BinaryExpression binaryExpr,
       @Nullable ClosureExpression closureExpr, boolean conditionBlock, AstNodeCache nodeCache) {

spock-core/src/main/java/org/spockframework/compiler/condition/BaseVerifyMethodTransform.java

@@ -19,6 +19,11 @@
 import org.codehaus.groovy.transform.ASTTransformation;
 import org.spockframework.compiler.*;
 import org.spockframework.compiler.model.Method;
+import org.spockframework.compiler.model.VerifyMethod;
+import spock.lang.Verify;
+import spock.lang.VerifyAll;
+
+import static org.spockframework.compiler.AstUtil.hasAnnotation;
 
 abstract class BaseVerifyMethodTransform implements ASTTransformation {
 
@@ -37,27 +42,25 @@ public void visit(ASTNode[] astNodes, SourceUnit sourceUnit) {
         if (!(node instanceof MethodNode)) {
           continue;
         }
+        MethodNode methodNode = (MethodNode) node;
 
-        if (((MethodNode) node).getDeclaringClass().isDerivedFrom(nodeCache.Specification)) {
+        if (methodNode.getDeclaringClass().isDerivedFrom(nodeCache.Specification)) {
           // SpecRewriter already handles the method, so don't transform it again
           continue;
         }
 
-        boolean hasVerifyAnnotation = !((MethodNode) node).getAnnotations(nodeCache.Verify).isEmpty();
-        boolean hasVerifyAllAnnotation = !((MethodNode) node).getAnnotations(nodeCache.VerifyAll).isEmpty();
-        if (hasVerifyAnnotation && hasVerifyAllAnnotation) {
-          errorReporter.error(node, "Method '%s' cannot be annotated with both @Verify and @VerifyAll", ((MethodNode) node).getName());
+        if (hasAnnotation(node, Verify.class) && hasAnnotation(node, VerifyAll.class)) {
+          errorReporter.error(node, "Method '%s' cannot be annotated with both @Verify and @VerifyAll.", methodNode.getName());
         }
 
-        processVerificationHelperMethod(createVerifyMethod((MethodNode) node), errorReporter, sourceLookup);
+        processVerificationHelperMethod(createVerifyMethod(methodNode), errorReporter, sourceLookup);
       }
     }
   }
 
   private void processVerificationHelperMethod(Method method, ErrorReporter errorReporter, SourceLookup sourceLookup) {
     MethodNode methodAst = method.getAst();
-    if (!methodAst.isVoidMethod() && !methodAst.isDynamicReturnType()) {
-      errorReporter.error("Verification helper method '%s' must have a void or dynamic return type.", method.getName());
+    if (!VerifyMethod.verifyReturnType(methodAst, errorReporter)) {
       return;
     }
 

spock-core/src/main/java/org/spockframework/compiler/condition/VerifyAllMethodRewriter.java

@@ -39,7 +39,6 @@ void defineErrorCollector(boolean methodHasCondition, boolean methodHasDeepNonGr
 
   @Override
   ISpecialMethodCall createSpecialMethodCall() {
-    return new SpecialMethodCall(Identifiers.VERIFY_ALL, null, null, null,
-      null, null, false, resources.getAstNodeCache());
+    return new SpecialMethodCall(Identifiers.VERIFY_ALL, resources.getAstNodeCache());
   }
 }

spock-core/src/main/java/org/spockframework/compiler/condition/VerifyMethodRewriter.java

@@ -39,7 +39,6 @@ void defineErrorCollector(boolean methodHasCondition, boolean methodHasDeepNonGr
 
   @Override
   ISpecialMethodCall createSpecialMethodCall() {
-    return new SpecialMethodCall(Identifiers.WITH, null, null, null,
-      null, null, false, resources.getAstNodeCache());
+    return new SpecialMethodCall(Identifiers.WITH, resources.getAstNodeCache());
   }
 }

spock-core/src/main/java/org/spockframework/compiler/model/VerifyMethod.java

@@ -19,6 +19,7 @@
 import org.codehaus.groovy.ast.MethodNode;
 import org.codehaus.groovy.ast.stmt.Statement;
 import org.spockframework.compiler.AstUtil;
+import org.spockframework.compiler.ErrorReporter;
 
 import java.util.List;
 
@@ -38,4 +39,12 @@ public VerifyMethod(Spec parent, MethodNode code) {
     stats.clear();
     addBlock(block);
   }
+
+  public static boolean verifyReturnType(MethodNode method, ErrorReporter errorReporter) {
+    if (!method.isVoidMethod() && !method.isDynamicReturnType()) {
+      errorReporter.error("Verification helper method '%s' must have a void or dynamic return type.", method.getName());
+      return false;
+    }
+    return true;
+  }
 }

spock-specs/src/test/groovy/org/spockframework/smoke/ast/condition/BaseVerifyMethodsAstSpec.groovy

@@ -275,7 +275,7 @@ class SpecificAssertions extends BaseAssertions {
     snapshotter.assertThat(result.source).matchesSnapshot()
   }
 
-  def "fails when verification method has a non-void or -dynamic return value"() {
+  def "fails when verification method has a non-void or non-dynamic return value"() {
     when:
     compiler.compile("""
 class Assertions {
@@ -292,7 +292,7 @@ class Assertions {
     e.message.contains("Verification helper method 'isPositiveWithReturn' must have a void or dynamic return type.")
   }
 
-  def "fails when verification method has a non-void or -dynamic return value in Spec"() {
+  def "fails when verification method has a non-void or non-dynamic return value in Spec"() {
     when:
     compiler.compileSpecBody("""
 @${annotation.name}