TestCase is instanciated per test (correct), but not destroyed immediatelly (wrong, non GCable)

[mvorisek]

Q	A
PHPUnit version	9.5.5
PHP version	7.4
Installation Method	Composer

Summary

TestCase is instanciated per test (correct), but not destroyed immediatelly (wrong, non GCable)

Current behavior

not destroyed immediatelly

How to reproduce

see atk4/data@849a548 and related output https://github.com/atk4/data/runs/2782214124#step:8:31 (can be reproduced by running phpunit locally)

In the output, you can see that x representing TestCase::__destruct call is called after all tests are done.

Expected behavior

TestCase must be destroyed immediatelly after a test finishes. It is important to allow GC to free the TestCase instance immediatelly. This should be also tested in unit tests so same issue does not happen again.

[sebastianbergmann]

PHPUnit has worked like this since the beginning, which is now for over 20 years. While this is certainly not desired behaviour, it is the expected behaviour, at least for the time being. I would therefore not call it a bug. Unfortunately, we are not yet in a situation where this can easily be changed. My hope is that once the dust has settled from the major undertakings of implementing support for PHP 8 attributes, introducing an event system, and cleaning up PHPUnit's configuration handling that this can be approached.

[talkinnl]

@sebastianbergmann Since attributes and the event system have launched, could you maybe consider to evaluate this for v12?

I think this is one of the more common pitfalls, and multiple 3rd parties now hint to always clear properties with some default tearDown reflection magic. Random example: https://stackoverflow.com/questions/36032168/symfony-and-phpunit-memory-leak

In one of our bigger projects (200k test lines) we use explicit tearDown implementations which are slightly faster than the generic reflection trick, but since it tends to get forgotten and takes effort we’ll convert to the reflection trick too.

Alternatively: I suspect current postponed destruction to have quite an impact on used resources and cloud pipeline bills worldwide.

[rr-it]

This is one working approach to reduce memory consumption:

• In tearDown remove the properties - either in tests itselves or use the general \ReflectionClass-approach.
• Then run garbage collector either in every Xth tearDown via <phpinfo controlGarbageCollector="true"/> or better once after each test-case when all its tests have finished on tearDownAfterClass .

My example implementation from https://github.com/TYPO3/testing-framework/pull/562/files

BaseTestCase with GC in tearDownAfterClass and memory consumption info per test (click to open)

abstract class BaseTestCase extends TestCase
{
    public static function setUpBeforeClass(): void
    {
        if(getenv('MEMORY_INFO')) {
            echo PHP_EOL . '# \\' . static::class . PHP_EOL;

            // Force garbage collection to measure memory used by these test-cases only.
            gc_collect_cycles();
            $GLOBALS['memory_usage_start'] = memory_get_usage();
        }

        parent::setUpBeforeClass();
    }

    protected function tearDown(): void
    {
        parent::tearDown();

        /*
         * Unset properties from test-object
         *
         * The properties  - introduced in the test-class' setUp method - persist memory for each test-case run until all
         * test-runs have finished. So memory adds up with every test-case.
         *
         * All these properties should actually be unset in the test-class' tearDown method.
         * Unfortunately tearDown is not implemented in every test-class or tearDown does not accurately unset all
         * properties.
         *
         * Important: Gargbage collection must be triggered to really free up memory. As garbage collection consumes
         * time this is done only once per test-set in tearDownAfterClass method.
         *
         * Note: Private properties cannot be cleaned up here.
         */
        $reflection = new \ReflectionClass(static::class);
        foreach ($reflection->getProperties(
            \ReflectionProperty::IS_PUBLIC | \ReflectionProperty::IS_PROTECTED
        ) as $property) {
            if (!$property->isStatic() && $property->class === static::class && isset($this->{$property->name})) {
                unset($this->{$property->name});
                if(getenv('MEMORY_INFO')) {
                    echo '!';
                }
            }
        }
        unset($reflection);
    }

    public static function tearDownAfterClass():void
    {
        parent::tearDownAfterClass();

        /*
         * In tearDown method all properties of the test-object are unset to free up memory.
         * This memory is only reclaimed if neccessary, e.g. the memory of the running PHP process is running into its
         * limitation and thereby an automatic garbage collection is triggered.
         *
         * The limit for memory is really high, therefor garbage collection is never triggered and the memory adds up.
         *
         * To use as little memory as possible we force a garbage collection here to reclaim free memory after a
         * test-set is completed.
         */
        gc_collect_cycles();

        if(getenv('MEMORY_INFO')) {
            $memStart = $GLOBALS['memory_usage_start'];
            $memCurrent = memory_get_usage();
            echo PHP_EOL . 'Memory: '
                . sprintf("%0+1.2f", ($memCurrent - $memStart) / 1024 / 1024) . ' MB / '
                . sprintf("%01.2f", $memCurrent / 1024 / 1024) . ' MB' . PHP_EOL;
            unset($GLOBALS['memory_usage_start']);
        }
    }
}

[talkinnl]

@rr-it : One of many other example cleanup scripts: https://github.com/talkinnl/dont-leak , then you only need to add a single line to your BaseTestCase. It'll also cleanup privates, and privates of parents.

I think user-space remedies are covered with our suggestions, but I hope for this issue to be solved by design. :)

[mvorisek]

@sebastianbergmann this issue is not fixed as claimed in #5861 (comment)

[talkinnl]

@mvorisek Based on your PR (#5867 ), I didn't get yet that it really didn't work for you. And the change actually has proven to pay off for huge testsuites (my own, see my comment in the change PR, testsuite of 2 hours), Typo3 and others.

HOWEVER, (ping @sebastianbergmann ), I did just run your PR with proposed test and you're right about this specific scenario: TestCases with dataproviders aren't yet destructed immediately [edited to add: ], but after the various dataprover instances are done, which is still way earlier than before.

[sebastianbergmann]

Please stop commenting on a closed ticket. If what @talkinnl writes (test cases that use data providers are not destructed immediately) then please open a new ticket with a minimal, self-contained, reproducing test case that shows this.

[talkinnl]

Yes, you're right. One last time on this closed ticket, as a triaging service :)
It's only for this contrived example; The iterator is only destructed after all dataprovider values are done.

So it's not at the end as before, but after the last provided data instance. Which is already quite a bit better. I will NOT log a new issue; it hurts way less than before, and if this is really blocking you can still use a tearDown().

---

Curiosity got a hold of me: The issue is with TestSuiteIterator::$tests. By modifying the iterator to use array_shift instead of position logic, I could fix this. Needs more work though..

[mvorisek]

If you know how to fix this please propose a PR.

[mvorisek]

Fixed by #5861 and #5875, thank you all!