#218. Add a tearDown() method.

[Aerendir]

unset local properties and variables inside methods to freeup memory during tests execution.

Fix Issue #218.

[lsmith77]

@alexislefebvre looks fine to me .. that being said .. @Aerendir you confirmed that it reduces memory consumption?

[alexislefebvre]

I tested a few days ago with the Bundle's own tests and I didn't see any difference in the memory report from PHPUnit.

It's confirmed by Travis CI, the memory consumptions are identical, with PHP 5.6:

• the parent commit of this PR: Time: 5.73 seconds, Memory: 59.25Mb
• this PR: Time: 6.85 seconds, Memory: 59.25Mb

I have to test the PR with a bigger project.

---

Edit: Here is the result with one of my projects:

While using the 1.4.0 version:

$ phpunit -c app/phpunit.xml.dist --testsuite Groupe1
PHPUnit 5.1.7 by Sebastian Bergmann and contributors.

.................................                                 33 / 33 (100%)

Time: 12.39 seconds, Memory: 161.75Mb

OK (33 tests, 175 assertions)

Then I copy-pasted the file from this PR:

$ phpunit -c app/phpunit.xml.dist --testsuite Groupe1
PHPUnit 5.1.7 by Sebastian Bergmann and contributors.

.................................                                 33 / 33 (100%)

Time: 13.01 seconds, Memory: 215.00Mb

OK (33 tests, 175 assertions)

I ran tests 3 times in order to avoid effects of the cache, you can see the third result here.

That's totally weird, why memory consumption increase?

[tackerm]

Sorry, but I think some of the changes are bad because they will actually increase memory consumption - I commented this in the diff some days ago: [https://github.com/Aerendir/LiipFunctionalTestBundle/commit/90fcdd18e64247476b97b7da5d30503a2b35d367#commitcomment-15785200]

[Aerendir]

@tackerm , the memory consumption reported by PHPUnit is the total memory consumption. It means that maybe you don't see any change. But if you measure the consumption of each single test before and after the teardown, then you'll see the differences.

I've wrote two helper classes to measure this: PHPUnit_Helper and PHPUnit_Profiler.

Those two classes shows in more details the memory consumption of each single test.

Maybe sometimes there is an increase in memory usage of some Kilobytes (the max I registered is about 5Kb): this happens when the test is really simple and the instantiation of PHPUnit_Helper consumes more memory than how it freeup. But for large tests that uses a lot of memory like the ones conducted using LiipFunctionalTests, the saving is relevant.

Can you try to measure again using these helper classes? I'll do the same as I based my editing on past experience and hadn't yet the time to measure them with the project n which I'm using LiipFunctionalTest, believing, basing my opinion on past experiences, that anyway a tearDown() method can help.

[tackerm]

@Aerendir I'm not complaining about the tearDown() method, I'm very happy with that 👍
But replacing local function variables with class properties (things like $localClient, $localKernel etc) is a bad idea, because the memory will remain occupied longer. And since all these objects are re-created in every method anyways, there's no benefit of storing them in a class property 😉

[Aerendir]

@tackerm Yes, you're right, but putting them in local properties makes us able to do more things with the class adding some getter and setter methods that can only work if those objects are properties.

Take this example from one of my unit tests:

public function testEditCompany()
    {
        $this->addHelpResource(
            'fixtures',
            $this->loadFixtures([
                'AppBundle\DataFixtures\ORM\LoadMerchantUserData',
                'AppBundle\DataFixtures\ORM\LoadMerchantProfileData',
                'AppBundle\DataFixtures\ORM\Company1\LoadCompany1Data'
            ])->getReferenceRepository()
        );

        $this->loginAs($this->getHelpResource('fixtures')->getReference('MerchantUser'), 'main');
        // @todo Refact WebTestCase to set client and crawler when using fetchContent and fetchCrawler
        $this->addHelpResource('client', static::makeClient());

        // Go to Company edit page
        $this->addHelpResource('crawler', $this->getHelpResource('client')->request('GET', 'company/1/edit'));

        /*
         * Test response
         */
        $this->assertEquals(200, $this->getHelpResource('client')->getResponse()->getStatusCode());

        // Select the Form and submit new data
        $this->addExpectedValue('companyName', 'Crespi Gioielli', true);
        $this->addHelpResource('formEdit', $this->getHelpResource('crawler')->selectButton('Company[save]')->form([
            'Company[legalName]' => $this->getExpectedValue('companyName'),
        ]));

        // Submit the form
        $this->addHelpResource('crawlerForm', $this->getHelpResource('client')->submit($this->getHelpResource('formEdit')), true);

        // Test the editing was successful
        $this->addHelpResource('formEdited', $this->getHelpResource('crawlerForm')->selectButton('Company[save]')->form());

        $this->assertEquals($this->getExpectedValue('companyName'), $this->getHelpResource('formEdited')['Company[legalName]']->getValue());
    }

I cannot use the shortand form fetchCrawler because after I modify the crawler object I cannot resubmit it because I cannot get again the Client.

Make those objects as properties of the class, open a new world of possibilities.

For example, with some more refactorings to WebTestCase, this test may be rewritten in this way:

// THE CODE MAYBE IMPRECISE. IT SERVES ONLY AS A CONCEPT.
public function testEditCompany()
    {
        $this->addHelpResource(
            'fixtures',
            $this->loadFixtures([
                'AppBundle\DataFixtures\ORM\LoadMerchantUserData',
                'AppBundle\DataFixtures\ORM\LoadMerchantProfileData',
                'AppBundle\DataFixtures\ORM\Company1\LoadCompany1Data'
            ])->getReferenceRepository()
        );

        $this->loginAs($this->getHelpResource('fixtures')->getReference('MerchantUser'), 'main');
        // @todo Refact WebTestCase to set client and crawler when using fetchContent and fetchCrawler
        $this->addHelpResource('crawler', $this->fetchCrawler());
        // Select the Form and submit new data
        $this->addExpectedValue('companyName', 'My Company Name', true);
        $this->addHelpResource('formEdit', $this->getHelpResource('crawler')->selectButton('Company[save]')->form([
            'Company[legalName]' => $this->getExpectedValue('companyName'),
        ]));

        // Submit the form
        $result = $this->submitForm($this->getHelpResource('formEdit'));

        // ... other tests based on $results...
    }

This PR is only a first step ;)

Anyway, if I'm missing something, any suggestion is welcomed...

[tackerm]

@Aerendir I am not quite sure what the benefit of that would be?

Regarding your example, I would simplify the test to something like the following - no need for a single getter/setter/class property.

public function testEditCompany()
{
    // You want to use a fresh client!!
    $client = static::createClient();

    $repo = $this->loadFixtures([...]);
    $this->loginAs($repo->getReference('MerchantUser'), 'main');

    $client->request('GET', 'company/1/edit')
    $this->assertEquals(200, $client->getResponse()->getStatusCode());

    // Select the Form and submit new data
    $expected = 'Crespi Gioielli';
    $form = $client->getCrawler()->selectButton('Company[save]')->form('Company[legalName]' => $expected);

    $client->submit($form);

    $editedForm = $client->getCrawler()->selectButton('Company[save]');

    $this->assertEquals($expected, $editedForm['Company[legalName]']->getValue());
}

BTW, I am not sure if the assertion really does what you expect it to do...

[alexislefebvre]

@Aerendir on this PR you tried to use less memory during tests then in a comment you suggest to make the private properties accessible in your tests. Isn't it contradictory? Can tests use less memory and in the same time give access to properties which were previously private?