[Pico] Fixed missing calls to start, stop and dispose handles (#2772)

Corrected the pico-container lifecycle: the start, stop and dispose
methods are now called for every test scenarios.

The code is the same as before #2725 (so it's safer in terms of
lifecycle), except that the pico container is recycled in the `start()`
method when it already exists. 

When a second call to `start()` is done (e.g. for the 2nd scenario), 
the container component instances are all in disposed state and the
container lifecycle is in disposed state. The recycling operation
consists in removing all container component instances (i.e. flushing
the cache) and resetting the container lifecycle.


Co-authored-by: Julien Kronegg <julien [at] kronegg.ch>

Author: jkronegg

CHANGELOG.md

@@ -13,6 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Changed
 - [TestNG] Update dependency org.testng:testng to v7.8.0
 
+### Fixed
+- [Pico] Fixed missing calls to start, stop and dispose handles ([#2772](https://github.com/cucumber/cucumber-jvm/pull/2772) Julien Kronegg)
+
 ## [7.12.1] - 2023-06-02
 ### Fixed
 - [Core] Set html report viewport width to device width ([html-formatter/#238](https://github.com/cucumber/html-formatter/pull/238)  Tim Yao )

cucumber-picocontainer/src/main/java/io/cucumber/picocontainer/PicoFactory.java

@@ -5,6 +5,7 @@
 import org.picocontainer.MutablePicoContainer;
 import org.picocontainer.PicoBuilder;
 import org.picocontainer.behaviors.Cached;
+import org.picocontainer.lifecycle.DefaultLifecycleState;
 
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Modifier;
@@ -15,36 +16,45 @@
 public final class PicoFactory implements ObjectFactory {
 
     private final Set<Class<?>> classes = new HashSet<>();
-    private final MutablePicoContainer pico = new PicoBuilder()
-            .withCaching()
-            .withLifecycle()
-            .build();
-
-    public PicoFactory() {
-        this.pico.start();
-    }
+    private MutablePicoContainer pico;
 
     private static boolean isInstantiable(Class<?> clazz) {
         boolean isNonStaticInnerClass = !Modifier.isStatic(clazz.getModifiers()) && clazz.getEnclosingClass() != null;
         return Modifier.isPublic(clazz.getModifiers()) && !Modifier.isAbstract(clazz.getModifiers())
                 && !isNonStaticInnerClass;
     }
 
+    @Override
     public void start() {
-        // do nothing (was already started in constructor)
+        if (pico == null) {
+            pico = new PicoBuilder()
+                    .withCaching()
+                    .withLifecycle()
+                    .build();
+            for (Class<?> clazz : classes) {
+                pico.addComponent(clazz);
+            }
+        } else {
+            // we already get a pico container which is in "disposed" lifecycle,
+            // so recycle it by defining a new lifecycle and removing all
+            // instances
+            pico.setLifecycleState(new DefaultLifecycleState());
+            pico.getComponentAdapters()
+                    .forEach(cached -> ((Cached<?>) cached).flush());
+        }
+        pico.start();
     }
 
     @Override
     public void stop() {
-        pico.getComponentAdapters()
-                .forEach(cached -> ((Cached<?>) cached).flush());
+        pico.stop();
+        pico.dispose();
     }
 
     @Override
     public boolean addClass(Class<?> clazz) {
         if (isInstantiable(clazz) && classes.add(clazz)) {
             addConstructorDependencies(clazz);
-            pico.addComponent(clazz);
         }
         return true;
     }

cucumber-picocontainer/src/test/java/io/cucumber/picocontainer/DisposableCucumberBelly.java

@@ -3,8 +3,11 @@
 import org.picocontainer.Disposable;
 import org.picocontainer.Startable;
 
+import java.util.ArrayList;
 import java.util.List;
 
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
 /**
  * A test helper class which simulates a class that holds system resources which
  * need disposing at the end of the test.
@@ -13,19 +16,20 @@
  */
 public class DisposableCucumberBelly
         implements Disposable, Startable {
+    static final List<String> events = new ArrayList<>();
 
     private List<String> contents;
     private boolean isDisposed = false;
     private boolean wasStarted = false;
     private boolean wasStopped = false;
 
     public List<String> getContents() {
-        assert !isDisposed;
+        assertFalse(isDisposed);
         return contents;
     }
 
     public void setContents(List<String> contents) {
-        assert !isDisposed;
+        assertFalse(isDisposed);
         this.contents = contents;
     }
 
@@ -36,6 +40,7 @@ public void setContents(List<String> contents) {
      */
     @Override
     public void dispose() {
+        events.add("Disposed");
         isDisposed = true;
     }
 
@@ -45,6 +50,7 @@ public boolean isDisposed() {
 
     @Override
     public void start() {
+        events.add("Started");
         wasStarted = true;
     }
 
@@ -54,6 +60,7 @@ public boolean wasStarted() {
 
     @Override
     public void stop() {
+        events.add("Stopped");
         wasStopped = true;
     }
 

cucumber-picocontainer/src/test/java/io/cucumber/picocontainer/StepDefinitions.java

@@ -1,8 +1,6 @@
 package io.cucumber.picocontainer;
 
-import io.cucumber.java.After;
-import io.cucumber.java.Before;
-import io.cucumber.java.Scenario;
+import io.cucumber.java.*;
 import io.cucumber.java.en.Given;
 import io.cucumber.java.en.Then;
 import io.cucumber.java.en.When;
@@ -11,11 +9,12 @@
 import java.util.Collections;
 import java.util.List;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.*;
 
 public class StepDefinitions {
 
     private final DisposableCucumberBelly belly;
+    private static int scenarioCount = 0;
 
     public StepDefinitions(DisposableCucumberBelly belly) {
         this.belly = belly;
@@ -35,9 +34,37 @@ public void gh20() {
 
     @After
     public void after() {
+        scenarioCount++;
         // We might need to clean up the belly here, if it represented an
         // external resource.
-        assert !belly.isDisposed();
+
+        // Call order should be Started > After > Stopped > Disposed, so here we
+        // expect only Started
+        assertTrue(belly.wasStarted());
+        assertFalse(belly.wasStopped());
+        assertFalse(belly.isDisposed());
+    }
+
+    @BeforeAll
+    @SuppressWarnings("unused")
+    public static void beforeAll() {
+        // reset static variables
+        DisposableCucumberBelly.events.clear();
+        scenarioCount = 0;
+    }
+
+    @AfterAll
+    @SuppressWarnings("unused")
+    public static void afterAll() {
+        List<String> events = DisposableCucumberBelly.events;
+        // Call order should be Start > Stopped > Disposed, for each test
+        // scenario
+        assertEquals(3 * scenarioCount, events.size());
+        for (int i = 0; i < scenarioCount; i += 3) {
+            assertEquals("Started", events.get(i));
+            assertEquals("Stopped", events.get(i + 1));
+            assertEquals("Disposed", events.get(i + 2));
+        }
     }
 
     @Given("I have {int} {word} in my belly")