Open mollierobbert opened 8 years ago
Any news on this, @mlively? Should I submit a pull request for it?
Sorry, this ticket actually revealed some fairy serious issues with the entire concept of how static mocking works that I have to sort through. Static mocks can't easily be repeatedly mocked without busting the caching which then results in fairly hefty memory usage. The reason why you are having difficulty is because you really only get 1 shot to set the default answer for a static mock. That's the fundamental thing that has to be fixed and I've been tied up with some other projects.
You are more than welcome to take a crack at it and I can make sure to take time to review. It is a bit more involved then I would expect anyone to offer their free time for, but I would gladly accept and appreciate any help you'd offer :).
On Mon, Jun 20, 2016 at 6:24 AM, Robbert notifications@github.com wrote:
Any news on this, @mlively https://github.com/mlively? Should I submit a pull request for it?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mlively/Phake/issues/209#issuecomment-227140698, or mute the thread https://github.com/notifications/unsubscribe/AAKwFlDn578ei_xFY3dnJ1KbFqgE7ZR2ks5qNpSEgaJpZM4Gz9KZ .
I forked 2.1.1 and made a change which would allow this to work (basically set a default answer for all static methods of thenCallParent when creating the class).
It seems to work in 3.1.6 as well, so when I get a chance I'll put my change up as a sort of Request for Comments. It's not an ideal solution though.
Hi, I've submitted a fix, that makes partialMock call parent for static methods and disables class caching for partial mocks. It works for me, but may be not optimal, as generates a new class for each partial mock of given base class.
Hi :-) Could you please submit a pull request ? I see that you created a fix on your repo but you need to submit a pull request.
Thanks
@adoy looks like the pull request is open. https://github.com/phake/phake/pull/305
Any update on merging it in?
I added some comments on the code review. I have some concerns with memory usage with the crurent implementation.
@adoy I don't see any comments on the PR.
My concern with this PR is that with the proposed implementation, every time a partialMock is created, a new class is declared which increase the memory usage and might be an issue. I wonder if there is a possible solution to reduce the number of generated classes.
I wrote a quick script to see if there was any noticeable difference in the memory usage (https://gist.github.com/martinbutt/629293e2243675ab166ee2049c70e43f). Running it against both the old code and the new code shows an increase in memory usage:
Before: Memory allocated: 3939384 Memory used: 4194304
After: Memory allocated: 20497912 Memory used: 20971520
In practice with my test suite this is not noticeable, as the mocks are destroyed after every test. However, if the test suite had not been written that way, it could cause memory bloat.
The problem is that it's a class definition, not a class instance. And PHP will only free this memory when the "request" is finished. So even if your code is super clean, you will still have memory increasing each time you mock an object.
Can you provide the memory usage if you remove the object storing each instance ? If i'm not wrong the amount of memory beetween the before and after usecase should be approximativaly the same.
Without storing the objects:
Peak memory before PR: : 4194304 Peak memory after PR: 17987256
Static methods of a partial mock always return
NULL
, unless aPhake::when($partial_mock)->staticMethod()->thenCallParent()
is explicitly defined. I don't think this is expected behaviour - a partial mock should always call the parent by default, right?In
Phake_ClassGenerator_MockClass::generate()
, the static mock info is always defined with aPhake_Stubber_Answers_NoAnswer
as default answer.In
Phake_Facade::mock()
, if we pass on the$defaultAnswer
variable to the$mockGenerator->generate
call, the issue is fixed.