Merge pull request #112 from ashleyfrieze/RunNotifierErrorHandling

Fixes for `beforeAll` failures

docs/JunitRules.md

@@ -87,8 +87,8 @@ The rules are applied and the test object created just in time for each atomic t
 ### What is Supported
 
 * `@ClassRule` is applied
-* `@BeforeAll` is applied
-* `@AfterAll` is applied
+* `@BeforeClass` is applied
+* `@AfterClass` is applied
 * `@Rule` objects:
   * `TestRule`s are applied at the level of each atomic test
   * `MethodRule`s are applied at the level of each atomic test

src/main/java/com/greghaskins/spectrum/dsl/specification/Specification.java

@@ -146,7 +146,7 @@ static void afterEach(final Block block) {
    * @param block {@link Block} to run once before all specs in this suite
    */
   static void beforeAll(final Block block) {
-    DeclarationState.instance().addHook(before(new IdempotentBlock(block)), AppliesTo.EACH_CHILD,
+    DeclarationState.instance().addHook(before(new IdempotentBlock(block)), AppliesTo.ATOMIC_ONLY,
         Precedence.SET_UP);
   }
 

src/main/java/com/greghaskins/spectrum/internal/Suite.java

@@ -218,7 +218,7 @@ protected void runChild(final Child child, final RunReporting<Description, Failu
     } else if (childIsNotInFocus(child)) {
       reporting.fireTestIgnored(child.getDescription());
     } else {
-      this.hooks.forThisLevel().sorted().runAround(child.getDescription(), reporting,
+      addLeafHook(this.hooks.forThisLevel().sorted(), child).runAround(child.getDescription(), reporting,
           () -> runChildWithHooks(child, reporting));
     }
   }
@@ -228,7 +228,7 @@ private boolean childIsNotInFocus(Child child) {
   }
 
   private void runChildWithHooks(final Child child, final RunReporting<Description, Failure> reporting) {
-    addLeafHook(getHooksFor(child).sorted(), child).runAround(child.getDescription(), reporting,
+    getHooksFor(child).sorted().runAround(child.getDescription(), reporting,
         () -> child.run(reporting));
 
   }

src/main/java/com/greghaskins/spectrum/internal/hooks/HookContext.java

@@ -62,17 +62,16 @@ public enum Precedence {
      */
     GUARANTEED_CLEAN_UP_GLOBAL(2),
 
-
     /**
-     * Guaranteed tidy up code should be allowed to run no matter
-     * what, once the test has started.
+     * Set up code should run before the local context.
      */
-    GUARANTEED_CLEAN_UP_LOCAL(3),
+    SET_UP(3),
 
     /**
-     * Set up code should run before the local context.
+     * Guaranteed tidy up code should be allowed to run no matter
+     * what, once the test has started.
      */
-    SET_UP(4),
+    GUARANTEED_CLEAN_UP_LOCAL(4),
 
     /**
      * Local context - the order depends on declaration.

src/main/java/com/greghaskins/spectrum/internal/junit/RuleContext.java

@@ -161,17 +161,25 @@ private synchronized Statement withClassRules(final Statement base,
   }
 
   private Statement withAfterClasses(final Statement base) {
-    List<FrameworkMethod> afters = testClass.getAnnotatedMethods(AfterClass.class);
+    List<FrameworkMethod> afters = getAfterClassMethods();
 
     return afters.isEmpty() ? base : new RunAfters(base, afters, null);
   }
 
+  private List<FrameworkMethod> getAfterClassMethods() {
+    return testClass.getAnnotatedMethods(AfterClass.class);
+  }
+
   private Statement withBeforeClasses(final Statement base) {
-    List<FrameworkMethod> befores = testClass.getAnnotatedMethods(BeforeClass.class);
+    List<FrameworkMethod> befores = getBeforeClassMethods();
 
     return befores.isEmpty() ? base : new RunBefores(base, befores, null);
   }
 
+  private List<FrameworkMethod> getBeforeClassMethods() {
+    return testClass.getAnnotatedMethods(BeforeClass.class);
+  }
+
   private List<TestRule> getClassRules() {
     return Stream.concat(
         testClass.getAnnotatedMethodValues(null, ClassRule.class, TestRule.class).stream(),
@@ -198,7 +206,8 @@ public void evaluate() throws Throwable {
    * @return true if there are rules
    */
   boolean hasAnyJUnitAnnotations() {
-    return getClassRules().size() > 0 || hasAnyTestOrMethodRules();
+    return hasAnyTestOrMethodRules()
+        || getClassRules().size() + getAfterClassMethods().size() + getBeforeClassMethods().size() > 0;
   }
 
   /**

src/test/java/com/greghaskins/spectrum/SpectrumHelper.java

@@ -12,6 +12,26 @@
 import java.util.List;
 
 public class SpectrumHelper {
+  // Allows us to hide a test class from IDE/test discovery
+  public static class NullRunner extends Runner {
+    private Class<?> clazz;
+
+    public NullRunner(Class<?> clazz) {
+      this.clazz = clazz;
+    }
+
+    @Override
+    public Description getDescription() {
+      // empty suite
+
+      return Description.createSuiteDescription(clazz);
+    }
+
+    @Override
+    public void run(RunNotifier notifier) {
+      // do nothing
+    }
+  }
 
   public static class RecordingListener extends RunListener {
     private List<Description> testsStarted = new ArrayList<>();
@@ -56,5 +76,4 @@ public static <T extends RunListener> T runWithListener(final Class<?> specClass
   private static Result runWithJUnit(final Runner runner) {
     return new JUnitCore().run(Request.runner(runner));
   }
-
 }

src/test/java/junit/RunNotifierTest.java

@@ -7,9 +7,9 @@
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.argThat;
 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.times;
 
 import com.greghaskins.spectrum.Spectrum;
+import com.greghaskins.spectrum.SpectrumHelper;
 
 import org.junit.Before;
 import org.junit.BeforeClass;
@@ -104,9 +104,31 @@ public class RunNotifierTest {
         // note: this is a deviation from the way BlockJUnitRunner specifically runs
         RunNotifier notifier = runWithSpectrumNotifier(failingBeforeAllTest());
         InOrder inOrder = Mockito.inOrder(notifier);
-        inOrder.verify(notifier, times(2)).fireTestFailure(any());
+        inOrder.verify(notifier).fireTestStarted(forMethod("failsTransitively1"));
+        inOrder.verify(notifier).fireTestFailure(any());
+        inOrder.verify(notifier).fireTestFinished(forMethod("failsTransitively1"));
+        inOrder.verify(notifier).fireTestStarted(forMethod("failsTransitively2"));
+        inOrder.verify(notifier).fireTestFailure(any());
+        inOrder.verify(notifier).fireTestFinished(forMethod("failsTransitively2"));
+      });
+
+      it("Reports single failure when it's the JUnit @BeforeClass that's failing", () -> {
+        RunNotifier notifier = runWithSpectrumNotifier(BeforeClassFailsInJunitStyle.class);
+        InOrder inOrder = Mockito.inOrder(notifier);
+        inOrder.verify(notifier).fireTestFailure(any());
         inOrder.verifyNoMoreInteractions();
       });
+
+      it("Reports errors correctly in nested tests", () -> {
+        RunNotifier notifier = runWithSpectrumNotifier(failingNestedBeforeAllTest());
+        InOrder inOrder = Mockito.inOrder(notifier);
+        inOrder.verify(notifier).fireTestStarted(forMethod("failsTransitively1"));
+        inOrder.verify(notifier).fireTestFailure(any());
+        inOrder.verify(notifier).fireTestFinished(forMethod("failsTransitively1"));
+        inOrder.verify(notifier).fireTestStarted(forMethod("failsTransitively2"));
+        inOrder.verify(notifier).fireTestFailure(any());
+        inOrder.verify(notifier).fireTestFinished(forMethod("failsTransitively2"));
+      });
     });
   }
 
@@ -134,26 +156,28 @@ private RunNotifier runWithSpectrumNotifier(Class<?> clazz) {
   }
 
   // these classes will ACTUALLY be run with BlockJUnitRunner above, but we mark them
-  // as run with Spectrum so that they are passed over if an IDE's test runner decides
+  // as run with null so that they are passed over if an IDE's test runner decides
   // to try to execute them
-  @RunWith(Spectrum.class)
+  @RunWith(SpectrumHelper.NullRunner.class)
   public static class OnePassing {
     @Test
     public void passes() {
       successfulAssertion();
     }
   }
 
-  @RunWith(Spectrum.class)
+
+  @RunWith(SpectrumHelper.NullRunner.class)
   public static class OneFailing {
     @Test
     public void fails() {
       failAnAssertion();
     }
   }
 
+
   // method order is fixed to enable order-based verification
-  @RunWith(Spectrum.class)
+  @RunWith(SpectrumHelper.NullRunner.class)
   @FixMethodOrder(MethodSorters.NAME_ASCENDING)
   public static class BeforeMethodFails {
     @Before
@@ -172,8 +196,9 @@ public void failsTransitively2() {
     }
   }
 
+
   // method order is fixed to enable order-based verification
-  @RunWith(Spectrum.class)
+  @RunWith(SpectrumHelper.NullRunner.class)
   @FixMethodOrder(MethodSorters.NAME_ASCENDING)
   public static class BeforeClassMethodFails {
     @BeforeClass
@@ -259,6 +284,45 @@ class FailingBeforeAll {
     return FailingBeforeAll.class;
   }
 
+  private static Class<?> failingNestedBeforeAllTest() {
+    class FailingNestedBeforeAll {
+      {
+        describe("suite", () -> {
+          beforeAll(() -> {
+            throw new IllegalArgumentException("aaagh");
+          });
+
+          describe("with sub suite", () -> {
+            it("failsTransitively1", () -> {
+            });
+
+            it("failsTransitively2", () -> {
+            });
+          });
+        });
+      }
+    }
+
+    return FailingNestedBeforeAll.class;
+  }
+
+  @RunWith(SpectrumHelper.NullRunner.class)
+  public static class BeforeClassFailsInJunitStyle {
+    @BeforeClass
+    public static void beforeClass() {
+      failAnAssertion();
+    }
+
+    {
+      describe("A suite", () -> {
+        it("fails because of the Junit before class", () -> {
+        });
+        it("fails again because of the Junit before class", () -> {
+        });
+      });
+    }
+  }
+
   private static void successfulAssertion() {
     assertThat("black", is("black"));
   }

src/test/java/specs/JUnitRuleExample.java

@@ -9,6 +9,7 @@
 
 import com.greghaskins.spectrum.Spectrum;
 
+import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.rules.TemporaryFolder;
 import org.junit.runner.RunWith;
@@ -20,11 +21,29 @@
 
 @RunWith(Spectrum.class)
 public class JUnitRuleExample {
+  // mixins for the Spectrum native style of mixin
   public static class TempFolderRuleMixin {
     @Rule
     public TemporaryFolder tempFolderRule = new TemporaryFolder();
   }
 
+  public static class JUnitBeforeClassExample {
+    public static String value;
+
+    @BeforeClass
+    public static void beforeClass() {
+      value = "Hello world";
+    }
+  }
+
+  // can also use native junit annotations
+  private static String classValue;
+
+  @BeforeClass
+  public static void beforeClass() {
+    classValue = "initialised";
+  }
+
   {
     final Set<File> ruleProvidedFoldersSeen = new HashSet<>();
     describe("A spec with a rule mix-in", () -> {
@@ -46,6 +65,17 @@ public static class TempFolderRuleMixin {
       it("has received a different instance of the rule-provided object each time", () -> {
         assertThat(ruleProvidedFoldersSeen.size(), is(3));
       });
+
+      describe("with just a beforeClass mixin", () -> {
+        Supplier<JUnitBeforeClassExample> mixin = junitMixin(JUnitBeforeClassExample.class);
+        it("the mixin's before class has been called", () -> {
+          assertThat(JUnitBeforeClassExample.value, is("Hello world"));
+        });
+      });
+
+      it("has also initialised a class member owing to a local JUnit annotation", () -> {
+        assertThat(classValue, is("initialised"));
+      });
     });
   }