Implement Review Comments

Author: leonard84

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

@@ -399,16 +399,20 @@ private void handleWhereBlock(Method method) {
   public void visitMethodAgain(Method method) {
     this.block = null;
 
-    if (!movedStatsBackToMethod)
+    if (!movedStatsBackToMethod) {
       for (Block b : method.getBlocks()) {
-        // this will only have the blocks if there was no 'cleanup' block in the method
+        // This will only run if there was no 'cleanup' block in the method.
+        // Otherwise, the blocks have already been copied to try block by visitCleanupBlock.
+        // We need to run as late as possible, so we'll have to do the handling here and in visitCleanupBlock.
         addBlockListeners(b);
         method.getStatements().addAll(b.getAst());
       }
+    }
 
     // for global required interactions
-    if (method instanceof FeatureMethod)
+    if (method instanceof FeatureMethod) {
       method.getStatements().add(createMockControllerCall(nodeCache.MockController_LeaveScope));
+    }
 
     if (methodHasCondition) {
       defineValueRecorder(method.getStatements(), "");
@@ -423,16 +427,18 @@ private void addBlockListeners(Block block) {
     BlockParseInfo blockType = block.getParseInfo();
     if (blockType == BlockParseInfo.WHERE
       || blockType == BlockParseInfo.METHOD_END
+      || blockType == BlockParseInfo.COMBINED
       || blockType == BlockParseInfo.ANONYMOUS) return;
 
     // SpockRuntime.enterBlock(getSpecificationContext(), new BlockInfo(blockKind, [blockTexts]))
     MethodCallExpression enterBlockCall = createBlockListenerCall(block, blockType, nodeCache.SpockRuntime_CallEnterBlock);
     // SpockRuntime.exitedBlock(getSpecificationContext(), new BlockInfo(blockKind, [blockTexts]))
     MethodCallExpression exitBlockCall = createBlockListenerCall(block, blockType, nodeCache.SpockRuntime_CallExitBlock);
 
-    // As the cleanup block finalizes the specification, it would override any previous block in ErrorInfo,
-    // so we only call enterBlock if there is no error yet.
     if (blockType == BlockParseInfo.CLEANUP) {
+      // In case of a cleanup block we need store a reference of the previously `currentBlock` in case that an exception occurred
+      // and restore it at the end of the cleanup block, so that the correct `BlockInfo` is available for the `IErrorContext`.
+      // The restoration happens in the `finally` statement created by `createCleanupTryCatch`.
       VariableExpression failedBlock = new VariableExpression(SpockNames.FAILED_BLOCK, nodeCache.BlockInfo);
       block.getAst().addAll(0, asList(
         ifThrowableIsNotNull(storeFailedBlock(failedBlock)),
@@ -451,7 +457,6 @@ private void addBlockListeners(Block block) {
   }
 
   private @NotNull Statement restoreFailedBlock(VariableExpression failedBlock) {
-
     return new ExpressionStatement(createDirectMethodCall(new CastExpression(nodeCache.SpecificationContext, getSpecificationContext()), nodeCache.SpecificationContext_SetBlockCurrentBlock, new ArgumentListExpression(failedBlock)));
   }
 

spock-core/src/main/java/org/spockframework/runtime/ErrorContext.java

@@ -1,14 +1,15 @@
 package org.spockframework.runtime;
 
 import org.spockframework.runtime.model.*;
+import org.spockframework.util.Nullable;
 
 class ErrorContext implements IErrorContext {
   private final SpecInfo spec;
   private final FeatureInfo feature;
   private final IterationInfo iteration;
   private final BlockInfo block;
 
-  private ErrorContext(SpecInfo spec, FeatureInfo feature, IterationInfo iteration, BlockInfo block) {
+  private ErrorContext(@Nullable SpecInfo spec, @Nullable FeatureInfo feature, @Nullable IterationInfo iteration, @Nullable BlockInfo block) {
     this.spec = spec;
     this.feature = feature;
     this.iteration = iteration;
@@ -43,4 +44,13 @@ public IterationInfo getCurrentIteration() {
   public BlockInfo getCurrentBlock() {
     return block;
   }
+
+  @Override
+  public String toString() {
+    return "ErrorContext{Spec: " + (spec == null ? "null" : spec.getDisplayName()) +
+      ", Feature: " + (feature == null ? "null" : feature.getDisplayName()) +
+      ", Iteration: " + (iteration == null ? "null" : iteration.getDisplayName()) +
+      ", Block: " + (block == null ? "null" : (block.getKind() + " " + block.getTexts()))
+      + "}";
+  }
 }

spock-core/src/main/java/org/spockframework/runtime/IRunListener.java

@@ -21,7 +21,9 @@
  * Listens to a spec run. Currently, only extensions can register listeners.
  * They do so by invoking <tt>SpecInfo.addListener()<tt>. See
  * {@link StepwiseExtension} for an example of how to use a listener.
+ * <p>
  *
+ * @see org.spockframework.runtime.extension.IBlockListener
  * @author Peter Niederwieser
  */
 public interface IRunListener {

spock-core/src/main/java/org/spockframework/runtime/SpecInfoBuilder.java

@@ -169,10 +169,7 @@ private FeatureInfo createFeature(Method method, FeatureMetadata featureMetadata
     }
 
     for (BlockMetadata blockMetadata : featureMetadata.blocks()) {
-      BlockInfo block = new BlockInfo();
-      block.setKind(blockMetadata.kind());
-      block.setTexts(asList(blockMetadata.texts()));
-      feature.addBlock(block);
+      feature.addBlock(new BlockInfo(blockMetadata.kind(), asList(blockMetadata.texts())));
     }
 
     return feature;

spock-core/src/main/java/org/spockframework/runtime/extension/IBlockListener.java

@@ -1,9 +1,43 @@
 package org.spockframework.runtime.extension;
 
 import org.spockframework.runtime.model.BlockInfo;
+import org.spockframework.runtime.model.ErrorInfo;
 import org.spockframework.runtime.model.IterationInfo;
+import org.spockframework.util.Beta;
 
+/**
+ * Listens to block events during the execution of a feature.
+ * <p>
+ * Usually used in conjunction with {@link org.spockframework.runtime.IRunListener}.
+ * Currently, only extensions can register listeners.
+ * They do so by invoking {@link org.spockframework.runtime.model.FeatureInfo#addBlockListener(IBlockListener)}.
+ * It is preferred to use a single instance of this.
+ * <p>
+ * It is discouraged to perform long-running operations in the listener methods,
+ * as they are called during the execution of the specification.
+ * It is discouraged to perform any side effects affecting the tests.
+ * <p>
+ * When an exception is thrown in a block, the {@code blockExited} will not be called for that block.
+ * If a cleanup block is present the cleanup block listener methods will still be called.
+ *
+ * @see org.spockframework.runtime.IRunListener
+ * @author Leonard Brünings
+ * @since 2.4
+ */
+@Beta
 public interface IBlockListener {
+
+  /**
+   * Called when a block is entered.
+   */
   default void blockEntered(IterationInfo iterationInfo, BlockInfo blockInfo) {}
+
+  /**
+   * Called when a block is exited.
+   * <p>
+   * This method is not called if an exception is thrown in the block.
+   * The block that was active will be available in the {@link org.spockframework.runtime.model.IErrorContext}
+   * and can be observed via {@link org.spockframework.runtime.IRunListener#error(ErrorInfo)}.
+   */
   default void blockExited(IterationInfo iterationInfo, BlockInfo blockInfo) {}
 }

spock-core/src/main/java/org/spockframework/runtime/model/BlockInfo.java

@@ -16,6 +16,8 @@
 
 package org.spockframework.runtime.model;
 
+import org.spockframework.util.Nullable;
+
 import java.util.List;
 
 /**
@@ -35,6 +37,7 @@ public BlockInfo(BlockKind kind, List<String> texts) {
     this.texts = texts;
   }
 
+  @Nullable
   public BlockKind getKind() {
     return kind;
   }
@@ -43,6 +46,7 @@ public void setKind(BlockKind kind) {
     this.kind = kind;
   }
 
+  @Nullable
   public List<String> getTexts() {
     return texts;
   }

spock-core/src/main/java/org/spockframework/runtime/model/ErrorInfo.java

@@ -36,4 +36,13 @@ public Throwable getException() {
   public IErrorContext getErrorContext() {
     return errorContext;
   }
+
+  @Override
+  public String toString() {
+    return "ErrorInfo{" +
+      "method=" + method +
+      ", errorContext=" + errorContext +
+      ", error=" + error +
+      '}';
+  }
 }

spock-core/src/main/java/org/spockframework/runtime/model/FeatureInfo.java

@@ -168,7 +168,7 @@ public List<IMethodInterceptor> getInitializerInterceptors() {
   }
 
   /**
-   * Adds a initializer interceptor for this feature.
+   * Adds an initializer interceptor for this feature.
    * <p>
    * The feature-scoped interceptors will execute before the spec interceptors.
    *
@@ -224,10 +224,18 @@ public void addIterationInterceptor(IMethodInterceptor interceptor) {
     iterationInterceptors.add(interceptor);
   }
 
+  /**
+   * @since 2.4
+   */
+  @Beta
   public List<IBlockListener> getBlockListeners() {
     return blockListeners;
   }
 
+  /**
+   * @since 2.4
+   */
+  @Beta
   public void addBlockListener(IBlockListener blockListener) {
     blockListeners.add(blockListener);
   }

spock-core/src/main/java/org/spockframework/runtime/model/IErrorContext.java

@@ -2,6 +2,11 @@
 
 import org.spockframework.util.Nullable;
 
+/**
+ * Provides context information for an error that occurred during the execution of a specification.
+ * <p>
+ * Depending on the context in which the error occurred, some of the methods may return {@code null}.
+ */
 public interface IErrorContext {
   @Nullable
   SpecInfo getCurrentSpec();

spock-specs/src/test/groovy/org/spockframework/runtime/RunListenerSpec.groovy

@@ -234,6 +234,7 @@ class ASpec extends Specification {
         it.kind == block
         it.texts == blockTexts
       }
+      assert errorInfo.errorContext.toString() == ''
     } else {
       assert errorInfo.errorContext.currentBlock == null
     }