peridot-php / peridot

Event driven BDD test framework for PHP
http://peridot-php.github.io/
MIT License
328 stars 27 forks source link

Memory limit error #176

Closed djones8520 closed 8 years ago

djones8520 commented 8 years ago

I suspect this issue may be related to issue 174, but I didn't want to hijack their issue, since it may not be related at all.

Error: PHP Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 3072 bytes).

Recently, I created new test file and was running fine locally, but when I created the pull request, the tests failed on circleci. I believe circleci's memory limit is 128 (locally is 512), because when I lower local's memory limit to 128, the tests run out of memory as well.

That was when I discovered how scopes were leaking as described in issue 174. We suspected that may be the issue and added an suite.end event that removed the properties from the scope. However, the issue still persists. I var_dumped the scope after unsetting the properties and noticed there is a ProphecyScope and it still has methodProphecies created by our tests.

I'm wondering if perhaps the ProphecyScope is just getting bigger over the course of the tests and finally runs out of memory. Is there a way to reset/clear a ProphecyScope? Perhaps another suggestion to what the issue is?

Below is the code for our peridot.php file.

return function(EventEmitterInterface $emitter) {
    $plugin = new ProphecyPlugin($emitter);
    $watcher = new WatcherPlugin($emitter);
    $watcher->track(__DIR__ . '/../../OS');

    $testingScope = new TestingScope();

    $emitter->on('suite.start', function($test)use ($testingScope){
        $test->getScope()->peridotAddChildScope($testingScope);
    });

    $emitter->on('suite.end', function($test){
        $scope = $test->getScope();

        foreach($scope as $key => $value){
            unset($scope->$key);
        }
    });
};

Thanks for any suggestions.

brianium commented 8 years ago

@djones8520 I will definitely be taking a look at this. There are certainly some optimizations that can be made to scope. I don't have a solution off the top of my head, but I will look into this shortly.

Thanks for opening the issue :)

djones8520 commented 8 years ago

So, I found the handy clearProphet function on ProphecyScope and added that function call to any ProphecyScope children on $scope. That allowed the tests to pass locally at 128 MB (was failing). However, they still fail on circleci at the same limit.

Locally, I tested a few different limits and it fails at 127 MB. That seems like a pretty tight window. It got me wondering about the memory needed to run peridot and I didn't see any memory requirements listed. Is 128 not enough memory?

brianium commented 8 years ago

Hmmmm. That is a pretty variable requirement in my opinion. I will definitely acknowledge there could be some improvements to the shared nature of scopes with regard to this problem, but the memory requirement for just about any testing tool increases the larger the test suite. Can I ask how large your test suite is?

brianium commented 8 years ago

If it is a bit on the larger side, this article here seems helpful on bumping up your memory limit for PHP in a circle-ci environment

https://www.neontsunami.com/posts/running-laravel-through-continuous-integration-(ci)-using-circleci

brianium commented 8 years ago

Something to the effect of:

// circle.yml
machine:
  php:
    version: 5.4.21
dependencies:
  pre:
    - echo "memory_limit = 1024M" > ~/.phpenv/versions/5.4.21/etc/conf.d/memory.ini
djones8520 commented 8 years ago

We have about 800 tests at the moment. I'm not sure how that stacks up to the norm, but it doesn't seem too big.

djones8520 commented 8 years ago

I don't mind bumping up the memory limit of circleci, but we didn't want to raise it just because. If 128 MB is too small for tests, so be it, but if we're using too much memory in our tests for some reason, we'd rather fix that.

brianium commented 8 years ago

I hear you :)

I suspect scopes are too blame. The loader essentially builds a giant tree out of the entire suite, and with 800 tests, you end up with a decent sized tree. Throw a bunch of large scopes on those nodes and the memory requirement goes up. There might be a way to optimize how that scope is shared/set to prevent unnecessary bloat.

You might be able to free up some memory by running your tests concurrently as well. There is a peridot plugin for that you might try - it currently does not support Windows.

I will take a look into general scope optimization, and specifically the ProphecyPlugin to see if any optimizations can be made there.

Thanks so much for opening this :)

djones8520 commented 8 years ago

Sure thing. We ended up increasing the memory limit to solve the "issue." It may have been too low in general.

In case anyone is curious about the scope problem, we got a decent solution I think. Above I posted code that we have in our peridot.php config file. Inside the suite.end event, is:

$children = $scope->peridotGetChildScopes(); foreach($children as $child){ if(get_class($child) == ProphecyScope::class) $child->clearProphet(); }

This clears the variables created and prophecy predictions/other by our tests. suite.end event is after each describe section of the test. There is a test.end event, but that didn't work for use, since it seems to run before afterEach sections and we use those sometimes for asserts that each test in the describe share.

This added event didn't cause any issues that we see, so it seems pretty solid. In fact, we discovered a few tests that were passing due to the "scope leak."