Merge pull request #190 from mpdude/reset-driver-kernel-and-browser

Fix `fob_symfony.driver_kernel` state leaking between requests and scenarios

DOCUMENTATION.md

@@ -372,6 +372,8 @@ In your contexts, you can inject the `behat.driver.service_container` service (o
 * Both kernels and containers will be shut down and rebooted after every single scenario and/or example (for scenario outlines), in order to provide a clean separation between scenarios.
 * When making multiple Mink requests within a single scenario, the second kernel and container (`behat.driver.service_container`) needs to be reset to provide a clean state for the second and every additional request. This reset will happen immediately before the second and any subsequent request is handed to the kernel. So, while in general it is possible to inspect the driver's container state _after_ requests, setting it up (bringing it into desired state) easily is only possible for the _first_ request.
 
+In order to get the right (current) instances of services after such a reset has happened, make sure you call `ContainerInterface::get()` and related methods again after the request. Do not fetch services from the driver's container e. g. in your context constructors, since that will not give you the latest instances of those services.
+
 # Configuration reference
 
 By default, if no confguration is passed, _SymfonyExtension_ will try its best to guess it.

features/mink_integration/resetting_service_container_state.feature

@@ -0,0 +1,208 @@
+Feature: Resetting the driver's service container in the right places
+
+    Background:
+        Given a working Symfony application with SymfonyExtension configured
+        And a Behat configuration containing:
+        """
+        default:
+            extensions:
+                Behat\MinkExtension:
+                    base_url: "http://localhost:8080/"
+                    default_session: symfony
+                    sessions:
+                        symfony:
+                            symfony: ~
+            suites:
+                default:
+                    contexts:
+                        - App\Tests\SomeContext
+        """
+        And a YAML services file containing:
+        """
+        services:
+            _defaults:
+                autowire: true
+                autoconfigure: true
+
+            App\Tests\SomeContext: ~
+        """
+        And a context file "tests/SomeContext.php" containing:
+        """
+        <?php
+
+        namespace App\Tests;
+
+        use App\Counter;
+        use Behat\Behat\Context\Context;
+        use Behat\Mink\Mink;
+        use Symfony\Component\DependencyInjection\ContainerInterface;
+
+        final class SomeContext implements Context {
+            private $mink;
+            private $driverContainer;
+
+            public function __construct(Mink $mink, ContainerInterface $driverContainer)
+            {
+                $this->mink = $mink;
+                $this->driverContainer = $driverContainer;
+            }
+
+            /** @Given the counter service is zeroed */
+            public function counterServiceIsZeroed(): void
+            {
+                assert(0 === $this->getCounterService()->get());
+            }
+
+            /** @When I visit the page :page */
+            public function visitPage(string $page): void
+            {
+                $this->mink->getSession()->visit($page);
+            }
+
+            /** @When I increment the counter */
+            public function incrementCounter(): void
+            {
+                $this->getCounterService()->increase();
+            }
+
+            /** @Then the counter service should return :number */
+            public function counterServiceShouldReturn(int $number): void
+            {
+                assert($number === $this->getCounterService()->get());
+            }
+
+            /** @Then I should see :content on the page */
+            public function shouldSeeContentOnPage(string $content): void
+            {
+                assert(false !== strpos($this->mink->getSession()->getPage()->getContent(), $content));
+            }
+
+            private function getCounterService(): Counter
+            {
+                return $this->driverContainer->get('App\Counter');
+            }
+        }
+        """
+
+    Scenario: Driver's service container is reset between scenarios
+        # Regression testing https://github.com/FriendsOfBehat/SymfonyExtension/issues/149
+        Given a feature file containing:
+        """
+        Feature:
+            Scenario: First pass
+                Given the counter service is zeroed
+                When I increment the counter
+                Then the counter service should return 1
+            Scenario: Second pass
+                Given the counter service is zeroed
+        """
+        When I run Behat
+        Then it should pass
+
+    Scenario: Driver's service container is reset between requests
+        Given a feature file containing:
+        """
+        Feature:
+            Scenario:
+                Given the counter service is zeroed
+                When I visit the page "/hello-world"
+                Then I should see "The counter value is 1" on the page
+                When I visit the page "/hello-world"
+                Then I should see "The counter value is 1" on the page
+        """
+        When I run Behat
+        Then it should pass
+
+    Scenario: Driver's service container can be prepared before a request is made
+        Given a feature file containing:
+        """
+        Feature:
+            Scenario:
+                Given the counter service is zeroed
+                And I increment the counter
+                When I visit the page "/hello-world"
+                Then I should see "The counter value is 2" on the page
+        """
+        When I run Behat
+        Then it should pass
+
+    Scenario: Driver's service container is not reset before a request is made, even when another scenario made a request before
+        # Regression testing https://github.com/FriendsOfBehat/SymfonyExtension/issues/149
+        Given a feature file containing:
+        """
+        Feature:
+            Scenario:
+                Given the counter service is zeroed
+                And I increment the counter
+                When I visit the page "/hello-world"
+                Then I should see "The counter value is 2" on the page
+            Scenario:
+                Given the counter service is zeroed
+                And I increment the counter
+                When I visit the page "/hello-world"
+                Then I should see "The counter value is 2" on the page
+        """
+        When I run Behat
+        Then it should pass
+
+    Scenario: Driver's service container can be inspected after a request has been made
+        Given a feature file containing:
+        """
+        Feature:
+            Scenario:
+                Given the counter service is zeroed
+                When I visit the page "/hello-world"
+                Then the counter service should return 1
+        """
+        When I run Behat
+        Then it should pass
+
+    Scenario: When multiple requests are made, the driver's service container is reset, but we can inspect "in between" states
+        Given a feature file containing:
+        """
+        Feature:
+            Scenario:
+                Given the counter service is zeroed
+                # Increment the counter before the first step, so that we can
+                # really observe a difference (i. e. the container being reset
+                # and the answer being "1" instead of "2") after the second request.
+                And I increment the counter
+                When I visit the page "/hello-world"
+                Then the counter service should return 2
+                # This will reset the driver's container:
+                When I visit the page "/hello-world"
+                Then the counter service should return 1
+                # Remark: Our context had the driver's container constructor-injected
+                # and thus that container instance cannot/did not change (!). However,
+                # due to the way things work internally in Symfony (related to lazy
+                # loading? using the test container?), after the Kernel reboot we
+                # still get the _current_ service instances from that container, at least
+                # as long as we get() the services again.
+        """
+        When I run Behat
+        Then it should pass
+
+    Scenario: Driver's service container can not reasonably be modified between requests
+        # This is not really a feature, but rather documenting current behavior (judge yourself).
+        # One way around it might be to (how?) disable the reboot feature on the KernelBrowser and
+        # take responsibility for resetting the driver's container yourself.
+        Given a feature file containing:
+        """
+        Feature:
+            Scenario:
+                Given the counter service is zeroed
+                And I increment the counter
+                When I visit the page "/hello-world"
+                Then I should see "The counter value is 2" on the page
+                And the counter service should return 2
+                # Now a second request is made, which will reset the container, but leaves us
+                # with no easy way of pre-setting the container once again:
+                When I increment the counter
+                # ... you might expect "3"
+                And I visit the page "/hello-world"
+                # ... now you might expect "4". But, in fact, the reset happened just before the request.
+                Then I should see "The counter value is 1" on the page
+                And the counter service should return 1
+        """
+        When I run Behat
+        Then it should pass

src/Driver/SymfonyDriver.php

@@ -6,35 +6,72 @@ namespace FriendsOfBehat\SymfonyExtension\Driver;
 
 use Behat\Mink\Driver\BrowserKitDriver;
 use Symfony\Component\BrowserKit\AbstractBrowser;
-use Symfony\Component\BrowserKit\Client;
 use Symfony\Component\HttpKernel\KernelInterface;
 
 final class SymfonyDriver extends BrowserKitDriver
 {
+    /** @var KernelInterface */
+    private $kernel;
+
+    /** @var string|null */
+    private $baseUrl;
+
     public function __construct(KernelInterface $kernel, ?string $baseUrl)
     {
-        if (!$kernel->getContainer()->has('test.client')) {
+        $this->kernel = $kernel;
+        $this->baseUrl = $baseUrl;
+
+        if (!$this->kernel->getContainer()->has('test.client')) {
             throw new \RuntimeException(sprintf(
                 'Kernel "%s" used by Behat with "%s" environment and debug %s does not have "test.client" service. ' . "\n" .
                 'Please make sure the kernel is using "test" environment or have "framework.test" configuration option enabled.',
-                get_class($kernel),
+                get_class($this->kernel),
-                $kernel->getEnvironment(),
+                $this->kernel->getEnvironment(),
-                $kernel->isDebug() ? 'enabled' : 'disabled',
+                $this->kernel->isDebug() ? 'enabled' : 'disabled',
             ));
         }
 
-        /** @var object $testClient */
+        parent::__construct($this->createBrowser(), $this->baseUrl);
-        $testClient = $kernel->getContainer()->get('test.client');
+    }
 
-        if (!$testClient instanceof Client && !$testClient instanceof AbstractBrowser) {
+    public function reset()
+    {
+        parent::reset();
+
+        /*
+         * When \Behat\Mink\Driver\DriverInterface::visit() is called on this driver here,
+         * we ultimately end up in \Symfony\Bundle\FrameworkBundle\KernelBrowser::doRequest().
+         * That method tracks state across multiple requests to detect whether it is necessary
+         * to reboot the targeted-at kernel before performing the next request.
+         *
+         * We do not want this state to leak between Behat scenarios, and so this method here
+         * seems to be a good place to reset driver state as well.
+         *
+         * Since there is no other way to reset the KernelBrowser, we create a new instance.
+         *
+         * This also makes sense for another reason: The $kernel instance is rebooted by the
+         * KernelOrchestrator between Behat scenarios. So, every time we reset the driver
+         * (which happens at least for the first request during a scenario) we want to make
+         * sure we are using a KernelBrowser instance created in the currently active
+         * kernel "state" ("epoch"? "generation"?)
+         */
+
+        parent::__construct($this->createBrowser(), $this->baseUrl);
+    }
+
+    private function createBrowser(): AbstractBrowser
+    {
+        /** @var object $testClient */
+        $testClient = $this->kernel->getContainer()->get('test.client');
+
+        if (!$testClient instanceof AbstractBrowser) {
             throw new \RuntimeException(sprintf(
-                'Service "test.client" should be an instance of "%s" or "%s", "%s" given.',
+                'Service "test.client" should be an instance of "%s", "%s" given.',
-                Client::class,
                 AbstractBrowser::class,
                 get_class($testClient),
             ));
         }
 
-        parent::__construct($testClient, $baseUrl);
+        return $testClient;
     }
 }

src/Listener/KernelOrchestrator.php

@@ -15,12 +15,16 @@ final class KernelOrchestrator implements EventSubscriberInterface
     /** @var KernelInterface */
     private $symfonyKernel;
 
+    /** @var KernelInterface */
+    private $driverKernel;
+
     /** @var ContainerInterface */
     private $behatContainer;
 
-    public function __construct(KernelInterface $symfonyKernel, ContainerInterface $behatContainer)
+    public function __construct(KernelInterface $symfonyKernel, KernelInterface $driverKernel, ContainerInterface $behatContainer)
     {
         $this->symfonyKernel = $symfonyKernel;
+        $this->driverKernel = $driverKernel;
         $this->behatContainer = $behatContainer;
     }
 
@@ -42,8 +46,25 @@ final class KernelOrchestrator implements EventSubscriberInterface
 
     public function tearDown(): void
     {
+        $this->driverKernel->shutdown();
+
+        /*
+         * Reset both Kernel instances after a scenario has been run: The Kernel (and thus Container)
+         * used in Behat to configure Contexts; and the Kernel used by the SymfonyDriver to which
+         * requests are dispatched (through Mink).
+         *
+         * Since the "symfony" container is needed in a few other places (where and why exactly?) and
+         * has to be in a booted/usable state most of the time, we do not shut it down here in tearDown()
+         * and boot it in setUp().
+         *
+         * Instead, the definitions in \FriendsOfBehat\SymfonyExtension\ServiceContainer\SymfonyExtension
+         * make sure both kernels are booted immediately after being created, and we also initiate the
+         * re-boot() here right away.
+         */
         $this->symfonyKernel->getContainer()->set('behat.service_container', null);
         $this->symfonyKernel->shutdown();
         $this->symfonyKernel->boot();
+
+        $this->driverKernel->boot();
     }
 }

src/ServiceContainer/SymfonyExtension.php

@@ -134,7 +134,7 @@ final class SymfonyExtension implements Extension
 
     private function loadKernelRebooter(ContainerBuilder $container): void
     {
-        $definition = new Definition(KernelOrchestrator::class, [new Reference(self::KERNEL_ID), $container]);
+        $definition = new Definition(KernelOrchestrator::class, [new Reference(self::KERNEL_ID), new Reference(self::DRIVER_KERNEL_ID), $container]);
         $definition->addTag(EventDispatcherExtension::SUBSCRIBER_TAG);
 
         $container->setDefinition('fob_symfony.kernel_orchestrator', $definition);

tests/Behat/Context/TestContext.php

@@ -167,7 +167,7 @@ final class Controller
     {
         $this->counter->increase();
     
-        return new Response('Hello world!');
+        return new Response('Hello world! The counter value is ' . $this->counter->get());
     }
 }
 CON