Heuristics to set memory limit for mutants running with PHPUnit #258

sanmai · 2018-03-24T14:10:30Z

Since we know how much memory the initial test suite used, and only if we know, we can enforce a memory limit upon all mutation processes. Limit is set to be twice the known amount, because if we know that a normal test suite used X megabytes, if a mutant uses a lot more, this is a definite error.

Memory limit is introduced by altering a known temporary php.ini to include a directive to enable the limit as the very last line. We only apply a memory limit if there isn't one set.

This PR assumes no default memory limits.

This PR:

Adds heuristics to set memory limit for mutants running with PHPUnit ...
Covered by tests
Fixes Mutant processes are not memory-bound #247 as far I concerned

I rather hope this PR will be more satisfactory to @theofidry and others who didn't like an idea of a default memory limit. We are not forcing a default upon anyone here.

maks-rafalko

This looks very nice to me.

if we know that a normal test suite used X megabytes, if a mutants uses a lot more, this is a definite error.

👍

But, should we show in the CLI reporter that the mutant is killed by exceeding the memory consumption?

I mean add a new letter to .: killed, M: escaped, S: uncovered, E: fatal error, T: timed out, for example, X: memory limit.

Am I right that this PR is a replacement for #253?

maks-rafalko · 2018-03-24T15:44:53Z

src/Command/InfectionCommand.php

@@ -192,6 +194,11 @@ protected function execute(InputInterface $input, OutputInterface $output)
            return 1;
        }

+        // We only apply a memory limit if there isn't one set
+        if ($adapter instanceof PhpUnitAdapter && ini_get('memory_limit') === '-1') {


What about other test frameworks, e.g. PhpSpec? shouldn't we care about it as well?

I'm sure if we even end up with PhpUnitAdapter for now, we should introduce an interface like MemoryUsageAware (or whatever you name it) and check it rather than hardcoding PHPUnit related stuff in the very base command

if ($adapter instanceof MemoryUsageAware && ini_get('memory_limit') === '-1') { $this->applyMemoryLimitFromTestFrameworkProcess(); } class PhpUnitAdapter implement MemoryUsageAware...

✅ MemoryUsageAware

sanmai · 2018-03-24T16:02:52Z

If we could ignore other frameworks, then, yes, this is a bare-bones replacement for #253.

PhpSpec isn’t reporting memory usage like PHPUnit does (I checked), so if we need to support this in PhpSpec we have no choice but to go by #253.

sanmai · 2018-03-24T16:09:51Z

I believe PHP interpreter will not indicate by an exit code if it died because of a language error or not. Therefore, we could only reason about exit statuses if we capture an actual error. Right now PHPUnit will not print any errors, but if it could be made to, it's a possibility.

sanmai · 2018-03-24T16:16:01Z

I’m not sure if we can hack PhpSpec runner to output a number from memory_get_peak_usage(), and quick googling did not show anything.

Did I address all you concerns, @borNfreee?

theofidry · 2018-03-24T16:32:45Z

👍 I must say I much prefer this one to the others.

I mean add a new letter to .: killed, M: escaped, S: uncovered, E: fatal error, T: timed out, for example, X: memory limit.

👍 👍

IIRC you get an error along the lines of Fatal error: Allowed memory size of or a special code for this kind of error so we could know if the failure was caused by a memory exhaustion that way.

I think we should also make it configurable like we do for the time out; This can wait though.

If we could ignore other frameworks

We could try to add a memory consumption on their end but I don't think it's a blocker for this PR.

sidz

Is this approach really working as you expect? It will set memory_limit only for the main phpunit process :)
But PHPUnit runs sub-processes for each test case. And this approach won't work for them at all.

As Marco said yesterday we need to do it in the same way like phpunit does. So I investigated a bit yesterday and here is how phpunit works:

https://github.com/sebastianbergmann/phpunit/blob/7.0.2/src/Runner/PhptTestCase.php#L125 - here it takes ini section from the xml and puts all ini's to the main $settings variable

then getCommand https://github.com/sebastianbergmann/phpunit/blob/7.0.2/src/Util/PHP/DefaultPhpProcess.php#L95 method will generate /usr/bin/php -d bla_bla=-1 file for each test case.

So this PR will tweak only the main phpunit process.

In this case we need to add ini section to the phpunit.xml file which Infection generates if we want to apply memory limit to each sub-process.

Also as we will replace our own XdebugHandler by https://github.com/composer/xdebug-handler please don't use any classes, constants from the Php namespace

sanmai · 2018-03-25T02:00:05Z

@sidz, PHPUnit will pass trough all options we set in our custom php.ini even if we tell it to use process isolation:

bin/infection --test-framework-options="--process-isolation"

If someone decides to shoot oneself in the foot by setting own limits with <ini name="memory_limit" value="-1"/>, they gonna be hit by it. That's par for the course.

sanmai · 2018-03-25T02:07:23Z

@sidz we use ConfigBuilder::ENV_TEMP_PHP_CONFIG_PATH in PhpExecutableFinder also, and I don't see any replacement to it in the composer/xdebug-handler. It doesn't even export a similar path it has. And PhpExecutableFinder needs to know which php.ini to use. I don't see how you gonna do this.

sidz · 2018-03-25T07:07:01Z

@sanmai #215 (comment)

sanmai · 2018-03-25T13:41:44Z

@sidz That's excellent! We would only have to change this line and this line to use php_ini_loaded_file() instead. It would be best if there will be an utility function. In any case this is trivial task and not a blocker to this PR.

sidz · 2018-03-25T13:44:49Z

@sanmai you can change this line 33ee1df#diff-a99db3f318d0547a301f216cb03b51a7R287 without any changes right now :) So please do it. thx

sanmai · 2018-03-25T13:52:18Z

@sidz Done!

I'm happy to say that my E2E test for this issue is passing with php_ini_loaded_file(). This also means it is good to go in other places.

Anything else?

maks-rafalko

LGTM 👏 🥇 🎉 Just added a minor comment

@sidz do you have any concerns yet?

maks-rafalko · 2018-03-27T19:16:34Z

tests/Fixtures/e2e/Memory_Limit/README.md

@@ -0,0 +1,9 @@
+# Title


please either remove this file or add correct information

sidz

I'm not sure. What if I want or I need to set -1 for all sub-processes?

sidz · 2018-03-27T19:26:43Z

src/Command/InfectionCommand.php

+     *
+     * @var int
+     */
+    const DEFAULT_MEMORY_LIMIT_MULTIPLIER = 2;


imo all constants should be placed at the top of the class

This isn't going to make the code more readable.

PHP CS Fixer normally takes care of this if there's such policy, and it didn't.

When there's no policy, I side with Code Complete on minimizing live time for programming constructs.

if you find any place in the infection code where we did it in the same way - feel free to leave it as is. if no - please move it to the top of the class. thank you

However you wish. Done.

sidz · 2018-03-27T19:31:10Z

src/Command/InfectionCommand.php

+         * because if we know that a normal test suite used X megabytes,
+         * if a mutants uses a lot more, this is a definite error.
+         */
+        $memoryLimit *= self::DEFAULT_MEMORY_LIMIT_MULTIPLIER;


could you please move this line under condition as we don't use it elsewhere?

maks-rafalko · 2018-03-27T19:39:49Z

What if I want or I need to set -1 for all sub-processes?

what is the use case when you want PHPUnit to consume unlimited (and much more than initial test run consumed) memory?

sidz · 2018-03-27T19:43:37Z

can't give you any constructive example now but I have a stronger feeling that we're doing smth wrong.

maks-rafalko · 2018-03-27T19:50:03Z

TBH I don't get it. This PR limits PHPUnit processes only, with the twice bigger amount of memory than initial test suite consumes.

I'm just summing up it to myself as well to find any potential issues but nothing comes to my mind.

Anyway, we still have plenty of time before 0.9.0, so we can see how it's going

Since we know how much memory the initial test suite used, and only if we know, we can enforce a memory limit upon all mutation processes. Limit is set to be twice the known amount, because if we know that a normal test suite used X megabytes, if a mutants uses a lot more, this is a definite error. Memory limit is introduced by altering a known temporary php.ini to include a directive to enable the limit as the very last line.

…oesn't need to know it use a concrete PhpUnitAdapter.

Test will fail with a timeout if the automatic memory limit is disabled.

maks-rafalko · 2018-03-28T08:16:33Z

Thanks for all your hard work on this topic @sanmai, much appreciated

sanmai force-pushed the memory-limit-auto branch from 823f12d to c45182e Compare March 24, 2018 14:19

maks-rafalko reviewed Mar 24, 2018

View reviewed changes

sidz reviewed Mar 24, 2018

View reviewed changes

sanmai force-pushed the memory-limit-auto branch 3 times, most recently from 5b95b33 to 81b86ea Compare March 25, 2018 02:46

sanmai force-pushed the memory-limit-auto branch from fcdcc60 to 117af15 Compare March 25, 2018 13:43

sanmai force-pushed the memory-limit-auto branch from 84d04d7 to 5d9bd3f Compare March 25, 2018 13:55

This was referenced Mar 25, 2018

Syntax errors are not accounted for, not counted #262

Closed

Statement mutators: Assign, MethodCall #243

Closed

maks-rafalko approved these changes Mar 27, 2018

View reviewed changes

sidz reviewed Mar 27, 2018

View reviewed changes

PhpUnitAdapter can now get amount memory used by PHPUnit

57b4733

sanmai force-pushed the memory-limit-auto branch 3 times, most recently from 0de80b3 to 5eb00d0 Compare March 28, 2018 01:30

sanmai and others added 2 commits March 28, 2018 16:29

Adds interface TestFramework\MemoryUsageAware: InfectionCommand now d…

2a6f725

…oesn't need to know it use a concrete PhpUnitAdapter.

E2E test for memory limit heuristics.

4bf257c

Test will fail with a timeout if the automatic memory limit is disabled.

sanmai force-pushed the memory-limit-auto branch from 5eb00d0 to 4bf257c Compare March 28, 2018 07:31

sidz approved these changes Mar 28, 2018

View reviewed changes

maks-rafalko merged commit b7eb404 into infection:master Mar 28, 2018

maks-rafalko mentioned this pull request Mar 28, 2018

Add a mandatory memory limit for mutation sub-processes #253

Closed

7 tasks

sanmai deleted the memory-limit-auto branch March 28, 2018 09:08

sanmai mentioned this pull request Mar 28, 2018

Updated the PHP CS Fixer config #270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Heuristics to set memory limit for mutants running with PHPUnit #258

Heuristics to set memory limit for mutants running with PHPUnit #258

sanmai commented Mar 24, 2018 •

edited

maks-rafalko left a comment •

edited

maks-rafalko Mar 24, 2018

sanmai Mar 25, 2018 •

edited

sanmai commented Mar 24, 2018 •

edited

sanmai commented Mar 24, 2018 •

edited

sanmai commented Mar 24, 2018

theofidry commented Mar 24, 2018

sidz left a comment •

edited

sanmai commented Mar 25, 2018

sanmai commented Mar 25, 2018

sidz commented Mar 25, 2018

sanmai commented Mar 25, 2018

sidz commented Mar 25, 2018

sanmai commented Mar 25, 2018 •

edited

maks-rafalko left a comment •

edited

maks-rafalko Mar 27, 2018

sidz left a comment

sidz Mar 27, 2018

sanmai Mar 28, 2018

sidz Mar 28, 2018

sanmai Mar 28, 2018

sidz Mar 27, 2018

sanmai Mar 28, 2018

maks-rafalko commented Mar 27, 2018

sidz commented Mar 27, 2018

maks-rafalko commented Mar 27, 2018 •

edited

maks-rafalko commented Mar 28, 2018

Heuristics to set memory limit for mutants running with PHPUnit #258

Heuristics to set memory limit for mutants running with PHPUnit #258

Conversation

sanmai commented Mar 24, 2018 • edited

maks-rafalko left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanmai Mar 25, 2018 • edited

Choose a reason for hiding this comment

sanmai commented Mar 24, 2018 • edited

sanmai commented Mar 24, 2018 • edited

sanmai commented Mar 24, 2018

theofidry commented Mar 24, 2018

sidz left a comment • edited

Choose a reason for hiding this comment

sanmai commented Mar 25, 2018

sanmai commented Mar 25, 2018

sidz commented Mar 25, 2018

sanmai commented Mar 25, 2018

sidz commented Mar 25, 2018

sanmai commented Mar 25, 2018 • edited

maks-rafalko left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maks-rafalko commented Mar 27, 2018

sidz commented Mar 27, 2018

maks-rafalko commented Mar 27, 2018 • edited

maks-rafalko commented Mar 28, 2018

sanmai commented Mar 24, 2018 •

edited

maks-rafalko left a comment •

edited

sanmai Mar 25, 2018 •

edited

sanmai commented Mar 24, 2018 •

edited

sanmai commented Mar 24, 2018 •

edited

sidz left a comment •

edited

sanmai commented Mar 25, 2018 •

edited

maks-rafalko left a comment •

edited

maks-rafalko commented Mar 27, 2018 •

edited