[Spring] Start and stop test context once per scenario (#2517)

The `TestContextAdaptor` would start the scenario scope once at the start of each test, then another one to run the after test method hooks. Refactored code to make the order of operations more obvious. Also prevented scenario scopes from being registered more then once. Though because `CucumberScenarioScope` delegates everything to `CucumberTestContext` this effectively only reduces the log spam a bit at debug level. Fixes: #2516
cucumber · Apr 10, 2022 · f5f1ace · f5f1ace
1 parent 3df1505
commit f5f1ace
Show file tree

Hide file tree

Showing 6 changed files with 53 additions and 26 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -28,6 +28,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
     - `org.glassfish.jaxb:jaxb-runtime`
 - [TestNG] Remove spurious Optional\[<Feature Name>] from test name ([#2488](https://github.com/cucumber/cucumber-jvm/pull/2488) M.P. Korstanje)
 * [BOM] Add missing dependencies to bill of materials ([#2496](https://github.com/cucumber/cucumber-jvm/pull/2496) M.P. Korstanje)
+* [Spring] Start and stop test context once per scenario ([#2517](https://github.com/cucumber/cucumber-jvm/pull/2517) M.P. Korstanje)
 
 ## [7.2.3] (2022-01-13)
 

diff --git a/spring/src/main/java/io/cucumber/spring/CucumberScenarioScope.java b/spring/src/main/java/io/cucumber/spring/CucumberScenarioScope.java
@@ -37,7 +37,9 @@ public Object resolveContextualObject(String key) {
     @Override
     public String getConversationId() {
         CucumberTestContext context = CucumberTestContext.getInstance();
-        return context.getId();
+        return context.getId()
+                .map(id -> "cucumber_test_context_" + id)
+                .orElse(null);
     }
 
 }
diff --git a/spring/src/main/java/io/cucumber/spring/CucumberTestContext.java b/spring/src/main/java/io/cucumber/spring/CucumberTestContext.java
@@ -4,6 +4,7 @@
 
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Optional;
 import java.util.concurrent.atomic.AtomicInteger;
 
 @API(status = API.Status.STABLE)
@@ -31,8 +32,8 @@ void start() {
         sessionId = sessionCounter.incrementAndGet();
     }
 
-    String getId() {
-        return "cucumber_test_context_" + sessionId;
+    Optional<Integer> getId() {
+        return Optional.ofNullable(sessionId);
     }
 
     void stop() {

diff --git a/spring/src/main/java/io/cucumber/spring/SpringFactory.java b/spring/src/main/java/io/cucumber/spring/SpringFactory.java
@@ -21,8 +21,6 @@
 import java.util.HashSet;
 import java.util.Set;
 
-import static io.cucumber.spring.TestContextAdaptor.createTestContextManagerAdaptor;
-
 /**
  * Spring based implementation of ObjectFactory.
  * <p>
@@ -151,7 +149,7 @@ public void start() {
         // a singleton and reused between scenarios and shared between
         // threads.
         TestContextManager testContextManager = new TestContextManager(withCucumberContextConfiguration);
-        testContextAdaptor = createTestContextManagerAdaptor(testContextManager, stepClasses);
+        testContextAdaptor = new TestContextAdaptor(testContextManager, stepClasses);
         testContextAdaptor.start();
     }
 

diff --git a/spring/src/main/java/io/cucumber/spring/TestContextAdaptor.java b/spring/src/main/java/io/cucumber/spring/TestContextAdaptor.java
@@ -2,6 +2,7 @@
 
 import io.cucumber.core.backend.CucumberBackendException;
 import org.springframework.beans.factory.config.ConfigurableListableBeanFactory;
+import org.springframework.beans.factory.config.Scope;
 import org.springframework.beans.factory.support.BeanDefinitionBuilder;
 import org.springframework.beans.factory.support.BeanDefinitionRegistry;
 import org.springframework.context.ConfigurableApplicationContext;
@@ -13,34 +14,24 @@
 
 import static io.cucumber.spring.CucumberTestContext.SCOPE_CUCUMBER_GLUE;
 
-abstract class TestContextAdaptor {
+class TestContextAdaptor {
 
     private static final Object monitor = new Object();
 
     private final TestContextManager delegate;
     private final ConfigurableApplicationContext applicationContext;
     private final Collection<Class<?>> glueClasses;
 
-    protected TestContextAdaptor(
-            TestContextManager delegate,
-            ConfigurableApplicationContext applicationContext,
-            Collection<Class<?>> glueClasses
-    ) {
-        this.delegate = delegate;
-        this.applicationContext = applicationContext;
-        this.glueClasses = glueClasses;
-    }
-
-    static TestContextAdaptor createTestContextManagerAdaptor(
+    TestContextAdaptor(
             TestContextManager delegate,
             Collection<Class<?>> glueClasses
     ) {
         TestContext testContext = delegate.getTestContext();
         ConfigurableApplicationContext applicationContext = (ConfigurableApplicationContext) testContext
                 .getApplicationContext();
-        return new TestContextAdaptor(delegate, applicationContext, glueClasses) {
-
-        };
+        this.delegate = delegate;
+        this.applicationContext = applicationContext;
+        this.glueClasses = glueClasses;
     }
 
     public final void start() {
@@ -50,15 +41,15 @@ public final void start() {
         // #1153, #1148, #1106) we do this serially.
         synchronized (monitor) {
             registerGlueCodeScope(applicationContext);
-            notifyContextManagerAboutTestClassStarted();
             registerStepClassBeanDefinitions(applicationContext.getBeanFactory());
         }
+        notifyContextManagerAboutBeforeTestClass();
+        CucumberTestContext.getInstance().start();
         notifyTestContextManagerAboutBeforeTestMethod();
     }
 
     private void notifyTestContextManagerAboutBeforeTestMethod() {
         try {
-            CucumberTestContext.getInstance().start();
             Class<?> testClass = delegate.getTestContext().getTestClass();
             Object testContextInstance = applicationContext.getBean(testClass);
             Method dummyMethod = TestContextAdaptor.class.getMethod("cucumberDoesNotHaveASingleTestMethod");
@@ -70,12 +61,18 @@ private void notifyTestContextManagerAboutBeforeTestMethod() {
 
     final void registerGlueCodeScope(ConfigurableApplicationContext context) {
         while (context != null) {
-            context.getBeanFactory().registerScope(SCOPE_CUCUMBER_GLUE, new CucumberScenarioScope());
+            ConfigurableListableBeanFactory beanFactory = context.getBeanFactory();
+            // Scenario scope may have already been registered by another
+            // thread.
+            Scope registeredScope = beanFactory.getRegisteredScope(SCOPE_CUCUMBER_GLUE);
+            if (registeredScope == null) {
+                beanFactory.registerScope(SCOPE_CUCUMBER_GLUE, new CucumberScenarioScope());
+            }
             context = (ConfigurableApplicationContext) context.getParent();
         }
     }
 
-    private void notifyContextManagerAboutTestClassStarted() {
+    private void notifyContextManagerAboutBeforeTestClass() {
         try {
             delegate.beforeTestClass();
         } catch (Exception e) {
@@ -106,6 +103,10 @@ private void registerStepClassBeanDefinition(BeanDefinitionRegistry registry, Cl
     public final void stop() {
         notifyTestContextManagerAboutAfterTestMethod();
         CucumberTestContext.getInstance().stop();
+        notifyTestContextManagerAboutAfterTestClass();
+    }
+
+    private void notifyTestContextManagerAboutAfterTestClass() {
         try {
             delegate.afterTestClass();
         } catch (Exception e) {
@@ -115,7 +116,6 @@ public final void stop() {
 
     private void notifyTestContextManagerAboutAfterTestMethod() {
         try {
-            CucumberTestContext.getInstance().start();
             Class<?> testClass = delegate.getTestContext().getTestClass();
             Object testContextInstance = applicationContext.getBean(testClass);
             Method dummyMethod = TestContextAdaptor.class.getMethod("cucumberDoesNotHaveASingleTestMethod");

diff --git a/spring/src/test/java/io/cucumber/spring/SpringFactoryTest.java b/spring/src/test/java/io/cucumber/spring/SpringFactoryTest.java
@@ -23,6 +23,8 @@
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.test.context.ContextConfiguration;
 
+import java.util.Optional;
+
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.startsWith;
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -32,6 +34,8 @@
 import static org.hamcrest.core.IsNull.notNullValue;
 import static org.junit.jupiter.api.Assertions.assertAll;
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -59,6 +63,27 @@ void shouldGiveUsNewStepInstancesForEachScenario() {
             () -> assertThat(o2, is(not(equalTo(o1)))));
     }
 
+    @Test
+    void shouldStartOneCucumberContextForEachScenario() {
+        final ObjectFactory factory = new SpringFactory();
+        factory.addClass(BellyStepDefinitions.class);
+
+        // Scenario 1
+        assertTrue(CucumberTestContext.getInstance().getId().isEmpty());
+        factory.start();
+        Optional<Integer> testContextId1 = CucumberTestContext.getInstance().getId();
+        factory.stop();
+
+        // Scenario 2
+        assertTrue(CucumberTestContext.getInstance().getId().isEmpty());
+        factory.start();
+        Optional<Integer> testContextId2 = CucumberTestContext.getInstance().getId();
+        factory.stop();
+        assertTrue(CucumberTestContext.getInstance().getId().isEmpty());
+
+        assertEquals(testContextId1.get() + 1, testContextId2.get());
+    }
+
     @Test
     void shouldNeverCreateNewApplicationBeanInstances() {
         // Feature 1