FriendsOfBehat / SymfonyExtension

:musical_score: Extension integrating Behat with Symfony.
MIT License
472 stars 62 forks source link

Interacting with the driverContainer doesn't work when running multiple scenario in a row #149

Closed walva closed 1 year ago

walva commented 3 years ago

Hi there !

I'm using this very new feature in order to manipulate a mocked symfony HTTP client. I need to ensure my application behave well when receiving specific response from external services (such as 500).

Where is the scenario I want to test: When I try to ban a user, if its sessions haven't been deleted from our gateway (which use redis), I refuse the transition and keep the current state => so that the backoffice user can retry until the session are terminated.

Scenario: banning a user invalidates its gateway sessions
  Given The gateway return a "200" response with "3" deleted sessions
  When I send a PUT request to "/users/<<USER_ID>>/user/ban?confirm=1" as "<<ADMIN_ID>>" with permissions "admin" with body:
      """
        { "reason": "He tries to break intentionally our system." }
      """
  Then The gateway should have deleted 3 sessions
  Then the JSON node 'status' should be equal to "banned"

Under the hood, I'm manipulating my mocked http client:

    /**
     * @Given The gateway returns a :statusCode response with :count deleted sessions
     */
    public function theGatewayReturnAResponseWithDeletedSessions($statusCode, $count)
    {
        $client = $this->driverContainer->get(GatewayClient::class);
        $callback = function ($method, $url, $options) use ($statusCode, $count) {
            if ($statusCode == 200) {
                $body = ['deletedSessions' => $count];
            } else {
                $body = ['any error ...'];
            }
            return new MockResponse(json_encode($body), ['http_code' => $statusCode]);
        };

        $httpClient = new MockHttpClient($callback, 'http://test/');
        $client->httpClient = $httpClient;
        $this->gatewayClient = $client;
    }

If I run this scenario alone vendor/bin/behat features/user/ban.feature:65, it works ! But I run all the file, it fails vendor/bin/behat features/user/ban.feature: The gateway client have a null result, maybe the service wasn't called ? (Exception) => which means the instance of GatewayClient::class I have in my behat context hasn't been used.

reproducer

walva commented 3 years ago

I think it is related to #116, #86 and #110

walva commented 3 years ago

Updated issue with a reproducer

walva commented 2 years ago

Yesterday, I communicated with @silverbackdan on another issue related to this one (https://github.com/Kocal/SymfonyMailerTesting/issues/17)

He gave me the trick to solve this issue: calling Symfony\Bundle\FrameworkBundle\KernelBrowser::disableReboot(). The solution feels a bit hacky, but it does the job.

The issue is that the driverContainer injected in contexts (Behat\Behat\Context\Context) is not the same as the container running the scenario steps after the first scenario is executed with a call to Symfony\Bundle\FrameworkBundle\KernelBrowser::doRequest($request).

All other call to Symfony\Bundle\FrameworkBundle\KernelBrowser::doRequest($request) will call the method KernelBrowser::doRequest::shutdown() due to the properties hasPerformedRequest = true and reboot = true, which cause the container to be erased from the kernel.

The "hack" is to set the reboot property to false, but this might have a considerable impact on other tests, depending on the business/vendor code relying on this behaviour.

Let make some 2 diagrams:

  1. ❌ one with all the tests failing except the first due to current behavior, reboot being at true.
  2. ✔ one with all the tests passing thanks to reboot set to false.

image

image

There are 3 approaches:

  1. Having a BehatContext that forces the "reboot" to false.

This should be configurable or well documented. I stopped to use behat in many situations because of that and instead relied on PhpUnit. Now, I understand the issue and the way to prevent it, I can live with it and continue to use behat in more advanced scenarios

  1. Set the property hasPerformedRequest = false after each reboot

This will work in simple scenario. When you have more than one Symfony\Bundle\FrameworkBundle\KernelBrowser::doRequest($request) call within the same scenario will produce the same unwanted behavior.

  1. the method Symfony\Bundle\FrameworkBundle::doRequest::shutdown()

To me, it will totally solve the issue. But I fear the impact and eventual BC breaks it could imply. This is a question I would like to ask to @symfony/symfony core contributors. Ping @dunglas @weaverryan @Nyholm @nicolas-grekas .

Another solution is to extend the KernelBrowser and adapt the behaviour of doRequest method.

What do you think about this?

Nyholm commented 2 years ago

(I have not read/understood this yet)

But maybe @kbond should also be pinged.

Btw, your handwriting is awesome!

kbond commented 2 years ago

I think this looks like you want to mock a service. My first thought is to check out https://github.com/Happyr/service-mocking which helps with this. I've only used with phpunit but think it could be used with behat.

walva commented 2 years ago

Thanks to you both @Nyholm and @kbond for the reaction.

Actually, the error rely on who the kernel work in behat. You can assume the Mock a service part is working as expected. The issue is: The same test won't pass 2 times in a row, like in this reproducer.

kbond commented 2 years ago

I'm not 100% sure about the kernel/container with behat. I just know that setting/maintaining container state in tests/between requests can be troublesome in general. happyr/service-mocking mocking is a solution for this that I've used.

That being said, it does seem like a bug in behat/this extension - you'd expect a clean slate between scenarios.

The issue is that the driverContainer injected in contexts (Behat\Behat\Context\Context) is not the same as the container running the scenario steps after the first scenario is executed with a call to Symfony\Bundle\FrameworkBundle\KernelBrowser::doRequest($request).

This seems like the crux of the problem. I don't know enough about this extension/behat to know how to fix.

walva commented 2 years ago

@kbond Thanks for your answer.

That being said, it does seem like a bug in behat/this extension - you'd expect a clean slate between scenarios.

I agree with you on that ! But, in this case, we have a clean state between scenarios and steps. And I think it's a bit too much. What do you (all) think about this? Should we clean the state between steps?

Thus, the first executed scenario won't have a clean state but the 2...n scenarios will have a clean state which is inconsistent. This reinforces my idea that there is something to change here.

Before going futher (eg: a PR), I would like to have the opinions of other developers.

mpdude commented 1 year ago

I have a suggestion in #190 to

Not quite sure, but I think both parts are needed to get a clean separation.

Note that when you do make two (or more) requests from within a single scenario, you have no good place to interact with the fob_symfony.driver_kernel after it is rebooted/reset immediately before it handles the second request (e. g. mocking/preparing service state relevant for the second request is difficult).

develth commented 1 year ago

@Yozhef is it really solved by #190? Just get it work with disableReboot on @BeforeScenario when running multiple Scenarios

mpdude commented 1 year ago

@develth could you please open a new issue providing a (hopefully simple) reproduce case?