Merge pull request #30 from cucumber/classes-per-scenario

Instantiate classes per scenario

Author: gaeljw

CHANGELOG.md

@@ -28,6 +28,8 @@ See also the [CHANGELOG](https://github.com/cucumber/cucumber-jvm/blob/master/CH
 
 ### Fixed
 
+- [Core] Instantiate glue classes per scenario ([#1](https://github.com/cucumber/cucumber-jvm-scala/issues/1) Gaël Jourdan-Weil)
+
 ### Security
 
 ## [4.7.1] (2019-08-01)
@@ -96,3 +98,16 @@ See also the [CHANGELOG](https://github.com/cucumber/cucumber-jvm/blob/master/CH
 
 - [Core] Update `cucumber-core` dependency to 4.1.0 (Glib Briia)
 - [Build] Update Scala versions to 2.11.12 and 2.12.7 ([#11](https://github.com/cucumber/cucumber-jvm-scala/issues/11) Arturas Smorgun)
+
+<!-- References -->
+[Unreleased]: https://github.com/cucumber/cucumber-jvm-scala/compare/v4.7.1...master
+[4.7.1]:  https://github.com/cucumber/cucumber-jvm-scala/compare/v4.7.0...v4.7.1
+[4.7.0]:  https://github.com/cucumber/cucumber-jvm-scala/compare/v4.6.0...v4.7.0
+[4.6.0]:  https://github.com/cucumber/cucumber-jvm-scala/compare/v4.5.4...v4.6.0
+[4.5.4]:  https://github.com/cucumber/cucumber-jvm-scala/compare/v4.5.3...v4.5.4
+[4.5.3]:  https://github.com/cucumber/cucumber-jvm-scala/compare/v4.4.0...v4.5.3
+[4.4.0]:  https://github.com/cucumber/cucumber-jvm-scala/compare/v4.3.1...v4.4.0
+[4.3.1]:  https://github.com/cucumber/cucumber-jvm-scala/compare/v4.3.0...v4.3.1
+[4.3.0]:  https://github.com/cucumber/cucumber-jvm-scala/compare/v4.2.6...v4.3.0
+[4.2.6]:  https://github.com/cucumber/cucumber-jvm-scala/compare/v4.2.0...v4.2.6
+[4.2.0]:  https://github.com/cucumber/cucumber-jvm-scala/compare/v4.1.0...v4.2.0

README.md

@@ -42,3 +42,10 @@ Any contribution is welcome:
 - developing a new feature
 
 Please use this Github project for contributing, either through an issue or a Pull Request.
+
+### Documentation
+
+These pages aim to help Cucumber Scala developers understand the codebase.
+
+- [Scala implementation details](docs/scala_implementation.md)
+- [Project structure](docs/project_structure.md)

docs/project_structure.md

@@ -0,0 +1,50 @@
+# Project structure
+
+The Cucumber Scala project is a Maven multimodule project:
+- `scala` module: contains the codebase of the Cucumber Scala implementation
+  - `scala_2.11` submodule: build for Scala 2.11.x
+  - `scala_2.12` submodule: build for Scala 2.12.x
+  - `scala_2.13` submodule: build for Scala 2.13.x
+- `examples` module: contains a sample project 
+
+## Cross compilation
+
+The cross compilation for the different Scala versions is handled with 3 different Maven projects: the submodules of the `scala` module.
+
+Each project has a different Scala version as dependency:
+```xml
+<dependency>
+    <groupId>org.scala-lang</groupId>
+    <artifactId>scala-compiler</artifactId>
+    <version>${scala.2.13.version}</version>
+    <scope>provided</scope>
+</dependency>
+```
+
+To not copy/paste the sources across the 3 projects, the sources are put in a separated folder called `sources` in the `scala` module.
+Each project uses it by defining the following properties:
+```xml
+<sourceDirectory>../sources/src/main/scala</sourceDirectory>
+<resources>
+    <resource>
+        <directory>../sources/src/main/resources</directory>
+    </resource>
+</resources>
+<testSourceDirectory>../sources/src/test/scala</testSourceDirectory>
+<testResources>
+    <testResource>
+        <directory>../sources/src/test/resources</directory>
+    </testResource>
+</testResources>
+```
+
+**Note:** when using your favorite IDE, you might have to "close" or "unload" 2 of the 3 projects.
+Some IDE are not able to handle shared sources because a source path can be attached to a single IDE project.
+If so, only loading the latest (`scala_2.13` project) is recommended.
+
+## Language traits generation
+
+The language traits (`io.cucumber.scala.EN` for instance) are generated automatically using
+a Groovy script at compile time.
+
+See in `sources/src/main/groovy/` folder.

docs/scala_implementation.md

@@ -0,0 +1,59 @@
+# Scala implementation details
+
+This page covers some details about the Cucumber Scala implementation.
+
+## Running a Cucumber test
+
+### Backend
+
+From Cucumber core perspective, the entrypoint of a Cucumber implementation is what is called "backend".
+
+The `BackendServiceLoader` core service looks for a `BackendProviderService` implementation.
+Ours is defined in the class `ScalaBackendProviderService`.
+
+The implementing class also has to be registered as a "Java Service" in the `META-INF/services/io.cucumber.core.backend.BackendProviderService` file (in the `resources` folder).
+
+### Loading the glue
+
+When a Cucumber test starts, a Cucumber Runner starts and a `ScalaBackend` instance is created.
+The `ScalaBackend` instance will be used for running all the scenarios which are part of the test (defined by the _features path_ and the _glue path_).
+
+The first thing the Runner does is to "load the glue", that is find all the hooks and step definitions and register them.
+This is handled by the `ScalaBackend#loadGlue()` method.
+
+#### Scala implementation
+
+In the Cucumber Scala implementation, loading the glue code means:
+- finding all the **classes** inheriting `io.cucumber.scala.ScalaDsl` in the _glue path_, and for each:
+  - add it to the `Container` instance provided by Cucumber Core
+- finding all the **objects** singletons instances inheriting `io.cucumber.scala.ScalaDsl` in the _glue path_ and for each:
+  - extract the hooks and step definitions from it
+  - add the definitions to the `Glue` instance provided by Cucumber Core, as NOT `ScenarioScoped`
+
+Ideally all the glue code should be instantiated further (see next section), this is why we register classes (actually a list of `Class`) to the Container.
+But this cannot work for objects because they are by definitions singletons and already instantiated way before Cucumber.
+Thus, objects are not registered in the Container and their lifecycle is out of Cucumber scope.
+
+### Running a scenario
+
+For each scenario, the `buildWorld()` method of the backend is called.
+This is where the glue code should be initialized.
+
+#### Scala implementation
+
+For each **class** identified when loading the glue:
+- an instance is created by the `Lookup` provided by Cucumber Core
+- hooks and steps definitions are extracted from it
+- definitions are added to the `Glue` instance provided by Cucumber Core, as `ScenarioScoped`
+
+Being `ScenarioScoped` ensure instances are flushed at the end of the scenario and recreated for the next one.
+
+## Scala DSL
+
+The Scala DSL is made in a way that any class instance or object extending it contains what we call a **registry**:
+a list of the hooks and step definitions it contains.
+This is the purpose of `ScalaDslRegistry`.
+
+The registry is populated when the class instance or the object is created.
+Unlike other implementations there is no need to use annotations or reflection here.
+This is actually **similar to the Java8/Lambda implementation**.

docs/upgrade_v5.md

@@ -51,3 +51,17 @@ Before { _ =>
 ```
 
 See also the [Hooks documentation](hooks.md).
+
+## Under the hood
+
+### Instantiate glue classes per scenario
+
+Before Cucumber Scala 5.x, glue classes (classes extending `ScalaDsl`) were instantiated only once for a test suite.
+
+This means that if you wanted to keep state between steps of your scenarios, you had to make sure the state was not shared to other scenarios by using hooks or manual checks.
+
+Starting from Cucumber Scala 5.x, **each scenario creates new glue class instances**.
+
+You should not notice any change unless you rely on state kept between scenarios in your glue classes.
+Please note that this is not the proper way to keep a state.
+You might want to use an `object` for this purpose.  

scala/pom.xml

@@ -13,7 +13,7 @@
 
     <modules>
         <module>scala_2.11</module>
-	<module>scala_2.13</module>
+	    <module>scala_2.12</module>
     </modules>
 
     <profiles>
@@ -23,7 +23,7 @@
                 <jdk>1.8</jdk>
             </activation>
             <modules>
-                <module>scala_2.12</module>
+                <module>scala_2.13</module>
             </modules>
         </profile>
     </profiles>

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

@@ -1,15 +1,32 @@
 package io.cucumber.scala
 
-import io.cucumber.core.backend.Located
+import java.lang.reflect.InvocationTargetException
 
-abstract class AbstractGlueDefinition(location: StackTraceElement) extends Located {
+import io.cucumber.core.backend.{CucumberInvocationTargetException, Located}
 
-  def getLocation(): String = {
+import scala.util.{Failure, Try}
+
+trait AbstractGlueDefinition extends Located {
+
+  val location: StackTraceElement
+
+  override def getLocation(): String = {
     location.toString
   }
 
-  def isDefinedAt(stackTraceElement: StackTraceElement): Boolean = {
+  override def isDefinedAt(stackTraceElement: StackTraceElement): Boolean = {
     location.getFileName != null && location.getFileName == stackTraceElement.getFileName
   }
 
+  /**
+   * Executes the block of code and handle failures in the way asked by Cucumber specification: that is throwing a CucumberInvocationTargetException.
+   */
+  protected def executeAsCucumber(block: => Unit): Unit = {
+    Try(block)
+      .recoverWith {
+        case ex => Failure(new CucumberInvocationTargetException(this, new InvocationTargetException(ex)))
+      }
+      .get
+  }
+
 }

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

@@ -4,12 +4,18 @@ import io.cucumber.core.backend.Glue
 
 class GlueAdaptor(glue: Glue) {
 
-  def addDefinition(registry: ScalaDslRegistry): Unit = {
-    registry.stepDefinitions.foreach(glue.addStepDefinition)
-    registry.beforeHooks.foreach(glue.addBeforeHook)
-    registry.afterHooks.foreach(glue.addAfterHook)
-    registry.afterStepHooks.foreach(glue.addAfterStepHook)
-    registry.beforeStepHooks.foreach(glue.addBeforeStepHook)
+  /**
+   * Load the step definitions and hooks from a ScalaDsl instance into the glue.
+   *
+   * @param registry       ScalaDsl instance registry
+   * @param scenarioScoped true for class instances, false for object singletons
+   */
+  def loadRegistry(registry: ScalaDslRegistry, scenarioScoped: Boolean): Unit = {
+    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)
   }
 
 }

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

@@ -11,53 +11,58 @@ import io.cucumber.core.resource.{ClasspathScanner, ClasspathSupport}
 import scala.collection.JavaConverters._
 import scala.util.Try
 
-class ScalaBackend(classLoaderProvider: Supplier[ClassLoader]) extends Backend {
+class ScalaBackend(lookup: Lookup, container: Container, classLoaderProvider: Supplier[ClassLoader]) extends Backend {
 
   private val classFinder = new ClasspathScanner(classLoaderProvider)
 
-  private[scala] var scalaGlueInstances: Seq[ScalaDsl] = Nil
+  private var glueAdaptor: GlueAdaptor = _
+  private[scala] var scalaGlueClasses: Seq[Class[_ <: ScalaDsl]] = Nil
 
   override def disposeWorld(): Unit = {
-    scalaGlueInstances = Nil
+    // Nothing to do
   }
 
   override def getSnippet(): Snippet = {
     new ScalaSnippet()
   }
 
   override def buildWorld(): Unit = {
-    // Nothing to do
+    // Instantiate all the glue classes and load the glue code from them
+    scalaGlueClasses.foreach { glueClass =>
+      val glueInstance = lookup.getInstance(glueClass)
+      glueAdaptor.loadRegistry(glueInstance.registry, scenarioScoped = true)
+    }
   }
 
   override def loadGlue(glue: Glue, gluePaths: JList[URI]): Unit = {
 
+    glueAdaptor = new GlueAdaptor(glue)
+
     val dslClasses = gluePaths.asScala
       .filter(gluePath => ClasspathSupport.CLASSPATH_SCHEME.equals(gluePath.getScheme))
       .map(ClasspathSupport.packageName)
       .flatMap(basePackageName => classFinder.scanForSubClassesInPackage(basePackageName, classOf[ScalaDsl]).asScala)
       .filter(glueClass => !glueClass.isInterface)
-      .filter(glueClass => glueClass.getConstructors.length > 0)
 
     val (clsClasses, objClasses) = dslClasses.partition(isRegularClass)
 
+    // Retrieve Scala objects (singletons)
     val objInstances = objClasses.map { cls =>
       val instField = cls.getDeclaredField("MODULE$")
       instField.setAccessible(true)
       instField.get(null).asInstanceOf[ScalaDsl]
     }
-    val clsInstances = clsClasses.map {
-      _.newInstance()
-    }
-
-    // FIXME we should not create instances above but fill the container like Cucumber Java does
-    // https://github.com/cucumber/cucumber-jvm-scala/issues/1
-    //clsClasses.foreach(container.addClass(_))
-    scalaGlueInstances = objInstances.toSeq ++ clsInstances
 
-    val glueAdaptor = new GlueAdaptor(glue)
+    // Regular Scala classes are added to the container, they will be instantiated by the container depending on its logic
+    // Object are not because by definition they are singletons
+    clsClasses.foreach { glueClass =>
+      container.addClass(glueClass)
+      scalaGlueClasses = scalaGlueClasses :+ glueClass
+    }
 
-    scalaGlueInstances.foreach { glueInstance =>
-      glueAdaptor.addDefinition(glueInstance.registry)
+    // For object, we add the definitions here, once for all
+    objInstances.foreach { glueInstance =>
+      glueAdaptor.loadRegistry(glueInstance.registry, scenarioScoped = false)
     }
 
     ()

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

@@ -7,7 +7,7 @@ import io.cucumber.core.backend.{Backend, BackendProviderService, Container, Loo
 class ScalaBackendProviderService extends BackendProviderService {
 
   override def create(lookup: Lookup, container: Container, classLoaderSupplier: Supplier[ClassLoader]): Backend = {
-    new ScalaBackend(classLoaderSupplier)
+    new ScalaBackend(lookup, container, classLoaderSupplier)
   }
 
 }