All hooks now use a uniform mechanism.

greghaskins · Dec 4, 2016 · 105a04b · 105a04b
1 parent b8cf10d
commit 105a04b
Show file tree

Hide file tree

Showing 8 changed files with 92 additions and 108 deletions.
diff --git a/src/main/java/com/greghaskins/spectrum/Suite.java b/src/main/java/com/greghaskins/spectrum/Suite.java
@@ -1,5 +1,8 @@
 package com.greghaskins.spectrum;
 
+import static com.greghaskins.spectrum.internal.AfterHook.after;
+import static com.greghaskins.spectrum.internal.BeforeHook.before;
+
 import com.greghaskins.spectrum.internal.Atomic;
 import com.greghaskins.spectrum.internal.Child;
 import com.greghaskins.spectrum.internal.NotifyingBlock;
@@ -9,9 +12,7 @@
 import com.greghaskins.spectrum.model.Hooks;
 import com.greghaskins.spectrum.model.IdempotentBlock;
 import com.greghaskins.spectrum.model.PreConditions;
-import com.greghaskins.spectrum.model.SetupBlock;
 import com.greghaskins.spectrum.model.TaggingState;
-import com.greghaskins.spectrum.model.TeardownBlock;
 
 import org.junit.runner.Description;
 import org.junit.runner.notification.Failure;
@@ -23,13 +24,6 @@
 import java.util.Set;
 
 class Suite implements Parent, Child {
-
-  private final SetupBlock beforeAll = new SetupBlock();
-  private final TeardownBlock afterAll = new TeardownBlock();
-
-  private final SetupBlock beforeEach = new SetupBlock();
-  private final TeardownBlock afterEach = new TeardownBlock();
-
   // the hooks - they will be turned into a chain of responsibility
   // so the first one will be executed last as the chain is built up
   // from first to last.
@@ -88,13 +82,10 @@ Suite addSuite(final String name, final ChildRunner childRunner) {
         new Suite(Description.createSuiteDescription(sanitise(name)), this, childRunner,
             this.tagging.clone());
 
-    return withExtraValues(suite);
+    return addedToThis(suite);
   }
 
-  private Suite withExtraValues(Suite suite) {
-    suite.beforeAll.addBlock(this.beforeAll);
-    suite.beforeEach.addBlock(this.beforeEach);
-    suite.afterEach.addBlock(this.afterEach);
+  private Suite addedToThis(Suite suite) {
     addChild(suite);
 
     return suite;
@@ -105,7 +96,7 @@ Suite addAbortingSuite(final String name) {
         new CompositeTest(Description.createSuiteDescription(sanitise(name)), this,
             this.tagging.clone());
 
-    return withExtraValues(suite);
+    return addedToThis(suite);
   }
 
   Child addSpec(final String name, final Block block) {
@@ -119,33 +110,7 @@ private Child createSpec(final String name, final Block block) {
     final Description specDescription =
         Description.createTestDescription(this.description.getClassName(), sanitise(name));
 
-    final NotifyingBlock specBlockInContext = (description, notifier) -> {
-      try {
-        this.beforeAll.run();
-      } catch (final Throwable exception) {
-        notifier.fireTestFailure(new Failure(description, exception));
-        return;
-      }
-
-      NotifyingBlock.wrap(() -> {
-
-        Variable<Boolean> blockWasRun = new Variable<>(false);
-
-        blockWasRun.set(true);
-
-        NotifyingBlock.wrap(() -> {
-          this.beforeEach.run();
-          block.run();
-        }).run(description, notifier);
-
-        this.afterEach.run(description, notifier);
-
-        if (!blockWasRun.get()) {
-          throw new RuntimeException("aroundEach did not run the block");
-        }
-
-      }).run(description, notifier);
-    };
+    final NotifyingBlock specBlockInContext = NotifyingBlock.wrap(block);
 
     PreConditionBlock preConditionBlock =
         PreConditionBlock.with(this.preconditions.forChild(), block);
@@ -159,23 +124,23 @@ private void addChild(final Child child) {
   }
 
   void beforeAll(final Block block) {
-    this.beforeAll.addBlock(new IdempotentBlock(block));
+    addHook(new HookContext(before(new IdempotentBlock(block)), false, false, true));
   }
 
   void afterAll(final Block block) {
-    this.afterAll.addBlock(block);
+    addHook(new HookContext(after(block), false, true, false));
   }
 
   void beforeEach(final Block block) {
-    this.beforeEach.addBlock(block);
+    addHook(new HookContext(before(block), true, false, false));
   }
 
   void afterEach(final Block block) {
-    this.afterEach.addBlock(block);
+    addHook(new HookContext(after(block), true, false, false));
   }
 
   /**
-   * Adds a hook to be the last one executed before the block.
+   * Adds a hook to be the first one executed before the block.
    * This is the default. Hooks should be executed in the order they
    * are declared in the test.
    * @param hook to add
@@ -185,8 +150,8 @@ void addHook(final HookContext hook) {
   }
 
   /**
-   * Insert a hook at the front - this is for situations where a hook is only creatable
-   * after test definition, but is still to be run first.
+   * Insert a hook closest to the start of the chain of responsibility. This is for situations
+   * where a hook is only creatable after test definition, and needs to be run first.
    * @param hook to add
    */
   void insertHook(final HookContext hook) {
@@ -274,7 +239,6 @@ private void doRunSuite(final RunNotifier notifier) {
 
     NotifyingBlock.run(this.description, notifier, () -> {
       runChildren(notifier);
-      runAfterAll(notifier);
     });
   }
 
@@ -284,15 +248,17 @@ private void runChildren(final RunNotifier notifier) {
 
   protected void runChild(final Child child, final RunNotifier notifier) {
     if (this.focusedChildren.isEmpty() || this.focusedChildren.contains(child)) {
-      hooks.forThisLevel().runAround(getDescription(), notifier,
-          () -> getHooksFor(child).runAround(getDescription(), notifier, () -> child.run(notifier)));
+      hooks.forThisLevel().runAround(child.getDescription(), notifier,
+          () -> runChildWithHooksInNotifierBlock(child, notifier));
     } else {
       notifier.fireTestIgnored(child.getDescription());
     }
   }
 
-  private void runAfterAll(final RunNotifier notifier) {
-    this.afterAll.run(this.description, notifier);
+  private void runChildWithHooksInNotifierBlock(final Child child, final RunNotifier notifier) {
+    NotifyingBlock.run(child.getDescription(), notifier,
+        () -> getHooksFor(child).runAround(child.getDescription(), notifier,
+            () -> child.run(notifier)));
   }
 
   @Override
@@ -331,10 +297,10 @@ private String sanitise(final String name) {
   }
 
   public void aroundEach(Hook consumer) {
-    hooks.add(new HookContext(consumer, true, false, false));
+    addHook(new HookContext(consumer, true, false, false));
   }
 
   public void aroundAll(Hook consumer) {
-    hooks.add(new HookContext(consumer, false, true, false));
+    addHook(new HookContext(consumer, false, true, false));
   }
 }
diff --git a/src/main/java/com/greghaskins/spectrum/internal/AfterHook.java b/src/main/java/com/greghaskins/spectrum/internal/AfterHook.java
@@ -15,8 +15,11 @@ public interface AfterHook {
    */
   static Hook after(final Block block) {
     return inner -> {
-      inner.run();
-      block.run();
+      try {
+        inner.run();
+      } finally {
+        block.run();
+      }
     };
   }
 }
diff --git a/src/main/java/com/greghaskins/spectrum/model/Hooks.java b/src/main/java/com/greghaskins/spectrum/model/Hooks.java
@@ -4,11 +4,15 @@
 import com.greghaskins.spectrum.ThrowingConsumer;
 import com.greghaskins.spectrum.Variable;
 import com.greghaskins.spectrum.internal.NotifyingBlock;
+
 import org.junit.runner.Description;
 import org.junit.runner.notification.RunNotifier;
 
+import java.util.Collections;
 import java.util.LinkedList;
+import java.util.List;
 import java.util.function.Predicate;
+import java.util.stream.Collectors;
 
 /**
  * Collection of hooks. It is a linked list, but provides some helpers for
@@ -37,38 +41,43 @@ public Hooks forThisLevel() {
    * @return this for fluent use
    */
   public Hooks plus(Hooks other) {
-    other.forEach(this::addFirst);
+    List<HookContext> list = other.stream().collect(Collectors.toList());
+    Collections.reverse(list);
+    list.forEach(this::addFirst);
 
     return this;
   }
 
   /**
    * Convert the hooks into a chain of responsibility and execute as
    * a consumer of the given block.
+   * @param description test node being run
+   * @param notifier test result notifier
    * @param block to execute
    */
-  public void runAround(final Description description, final RunNotifier notifier, final Block block) {
-	  NotifyingBlock.run(description, notifier, () -> runAroundInternal(block));
+  public void runAround(final Description description, final RunNotifier notifier,
+      final Block block) {
+    NotifyingBlock.run(description, notifier, () -> runAroundInternal(block));
   }
 
-	private void runAroundInternal(Block block) {
-		Variable<Boolean> hooksRememberedToRunTheInner = new Variable<>(false);
-		ThrowingConsumer<Block> consumer = innerBlock -> {
-			hooksRememberedToRunTheInner.set(true);
-			innerBlock.run();
-		};
-
-		for (HookContext context : this) {
-		  consumer = wrap(consumer, context);
-		}
-		consumer.accept(block);
-
-		if (!hooksRememberedToRunTheInner.get()) {
-			throw new RuntimeException("At least one of the test hooks did not run the test block.");
-		}
-	}
+  private void runAroundInternal(Block block) {
+    Variable<Boolean> hooksRememberedToRunTheInner = new Variable<>(false);
+    ThrowingConsumer<Block> consumer = innerBlock -> {
+      hooksRememberedToRunTheInner.set(true);
+      innerBlock.run();
+    };
+
+    for (HookContext context : this) {
+      consumer = wrap(consumer, context);
+    }
+    consumer.accept(block);
+
+    if (!hooksRememberedToRunTheInner.get()) {
+      throw new RuntimeException("At least one of the test hooks did not run the test block.");
+    }
+  }
 
-	private ThrowingConsumer<Block> wrap(ThrowingConsumer<Block> inner, HookContext outer) {
+  private ThrowingConsumer<Block> wrap(ThrowingConsumer<Block> inner, HookContext outer) {
     return block -> outer.getHook().acceptOrThrow(() -> inner.acceptOrThrow(block));
   }
 

diff --git a/src/test/java/given/a/spec/with/exception/in/aftereach/block/WhenRunningTheSpec.java b/src/test/java/given/a/spec/with/exception/in/aftereach/block/WhenRunningTheSpec.java
@@ -27,7 +27,8 @@ public void thereIsOneFailureForEachAffectedTest() throws Exception {
   @Test
   public void theFailureExplainsWhatHappened() throws Exception {
     assertThat(this.result.getFailures().get(0),
-        is(failure("a passing test", Fixture.SomeException.class, "kaboom")));
+        is(failure("a passing test", RuntimeException.class,
+            "given.a.spec.with.exception.in.aftereach.block.Fixture$SomeException: kaboom")));
   }
 
 }
diff --git a/...est/java/given/a/spec/with/exception/in/beforeeach/block/and/aftereach/block/Fixture.java b/...est/java/given/a/spec/with/exception/in/beforeeach/block/and/aftereach/block/Fixture.java
@@ -13,11 +13,11 @@ class Spec {
         describe("an exploding beforeEach", () -> {
 
           beforeEach(() -> {
-            throw new SomeException("kaboom");
+            throw new SomeException("beforeEach went kaboom");
           });
 
           afterEach(() -> {
-            throw new SomeException("poof");
+            throw new SomeException("afterEach went poof");
           });
 
           it("a failing test", () -> {

diff --git a/...ven/a/spec/with/exception/in/beforeeach/block/and/aftereach/block/WhenRunningTheSpec.java b/...ven/a/spec/with/exception/in/beforeeach/block/and/aftereach/block/WhenRunningTheSpec.java
@@ -21,16 +21,20 @@ public void before() throws Exception {
   }
 
   @Test
-  public void thereAreTwoFailuresForEachAffectedTest() throws Exception {
-    assertThat(this.result.getFailureCount(), is(4));
+  public void theActualTestsCanOnlyFailOnceEach() throws Exception {
+    assertThat(this.result.getFailureCount(), is(2));
   }
 
   @Test
   public void theFailuresExplainWhatHappened() throws Exception {
     assertThat(this.result.getFailures().get(0),
-        is(failure("a failing test", Fixture.SomeException.class, "kaboom")));
+        is(failure("a failing test", RuntimeException.class,
+            "given.a.spec.with.exception.in.beforeeach.block.and.aftereach.block."
+                + "Fixture$SomeException: beforeEach went kaboom")));
     assertThat(this.result.getFailures().get(1),
-        is(failure("a failing test", Fixture.SomeException.class, "poof")));
+        is(failure("another failing test", RuntimeException.class,
+            "given.a.spec.with.exception.in.beforeeach.block.and.aftereach.block."
+                + "Fixture$SomeException: beforeEach went kaboom")));
   }
 
 }
diff --git a/src/test/java/specs/AroundSpecs.java b/src/test/java/specs/AroundSpecs.java
@@ -199,7 +199,8 @@ public class AroundSpecs {
 
         assertThat(result.getFailureCount(), is(1));
         Failure failure = result.getFailures().get(0);
-        assertThat(failure.getMessage(), is("At least one of the test hooks did not run the test block."));
+        assertThat(failure.getMessage(),
+            is("At least one of the test hooks did not run the test block."));
       });
 
       describe("in multiples", () -> {