liip / LiipFunctionalTestBundle

Some helper classes for writing functional tests in Symfony
http://liip.ch
MIT License
639 stars 182 forks source link

Double container instances is causing an overhead #586

Open gmponos opened 6 years ago

gmponos commented 6 years ago

Hello there,

According to symfony official documentation in order to test a controller you have to initialize a client like this:

        $client = static::createClient();

And this is what I've been doing.

after creating my client I used to do something like this:

        $this->loadFixtures($this->getFixtureData());

Then today I reached my memory limit and dived little deeper into this bundle. I realized that the reason was that there is a container loaded in $client = static::createClient(); and also loadFixtures is loading a second container.

After making some copy-paste custom code I changed the signature of loadFixtures to accept a container and the memory for PHP 5 dropped from ~500 MB to ~200 MB.

In my belief it would've been better if you let the developer set his own container somehow.

You could say that I could use the makeClient instead but unfortunately make client is doing the same thing and it loading a second container too by calling internally this

$client = static::createClient(array('environment' => $this->environment), $params)

Is there any workaround about this?

Jean85 commented 6 years ago

The fact that the client is loading a second container is absolutely correct. You don't want to use the same one, since the first one is spun up by the test and it's already booted, and it can be tainted by any modification or operation that you're doing to prepare your test case.

gmponos commented 6 years ago

The work around that I found was this:

    public function setUp()
    {
        parent::setUp();
        $this->client = static::createClient();
        $this->containers['|test'] = $this->client->getContainer();
        $this->loadFixtures($this->getFixtureData());
    }

    public function tearDown()
    {
        unset($this->containers);
        unset($this->client);
        parent::tearDown();
    }

I feel it's kinda dirty and developers can easily forget to do this. Unfortunately it seems that I will need to either fork the project or go with a custom solution because it's way costly.

@Jean85 do you suggest that the client should not load a container or that LiipBundle should not load a second one?

Jean85 commented 6 years ago

Nope, I suggest neither. You shouldn't mix those two different container, they are intended to stay completely separate! You're trying to optimize something that you shouldn't. And believe me, I'm speaking from experience, I've tried to do the same in the past, obtaining only issues and unreliable tests.

The system under test need it's own container, and it's inside the client. You (may) need a container inside the tests, to manipulate the state of your app before the test, or to make assertions after, but you have to use a separate one.

gnat42 commented 2 years ago

@Jean85 is it possible to get the container inside the app? The reason being is we've successfully setup an application so that we've created a decorated service in the test environment, but we want to configure its responses based a scenario (this service is an API client so we don't want actual requests going to the network). Is it possible to get access to that mock service from the app to say, when you see this request, respond with this data?

Jean85 commented 2 years ago

With Symfony 5.3 and further, the container of the client is the same that you get from the new getContainer native method, so if you rely on that (and not the one from this package) you can do it.

Otherwise, you could always call getContainer on the client itself... But if the service gets instantiated again due to a kernel reboot you would be out of luck.

That's why I circumvent these kind of stuff with static calls generally...

gnat42 commented 2 years ago

Thanks @Jean85 yeah we couldn't figure out how to get the container from the client reliably. I went the same way and just put some public static methods on the mock service so that its 'configurable' and that did the trick as well.