Merge pull request #111 from ashleyfrieze/RunNotifierIssues

Resolve the issues reported in #110 with bad reporting of errors

build.gradle

@@ -16,7 +16,7 @@ allprojects {
 dependencies {
   compile group: 'junit', name: 'junit', version: '4.12'
   testCompile group: 'org.hamcrest', name: 'hamcrest-library', version: '1.3'
-  testCompile group: 'org.mockito', name: 'mockito-core', version: '1.10.19'
+  testCompile group: 'org.mockito', name: 'mockito-core', version: '2.7.22'
   testCompile group: 'org.springframework', name:'spring-test', version: '4.3.4.RELEASE'
   testCompile group: 'org.springframework', name:'spring-core', version: '4.3.4.RELEASE'
   testCompile group: 'org.springframework', name:'spring-context', version: '4.3.4.RELEASE'

config/google_checks.xml

@@ -43,7 +43,6 @@
             <property name="max" value="106"/>
             <property name="ignorePattern" value="^package.*|^import.*|a href|href|http://|https://|ftp://"/>
         </module>
-        <module name="AvoidStarImport"/>
         <module name="OneTopLevelClass"/>
         <module name="NoLineWrap"/>
         <module name="EmptyBlock">

src/main/java/com/greghaskins/spectrum/Spectrum.java

@@ -5,6 +5,7 @@
 import com.greghaskins.spectrum.internal.Suite;
 import com.greghaskins.spectrum.internal.blocks.ConstructorBlock;
 import com.greghaskins.spectrum.internal.junit.Rules;
+import com.greghaskins.spectrum.internal.junit.RunNotifierReporting;
 
 import org.junit.runner.Description;
 import org.junit.runner.Runner;
@@ -257,7 +258,7 @@ public Description getDescription() {
 
   @Override
   public void run(final RunNotifier notifier) {
-    this.rootSuite.run(notifier);
+    this.rootSuite.run(new RunNotifierReporting(notifier));
   }
 
   /**

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

@@ -16,7 +16,6 @@
 import com.greghaskins.spectrum.internal.hooks.HookContext.AppliesTo;
 import com.greghaskins.spectrum.internal.hooks.HookContext.Precedence;
 import com.greghaskins.spectrum.internal.hooks.LetHook;
-import com.greghaskins.spectrum.internal.junit.Rules;
 
 import org.junit.AssumptionViolatedException;
 

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

@@ -5,13 +5,13 @@
 import com.greghaskins.spectrum.internal.configuration.TaggingFilterCriteria;
 
 import org.junit.runner.Description;
-import org.junit.runner.notification.RunNotifier;
+import org.junit.runner.notification.Failure;
 
 public interface Child {
 
   Description getDescription();
 
-  void run(RunNotifier notifier);
+  void run(RunReporting<Description, Failure> reporting);
 
   int testCount();
 
@@ -33,6 +33,14 @@ default boolean isAtomic() {
     return false;
   }
 
+  /**
+   * Does this child appear as an individual test within the test runner.
+   * @return true if it's an individual test item in the runner
+   */
+  default boolean isLeaf() {
+    return false;
+  }
+
   /**
    * Gets the object to be filtered appropriately with its preconditions.
    * @param block the block that will be executed by the child - this may be of

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

@@ -3,7 +3,7 @@
 import com.greghaskins.spectrum.internal.configuration.TaggingFilterCriteria;
 
 import org.junit.runner.Description;
-import org.junit.runner.notification.RunNotifier;
+import org.junit.runner.notification.Failure;
 
 /**
  * Subclass of {@link Suite} that represent the fact that some tests are composed
@@ -26,18 +26,15 @@ public boolean isAtomic() {
     return true;
   }
 
-  private static void abortOnFailureChildRunner(final Suite suite, final RunNotifier runNotifier) {
-    FailureDetectingRunListener listener = new FailureDetectingRunListener();
-    runNotifier.addListener(listener);
-    try {
-      for (Child child : suite.children) {
-        if (listener.hasFailedYet()) {
-          child.ignore();
-        }
-        suite.runChild(child, runNotifier);
+  private static void abortOnFailureChildRunner(final Suite suite,
+      final RunReporting<Description, Failure> reporting) {
+    FailureDetectingRunDecorator<Description, Failure> decoratedReporting =
+        new FailureDetectingRunDecorator<>(reporting);
+    for (Child child : suite.children) {
+      if (decoratedReporting.hasFailedYet()) {
+        child.ignore();
       }
-    } finally {
-      runNotifier.removeListener(listener);
+      suite.runChild(child, decoratedReporting);
     }
   }
 }

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

@@ -0,0 +1,49 @@
+package com.greghaskins.spectrum.internal;
+
+/**
+ * A listener to detect test failure.
+ */
+public class FailureDetectingRunDecorator<T, F> implements RunReporting<T, F> {
+  private boolean hasFailedYet = false;
+
+  private RunReporting<T, F> decoratee;
+
+  public FailureDetectingRunDecorator(RunReporting<T, F> decoratee) {
+    this.decoratee = decoratee;
+  }
+
+  /**
+   * Has the run failed since we've been listening.
+   * @return whether any previous failures have been reported
+   */
+  public boolean hasFailedYet() {
+    return hasFailedYet;
+  }
+
+  @Override
+  public void fireTestFailure(F failure) {
+    decoratee.fireTestFailure(failure);
+    hasFailedYet = true;
+  }
+
+  @Override
+  public void fireTestIgnored(T description) {
+    decoratee.fireTestIgnored(description);
+  }
+
+  @Override
+  public void fireTestStarted(T description) {
+    decoratee.fireTestStarted(description);
+  }
+
+  @Override
+  public void fireTestFinished(T description) {
+    decoratee.fireTestFinished(description);
+  }
+
+  @Override
+  public void fireTestAssumptionFailed(F failure) {
+    decoratee.fireTestAssumptionFailed(failure);
+    hasFailedYet = true;
+  }
+}

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

@@ -0,0 +1,41 @@
+package com.greghaskins.spectrum.internal;
+
+/**
+ * Abstraction of reporting that's done on a test run. This
+ * decouples Spectrum from JUnit and also allows Spectrum to
+ * report its own way with an adapter for each test framework
+ * providing the right updates according to the target framework's
+ * needs/expectations.
+ */
+public interface RunReporting<T, F> {
+  /**
+   * Marks the test as ignored.
+   * @param description description of test
+   */
+  void fireTestIgnored(final T description);
+
+  /**
+   * Markes the test as having started - call this before any test-specific results.
+   * @param description description of test
+   */
+  void fireTestStarted(final T description);
+
+  /**
+   * Marks the test as finished - call this after any test-specific results, whether
+   * passed or failed.
+   * @param description description of test
+   */
+  void fireTestFinished(final T description);
+
+  /**
+   * Marks a test as having failed.
+   * @param failure failure information
+   */
+  void fireTestFailure(final F failure);
+
+  /**
+   * Marks a test as having an assumption failure.
+   * @param failure failure information
+   */
+  void fireTestAssumptionFailed(final F failure);
+}

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

@@ -3,7 +3,7 @@
 import com.greghaskins.spectrum.internal.blocks.NotifyingBlock;
 
 import org.junit.runner.Description;
-import org.junit.runner.notification.RunNotifier;
+import org.junit.runner.notification.Failure;
 
 final class Spec implements Child {
 
@@ -25,15 +25,14 @@ public Description getDescription() {
   }
 
   @Override
-  public void run(final RunNotifier notifier) {
+  public void run(final RunReporting<Description, Failure> notifier) {
     if (this.ignored) {
       notifier.fireTestIgnored(this.description);
       return;
     }
 
-    notifier.fireTestStarted(this.description);
     this.block.run(this.description, notifier);
-    notifier.fireTestFinished(this.description);
+
   }
 
   @Override
@@ -60,6 +59,11 @@ public boolean isAtomic() {
     return true;
   }
 
+  @Override
+  public boolean isLeaf() {
+    return true;
+  }
+
   @Override
   public boolean isEffectivelyIgnored() {
     return ignored;