JUnit behaves better when a Before hook fails. Closes cucumber#106

README.md

@@ -134,3 +134,7 @@ This can be solved by changing the Compiler settings: `Preferences -> Compiler -
 
 Fork the repository on Github, clone it and send a pull request when you have fixed something. Please commit each feature/bugfix on a separate branch as this makes it easier for us to decide what to merge and what not to merge.
 
+## TODO
+
+* Reports exception when Before hook fails
+* Skips steps when before hook fails

core/src/main/java/cucumber/runtime/World.java

@@ -17,23 +17,22 @@ public class World {
     private final List<StepDefinition> stepDefinitions = new ArrayList<StepDefinition>();
     private final List<HookDefinition> beforeHooks = new ArrayList<HookDefinition>();
     private final List<HookDefinition> afterHooks = new ArrayList<HookDefinition>();
+    private final ScenarioResultImpl scenarioResult = new ScenarioResultImpl();
 
     private final Runtime runtime;
     private final Collection<String> tags;
 
-    private ScenarioResultImpl scenarioResult;
-    private boolean skipNext = false;
+    private boolean skipNextStep = false;
 
     public World(Runtime runtime, Collection<String> tags) {
         this.runtime = runtime;
         this.tags = tags;
     }
 
-    public void buildBackendWorldsAndRunBeforeHooks(List<String> gluePaths) throws Throwable {
+    public void buildBackendWorldsAndRunBeforeHooks(List<String> gluePaths, Reporter reporter) {
         runtime.buildBackendWorlds(gluePaths, this);
-        scenarioResult = new ScenarioResultImpl();
         Collections.sort(beforeHooks, new HookComparator(true));
-        runHooks(beforeHooks, null);
+        runHooks(beforeHooks, reporter);
     }
 
     private List<StepDefinitionMatch> stepDefinitionMatches(String uri, Step step) {
@@ -47,22 +46,33 @@ private List<StepDefinitionMatch> stepDefinitionMatches(String uri, Step step) {
         return result;
     }
 
-    public void runAfterHooksAndDisposeBackendWorlds() throws Throwable {
+    public void runAfterHooksAndDisposeBackendWorlds(Reporter reporter) {
         Collections.sort(afterHooks, new HookComparator(false));
         try {
-            runHooks(afterHooks, scenarioResult);
+            runHooks(afterHooks, reporter);
         } finally {
             runtime.disposeBackendWorlds();
         }
     }
 
-    private void runHooks(List<HookDefinition> hooks, ScenarioResult scenarioResult) throws Throwable {
-        for (HookDefinition hook : hooks) {
-            runHookMaybe(hook, scenarioResult);
+    private void runHooks(List<HookDefinition> hooks, Reporter reporter) {
+        long start = 0;
+        try {
+            for (HookDefinition hook : hooks) {
+                start = System.nanoTime();
+                runHookMaybe(hook);
+            }
+        } catch (Throwable t) {
+            skipNextStep = true;
+
+            long duration = System.nanoTime() - start;
+            Result result = new Result(Result.FAILED, duration, t, DUMMY_ARG);
+            scenarioResult.add(result);
+            reporter.result(result);
         }
     }
 
-    private void runHookMaybe(HookDefinition hook, ScenarioResult scenarioResult) throws Throwable {
+    private void runHookMaybe(HookDefinition hook) throws Throwable {
         if (hook.matches(tags)) {
             hook.execute(scenarioResult);
         }
@@ -75,15 +85,15 @@ public void runStep(String uri, Step step, Reporter reporter, Locale locale) thr
         } else {
             reporter.match(Match.UNDEFINED);
             reporter.result(Result.UNDEFINED);
-            skipNext = true;
+            skipNextStep = true;
             return;
         }
 
         if (runtime.isDryRun()) {
-            skipNext = true;
+            skipNextStep = true;
         }
 
-        if (skipNext) {
+        if (skipNextStep) {
             scenarioResult.add(Result.SKIPPED);
             reporter.result(Result.SKIPPED);
         } else {

core/src/main/java/cucumber/runtime/model/CucumberScenario.java

@@ -24,10 +24,8 @@ public CucumberScenario(CucumberFeature cucumberFeature, CucumberBackground cucu
         this.cucumberBackground = cucumberBackground;
     }
 
-    public World buildWorldAndRunBeforeHooks(List<String> gluePaths, Runtime runtime) throws Throwable {
+    public World newWorld(Runtime runtime) {
         world = new World(runtime, tags());
-        world.buildBackendWorldsAndRunBeforeHooks(gluePaths);
-        // TODO: If before hooks fail, we can't return the world, but we need it to run (skipped) steps
         return world;
     }
 
@@ -37,43 +35,31 @@ public World buildWorldAndRunBeforeHooks(List<String> gluePaths, Runtime runtime
     @Override
     public void run(Formatter formatter, Reporter reporter, Runtime runtime, List<? extends Backend> backends, List<String> gluePaths) {
         // TODO: Maybe get extraPaths from scenario
+        // TODO: maybe just try to make Background behave like a regular Scenario?? Printing wise at least.
 
-        // TODO: split up prepareAndFormat so we can run Background in isolation.
-        // Or maybe just try to make Background behave like a regular Scenario?? Printing wise at least.
-
-        try {
-            buildWorldAndRunBeforeHooks(gluePaths, runtime);
-        } catch (Throwable e) {
-            // TODO What do we do now??? #106
-        }
+        World world = newWorld(runtime);
+        world.buildBackendWorldsAndRunBeforeHooks(gluePaths, reporter);
 
         try {
             runBackground(formatter, reporter);
         } catch (Throwable t) {
-            // TODO What do we do now??? #106
+            // TODO What do we do now???
+            t.printStackTrace();
         }
 
         try {
             formatAndRunSteps(formatter, reporter, world);
-        } catch (Throwable throwable) {
-            throwable.printStackTrace();
-        }
-
-        try {
-            runAfterHooksAndDisposeWorld();
         } catch (Throwable t) {
-            // TODO What do we do now??? #106
+            // TODO What do we do now???
+            t.printStackTrace();
         }
+
+        world.runAfterHooksAndDisposeBackendWorlds(reporter);
     }
 
     public void runBackground(Formatter formatter, Reporter reporter) throws Throwable {
         if (cucumberBackground != null) {
             cucumberBackground.formatAndRunSteps(formatter, reporter, world);
         }
     }
-
-    public void runAfterHooksAndDisposeWorld() throws Throwable {
-        world.runAfterHooksAndDisposeBackendWorlds();
-    }
-
 }

core/src/main/java/cucumber/runtime/model/StepContainer.java

@@ -34,6 +34,7 @@ public void format(Formatter formatter) {
         }
     }
 
+    // TODO: Do not throw, just report like in a Scenario. In fact, use the same code
     public void formatAndRunSteps(Formatter formatter, Reporter reporter, World world) throws Throwable {
         format(formatter);
         Throwable e = null;

core/src/test/java/cucumber/runtime/HookOrderTest.java

@@ -1,8 +1,10 @@
 package cucumber.runtime;
 
+import gherkin.formatter.Reporter;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.InOrder;
+import org.mockito.Matchers;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -26,12 +28,12 @@ public void before_hooks_execute_in_order() throws Throwable {
             world.addBeforeHook(hook);
         }
 
-        world.buildBackendWorldsAndRunBeforeHooks(new ArrayList<String>());
+        world.buildBackendWorldsAndRunBeforeHooks(new ArrayList<String>(), mock(Reporter.class));
 
         InOrder inOrder = inOrder(hooks.toArray());
-        inOrder.verify(hooks.get(2)).execute(null);
-        inOrder.verify(hooks.get(0)).execute(null);
-        inOrder.verify(hooks.get(1)).execute(null);
+        inOrder.verify(hooks.get(2)).execute(Matchers.<ScenarioResult>any());
+        inOrder.verify(hooks.get(0)).execute(Matchers.<ScenarioResult>any());
+        inOrder.verify(hooks.get(1)).execute(Matchers.<ScenarioResult>any());
     }
 
     @Test
@@ -41,12 +43,12 @@ public void after_hooks_execute_in_reverse_order() throws Throwable {
             world.addAfterHook(hook);
         }
 
-        world.runAfterHooksAndDisposeBackendWorlds();
+        world.runAfterHooksAndDisposeBackendWorlds(mock(Reporter.class));
 
         InOrder inOrder = inOrder(hooks.toArray());
-        inOrder.verify(hooks.get(1)).execute(null);
-        inOrder.verify(hooks.get(2)).execute(null);
-        inOrder.verify(hooks.get(0)).execute(null);
+        inOrder.verify(hooks.get(1)).execute(Matchers.<ScenarioResult>any());
+        inOrder.verify(hooks.get(2)).execute(Matchers.<ScenarioResult>any());
+        inOrder.verify(hooks.get(0)).execute(Matchers.<ScenarioResult>any());
     }
 
     @Test
@@ -60,19 +62,19 @@ public void hooks_order_across_many_backends() throws Throwable {
             world.addBeforeHook(hook);
         }
 
-        world.buildBackendWorldsAndRunBeforeHooks(new ArrayList<String>());
+        world.buildBackendWorldsAndRunBeforeHooks(new ArrayList<String>(), mock(Reporter.class));
 
         List<HookDefinition> allHooks = new ArrayList<HookDefinition>();
         allHooks.addAll(backend1Hooks);
         allHooks.addAll(backend2Hooks);
 
         InOrder inOrder = inOrder(allHooks.toArray());
-        inOrder.verify(backend1Hooks.get(2)).execute(null);
-        inOrder.verify(backend2Hooks.get(0)).execute(null);
-        inOrder.verify(backend1Hooks.get(0)).execute(null);
-        inOrder.verify(backend2Hooks.get(2)).execute(null);
-        verify(backend2Hooks.get(1)).execute(null);
-        verify(backend1Hooks.get(1)).execute(null);
+        inOrder.verify(backend1Hooks.get(2)).execute(Matchers.<ScenarioResult>any());
+        inOrder.verify(backend2Hooks.get(0)).execute(Matchers.<ScenarioResult>any());
+        inOrder.verify(backend1Hooks.get(0)).execute(Matchers.<ScenarioResult>any());
+        inOrder.verify(backend2Hooks.get(2)).execute(Matchers.<ScenarioResult>any());
+        verify(backend2Hooks.get(1)).execute(Matchers.<ScenarioResult>any());
+        verify(backend1Hooks.get(1)).execute(Matchers.<ScenarioResult>any());
     }
 
     private List<HookDefinition> mockHooks(int... ordering) {

core/src/test/java/cucumber/runtime/HookTest.java

@@ -1,7 +1,9 @@
 package cucumber.runtime;
 
+import gherkin.formatter.Reporter;
 import org.junit.Test;
 import org.mockito.InOrder;
+import org.mockito.Matchers;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -28,10 +30,10 @@ public void after_hooks_execute_before_objects_are_disposed() throws Throwable {
         World world = new World(runtime, TAGS);
         world.addAfterHook(hook);
 
-        world.runAfterHooksAndDisposeBackendWorlds();
+        world.runAfterHooksAndDisposeBackendWorlds(mock(Reporter.class));
 
         InOrder inOrder = inOrder(hook, backend);
-        inOrder.verify(hook).execute(null);
+        inOrder.verify(hook).execute(Matchers.<ScenarioResult>any());
         inOrder.verify(backend).disposeWorld();
     }
 

java/src/main/java/cucumber/runtime/java/JavaStepDefinition.java

@@ -3,7 +3,6 @@
 import cucumber.annotation.DateFormat;
 import cucumber.annotation.Pending;
 import cucumber.runtime.*;
-import gherkin.TagExpression;
 import gherkin.formatter.Argument;
 import gherkin.formatter.model.Step;
 
@@ -12,7 +11,6 @@
 import java.lang.reflect.Method;
 import java.lang.reflect.Type;
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.List;
 import java.util.regex.Pattern;
 

java/src/test/java/cucumber/runtime/java/JavaStepDefinitionTest.java

@@ -47,7 +47,7 @@ public void throws_ambiguous_when_two_matches_are_found() throws Throwable {
         backend.addStepDefinition(BAR.getAnnotation(Given.class), BAR);
 
         Reporter reporter = mock(Reporter.class);
-        fooWorld.buildBackendWorldsAndRunBeforeHooks(NO_PATHS);
+        fooWorld.buildBackendWorldsAndRunBeforeHooks(NO_PATHS, reporter);
         fooWorld.runStep("uri", new Step(NO_COMMENTS, "Given ", "pattern", 1, null, null), reporter, Locale.US);
     }
 
@@ -57,7 +57,7 @@ public void does_not_throw_ambiguous_when_nothing_is_ambiguous() throws Throwabl
         backend.addStepDefinition(FOO.getAnnotation(Given.class), FOO);
 
         Reporter reporter = mock(Reporter.class);
-        fooWorld.buildBackendWorldsAndRunBeforeHooks(NO_PATHS);
+        fooWorld.buildBackendWorldsAndRunBeforeHooks(NO_PATHS, reporter);
         Step step = new Step(NO_COMMENTS, "Given ", "pattern", 1, null, null);
         fooWorld.runStep("uri", step, reporter, Locale.US);
         assertTrue(defs.foo);

junit/src/main/java/cucumber/junit/ExecutionUnitRunner.java

@@ -47,31 +47,24 @@ protected Description describeChild(Step step) {
 
     @Override
     public void run(RunNotifier notifier) {
-        jUnitReporter.setStepParentRunner(this, notifier);
+        jUnitReporter.startExecutionUnit(this, notifier);
         /*
            We're running the hooks and background without reporting the steps as junit children - we don't want them to show up in the
            junit report. However, if any of the before hooks or background steps fail, we mark the entire scenario as failed. Scenario steps
            will be skipped.
         */
-        try {
-            world = cucumberScenario.buildWorldAndRunBeforeHooks(gluePaths, runtime);
-        } catch (Throwable e) {
-            notifier.fireTestFailure(new Failure(getDescription(), e));
-        }
+        world = cucumberScenario.newWorld(runtime);
+        world.buildBackendWorldsAndRunBeforeHooks(gluePaths, jUnitReporter);
         try {
             cucumberScenario.runBackground(jUnitReporter.getFormatter(), jUnitReporter.getReporter());
         } catch (Throwable e) {
             notifier.fireTestFailure(new Failure(getDescription(), e));
         }
 
-        // Run the steps
         cucumberScenario.format(jUnitReporter);
+        // Run the steps
         super.run(notifier);
-        try {
-            cucumberScenario.runAfterHooksAndDisposeWorld();
-        } catch (Throwable e) {
-            notifier.fireTestFailure(new Failure(getDescription(), e));
-        }
+        world.runAfterHooksAndDisposeBackendWorlds(jUnitReporter);
     }
 
     @Override

junit/src/main/java/cucumber/junit/JUnitReporter.java

@@ -26,6 +26,15 @@ public JUnitReporter(Reporter reporter, Formatter formatter) {
         this.formatter = formatter;
     }
 
+    public void startExecutionUnit(ExecutionUnitRunner executionUnitRunner, RunNotifier notifier) {
+        this.executionUnitRunner = executionUnitRunner;
+        this.notifier = notifier;
+
+        Description description = executionUnitRunner.getDescription();
+        stepNotifier = new EachTestNotifier(notifier, description);
+        stepNotifier.fireTestStarted();
+    }
+
     public void match(Match match) {
         Description description = executionUnitRunner.describeChild(steps.remove(0));
         stepNotifier = new EachTestNotifier(notifier, description);
@@ -101,11 +110,6 @@ public void close() {
         formatter.close();
     }
 
-    public void setStepParentRunner(ExecutionUnitRunner executionUnitRunner, RunNotifier notifier) {
-        this.executionUnitRunner = executionUnitRunner;
-        this.notifier = notifier;
-    }
-
     public Formatter getFormatter() {
         return formatter;
     }