Raise an exception when loading glue code if hooks are not properly defined

And use immutable types with var rather than mutable types with val (ScalaDslRegistry)

Author: gaeljw

CHANGELOG.md

@@ -19,6 +19,8 @@ See also the [CHANGELOG](https://github.com/cucumber/cucumber-jvm/blob/master/CH
 
 ### Fixed
 
+- [Scala DSL] Raise an exception at runtime if hooks are not correctly defined ([#60](https://github.com/cucumber/cucumber-jvm-scala/issues/60) Gaël Jourdan-Weil)
+
 ## [5.7.0] (2020-05-10)
 
 ### Added

docs/hooks.md

@@ -16,11 +16,13 @@ Scenario hooks run for every scenario.
 ```scala
 Before { scenario : Scenario =>
   // Do something before each scenario
+  // Must return Unit
 }
 
 // Or:
 Before {
   // Do something before each scenario
+  // Must return Unit
 }
 ```
 
@@ -31,11 +33,13 @@ Before {
 ```scala
 After { scenario : Scenario =>
   // Do something after each scenario
+  // Must return Unit
 }
 
 // Or:
 After {
   // Do something after each scenario
+  // Must return Unit
 }
 ```
 
@@ -48,11 +52,13 @@ Step hooks invoked before and after a step.
 ```scala
 BeforeStep { scenario : Scenario =>
   // Do something before step
+  // Must return Unit
 }
 
 // Or:
 BeforeStep {
   // Do something before step
+  // Must return Unit
 }
 ```
 
@@ -61,11 +67,13 @@ BeforeStep {
 ```scala
 AfterStep { scenario : Scenario =>
   // Do something after step
+  // Must return Unit
 }
 
 // Or:
 AfterStep {
   // Do something after step
+  // Must return Unit
 }
 ```
 
@@ -76,6 +84,7 @@ Hooks can be conditionally selected for execution based on the tags of the scena
 ```scala
 Before("@browser and not @headless") { 
   // Do something before each scenario with tag @browser but not @headless
+  // Must return Unit
 }
 ```
 
@@ -86,10 +95,12 @@ You can define an order between multiple hooks.
 ```scala
 Before(10) { 
   // Do something before each scenario
+  // Must return Unit
 }
 
 Before(20) { 
   // Do something before each scenario
+  // Must return Unit
 }
 ```
 
@@ -101,5 +112,6 @@ You mix up conditional and order hooks with following syntax:
 ```scala
 Before("@browser and not @headless", 10) {
   // Do something before each scenario
+  // Must return Unit
 }
 ```

scala/sources/src/main/scala/io/cucumber/scala/GlueAdaptor.scala

@@ -11,11 +11,17 @@ class GlueAdaptor(glue: Glue) {
    * @param scenarioScoped true for class instances, false for object singletons
    */
   def loadRegistry(registry: ScalaDslRegistry, scenarioScoped: Boolean): Unit = {
+
+    // If the registry is not consistent, this indicates a mistake in the users definition and we want to let him know.
+    registry.checkConsistency().left.foreach { ex: IncorrectHookDefinitionException =>
+      throw ex
+    }
+
     registry.stepDefinitions.map(ScalaStepDefinition(_, scenarioScoped)).foreach(glue.addStepDefinition)
     registry.beforeHooks.map(ScalaHookDefinition(_, scenarioScoped)).foreach(glue.addBeforeHook)
     registry.afterHooks.map(ScalaHookDefinition(_, scenarioScoped)).foreach(glue.addAfterHook)
-    registry.afterStepHooks.map(ScalaHookDefinition(_, scenarioScoped)).foreach(glue.addAfterStepHook)
     registry.beforeStepHooks.map(ScalaHookDefinition(_, scenarioScoped)).foreach(glue.addBeforeStepHook)
+    registry.afterStepHooks.map(ScalaHookDefinition(_, scenarioScoped)).foreach(glue.addAfterStepHook)
     registry.docStringTypes.map(ScalaDocStringTypeDefinition(_, scenarioScoped)).foreach(glue.addDocStringType)
     registry.dataTableTypes.map(ScalaDataTableTypeDefinition(_, scenarioScoped)).foreach(glue.addDataTableType)
     registry.parameterTypes.map(ScalaParameterTypeDefinition(_, scenarioScoped)).foreach(glue.addParameterType)

scala/sources/src/main/scala/io/cucumber/scala/IncorrectHookDefinitionException.scala

@@ -0,0 +1,38 @@
+package io.cucumber.scala
+
+import io.cucumber.core.backend.CucumberBackendException
+
+object IncorrectHookDefinitionException {
+
+  def errorMessage(expectedHooks: Seq[UndefinedHook]): String = {
+    val hooksListToDisplay = expectedHooks.map { eh =>
+      s" - ${eh.stackTraceElement.getFileName}:${eh.stackTraceElement.getLineNumber} (${eh.hookType})"
+    }
+
+    s"""Some hooks are not defined properly:
+       |${hooksListToDisplay.mkString("\n")}
+       |
+       |This can be caused by defining hooks where the body returns a Int or String rather than Unit.
+       |
+       |For instance, the following code:
+       |
+       |  Before {
+       |    someInitMethodReturningInt()
+       |  }
+       |
+       |Should be replaced with:
+       |
+       |  Before {
+       |    someInitMethodReturningInt()
+       |    ()
+       |  }
+       |""".stripMargin
+  }
+
+}
+
+class IncorrectHookDefinitionException(val undefinedHooks: Seq[UndefinedHook]) extends CucumberBackendException(IncorrectHookDefinitionException.errorMessage(undefinedHooks)) {
+
+}
+
+case class UndefinedHook(hookType: HookType, stackTraceElement: StackTraceElement)

scala/sources/src/main/scala/io/cucumber/scala/ScalaDsl.scala

@@ -22,21 +22,22 @@ trait ScalaDsl extends BaseScalaDsl with StepDsl with HookDsl with DataTableType
 
 }
 
-private[scala] trait HookDsl extends BaseScalaDsl {
+sealed trait HookType
 
-  private sealed trait HookType
+object HookType {
 
-  private object HookType {
+  case object BEFORE extends HookType
 
-    case object BEFORE extends HookType
+  case object BEFORE_STEP extends HookType
 
-    case object BEFORE_STEP extends HookType
+  case object AFTER extends HookType
 
-    case object AFTER extends HookType
+  case object AFTER_STEP extends HookType
 
-    case object AFTER_STEP extends HookType
+}
 
-  }
+private[scala] trait HookDsl extends BaseScalaDsl {
+  self =>
 
   /**
    * Defines an before hook.
@@ -63,7 +64,7 @@ private[scala] trait HookDsl extends BaseScalaDsl {
    * @param tagExpression a tag expression, if the expression applies to the current scenario this hook will be executed
    * @param order         the order in which this hook should run. Higher numbers are run first
    */
-  def Before(tagExpression: String, order: Int) = new HookBody(HookType.BEFORE, tagExpression, order)
+  def Before(tagExpression: String, order: Int) = new HookBody(HookType.BEFORE, tagExpression, order, Utils.frame(self))
 
   /**
    * Defines an before step hook.
@@ -90,7 +91,7 @@ private[scala] trait HookDsl extends BaseScalaDsl {
    * @param tagExpression a tag expression, if the expression applies to the current scenario this hook will be executed
    * @param order         the order in which this hook should run. Higher numbers are run first
    */
-  def BeforeStep(tagExpression: String, order: Int) = new HookBody(HookType.BEFORE_STEP, tagExpression, order)
+  def BeforeStep(tagExpression: String, order: Int) = new HookBody(HookType.BEFORE_STEP, tagExpression, order, Utils.frame(self))
 
   /**
    * Defines and after hook.
@@ -117,7 +118,7 @@ private[scala] trait HookDsl extends BaseScalaDsl {
    * @param tagExpression a tag expression, if the expression applies to the current scenario this hook will be executed
    * @param order         the order in which this hook should run. Higher numbers are run first
    */
-  def After(tagExpression: String, order: Int) = new HookBody(HookType.AFTER, tagExpression, order)
+  def After(tagExpression: String, order: Int) = new HookBody(HookType.AFTER, tagExpression, order, Utils.frame(self))
 
   /**
    * Defines and after step hook.
@@ -144,26 +145,21 @@ private[scala] trait HookDsl extends BaseScalaDsl {
    * @param tagExpression a tag expression, if the expression applies to the current scenario this hook will be executed
    * @param order         the order in which this hook should run. Higher numbers are run first
    */
-  def AfterStep(tagExpression: String, order: Int) = new HookBody(HookType.AFTER_STEP, tagExpression, order)
+  def AfterStep(tagExpression: String, order: Int) = new HookBody(HookType.AFTER_STEP, tagExpression, order, Utils.frame(self))
+
+  final class HookBody(hookType: HookType, tagExpression: String, order: Int, frame: StackTraceElement) {
 
-  final class HookBody(hookType: HookType, tagExpression: String, order: Int) {
+    // When a HookBody is created, we want to ensure that the apply method is called
+    // To be able to check this, we notice the registry to expect a hook
+    registry.expectHook(hookType, frame)
 
     def apply(body: => Unit): Unit = {
       apply(_ => body)
     }
 
     def apply(body: Scenario => Unit): Unit = {
       val details = ScalaHookDetails(tagExpression, order, body)
-      hookType match {
-        case HookType.BEFORE =>
-          registry.beforeHooks += details
-        case HookType.BEFORE_STEP =>
-          registry.beforeStepHooks += details
-        case HookType.AFTER =>
-          registry.afterHooks += details
-        case HookType.AFTER_STEP =>
-          registry.afterStepHooks += details
-      }
+      registry.registerHook(hookType, details, frame)
     }
 
   }
@@ -181,7 +177,7 @@ private[scala] trait DocStringTypeDsl extends BaseScalaDsl {
    * @tparam T type to convert to
    */
   def DocStringType[T](contentType: String)(body: DocStringDefinitionBody[T])(implicit ev: ClassTag[T]): Unit = {
-    registry.docStringTypes += ScalaDocStringTypeDetails[T](contentType, body, ev)
+    registry.registerDocStringType(ScalaDocStringTypeDetails[T](contentType, body, ev))
   }
 
 }
@@ -209,19 +205,19 @@ private[scala] trait DataTableTypeDsl extends BaseScalaDsl {
   final class DataTableTypeBody(replaceWithEmptyString: Seq[String]) {
 
     def apply[T](body: DataTableEntryDefinitionBody[T])(implicit ev: ClassTag[T]): Unit = {
-      registry.dataTableTypes += ScalaDataTableEntryTypeDetails[T](replaceWithEmptyString, body, ev)
+      registry.registerDataTableType(ScalaDataTableEntryTypeDetails[T](replaceWithEmptyString, body, ev))
     }
 
     def apply[T](body: DataTableRowDefinitionBody[T])(implicit ev: ClassTag[T]): Unit = {
-      registry.dataTableTypes += ScalaDataTableRowTypeDetails[T](replaceWithEmptyString, body, ev)
+      registry.registerDataTableType(ScalaDataTableRowTypeDetails[T](replaceWithEmptyString, body, ev))
     }
 
     def apply[T](body: DataTableCellDefinitionBody[T])(implicit ev: ClassTag[T]): Unit = {
-      registry.dataTableTypes += ScalaDataTableCellTypeDetails[T](replaceWithEmptyString, body, ev)
+      registry.registerDataTableType(ScalaDataTableCellTypeDetails[T](replaceWithEmptyString, body, ev))
     }
 
     def apply[T](body: DataTableDefinitionBody[T])(implicit ev: ClassTag[T]): Unit = {
-      registry.dataTableTypes += ScalaDataTableTableTypeDetails[T](replaceWithEmptyString, body, ev)
+      registry.registerDataTableType(ScalaDataTableTableTypeDetails[T](replaceWithEmptyString, body, ev))
     }
 
   }
@@ -398,7 +394,7 @@ private[scala] trait ParameterTypeDsl extends BaseScalaDsl {
     }
 
     private def register[R](pf: PartialFunction[List[String], R])(implicit tag: ClassTag[R]): Unit = {
-      registry.parameterTypes += ScalaParameterTypeDetails[R](name, regex, pf, tag)
+      registry.registerParameterType(ScalaParameterTypeDetails[R](name, regex, pf, tag))
     }
 
   }
@@ -413,7 +409,7 @@ private[scala] trait DefaultTransformerDsl extends BaseScalaDsl {
    * @param body converts `String` argument to an instance of the `Type` argument
    */
   def DefaultParameterTransformer(body: DefaultParameterTransformerBody): Unit = {
-    registry.defaultParameterTransformers += ScalaDefaultParameterTransformerDetails(body)
+    registry.registerDefaultParameterTransformer(ScalaDefaultParameterTransformerDetails(body))
   }
 
   /**
@@ -441,7 +437,7 @@ private[scala] trait DefaultTransformerDsl extends BaseScalaDsl {
   }
 
   private def DefaultDataTableCellTransformer(replaceWithEmptyString: Seq[String])(body: DefaultDataTableCellTransformerBody): Unit = {
-    registry.defaultDataTableCellTransformers += ScalaDefaultDataTableCellTransformerDetails(replaceWithEmptyString, body)
+    registry.registerDefaultDataTableCellTransformer(ScalaDefaultDataTableCellTransformerDetails(replaceWithEmptyString, body))
   }
 
   /**
@@ -468,7 +464,7 @@ private[scala] trait DefaultTransformerDsl extends BaseScalaDsl {
   }
 
   private def DefaultDataTableEntryTransformer(replaceWithEmptyString: Seq[String])(body: DefaultDataTableEntryTransformerBody): Unit = {
-    registry.defaultDataTableEntryTransformers += ScalaDefaultDataTableEntryTransformerDetails(replaceWithEmptyString, body)
+    registry.registerDefaultDataTableEntryTransformer(ScalaDefaultDataTableEntryTransformerDetails(replaceWithEmptyString, body))
   }
 
 }
@@ -911,7 +907,7 @@ private[scala] trait StepDsl extends BaseScalaDsl {
     }
 
     private def register(manifests: Manifest[_ <: Any]*)(pf: PartialFunction[List[Any], Any]): Unit = {
-      registry.stepDefinitions += ScalaStepDetails(Utils.frame(self), name, regex, manifests, pf)
+      registry.registerStep(ScalaStepDetails(Utils.frame(self), name, regex, manifests, pf))
     }
 
   }