Removing hard-coded memory_limit#6916
Removing hard-coded memory_limit#6916ThomasLandauer wants to merge 1 commit intoCodeception:mainfrom
memory_limit#6916Conversation
Fixes Codeception#6840 This line was introduced in 2013 by Codeception@558498e Overriding what the user has in `php.ini` doesn't make sense to me.
|
As already mentioned in #6840 (comment) the memory limit is not hard-coded but configurable as documented in https://codeception.com/docs/reference/Configuration#settings. |
|
|
||
| public function run(string $suite, ?string $test = null, ?array $config = null): void | ||
| { | ||
| ini_set('memory_limit', $this->config['settings']['memory_limit'] ?? '1024M'); |
There was a problem hiding this comment.
@ThomasLandauer Doesn't this PR make the config option memory_limit useless?
There was a problem hiding this comment.
Yes! Having an extra memory limit doesn't make sense to me. If I set 3G in php.ini, then I expect it to be 3G - and not overridden by some obscure setting buried somewhere ;-)
This is a leftover from the early days of Codeception - back in 2013, maybe nobody had tests that needed more than 1G.
There was a problem hiding this comment.
There are other code places which read $this->config['settings']['memory_limit'] as well. Is it intended that they are not touched by this PR?
I'm afraid changing the behavior of the memory_limit config option could be considered a BC break and thus would require a new major version.
There was a problem hiding this comment.
- No, that's not intended, but forgotten ;-) I just wanted to get some feedback first before investing more time...
- I'm certainly no SemVer expert! But as far as I understand it: When this limit is reached, Codeception crashes. At least, that's what I observed. So dropping the limit cannot break anything, but (at most) make something work.
In any case: I didn't get a useful answer to Test runner sometimes just stops, sometimes gives "Allowed memory size exhausted" error #6840 in 10 months. And it was annoying to debug, cause it only affects large test suites, and you (obviously) always need to run them completely. => So IMO this would be worth even a new major version.
There was a problem hiding this comment.
But as far as I understand it: When this limit is reached, Codeception crashes. At least, that's what I observed. So dropping the limit cannot break anything, but (at most) make something work.
Yes, Codeception might crash, but this could be considered a BC break because it would not have been crashed without this PR.
There was a problem hiding this comment.
So dropping the limit cannot break anything, but (at most) make something work.
I think this assumption is only true if the memory_limit in your php.ini is greater than the Codeception default of 1G. According to https://www.php.net/manual/en/ini.core.php#ini.memory-limit the default would be only 128M if you don't set anything explicitly in your php.ini, which would mean that if we don't adjust this in Codeceptino to 1G by default anymore, I'd expect tests to crash for at least some people where they ran fine before.
|
OK, I must admit that "hard-coded" isn't the right term ;-) It's rather something like a "hidden" limit... |
3 participants
Fixes #6840
This line was introduced in 2013 by 558498e
Overriding what the user has in
php.inidoesn't make sense to me.