-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate TestCase::getMockBuilder()
#5252
Comments
TestCase::getMockBuilder()
Could you give some more insight on this? We're using I guess various of them could be substituted by createMock(), but we have some use cases that will be much harder to refactor:
One could argue such cases are ugly, and we're improving the situation, but it's still an important use case for us. If I understand the issue correctly, it means: Removed phpunit support for anything "partial". Is that correct? What's the alternative? Have fixture classes extending the test subject to "empty" the constructor? Have fixture classes that "empty" sub-methods to check arguments when called? Stuff like that is much more easy to create, read and follow, when you can create dedicated mocks in the tests for such things. |
That is correct.
I understand that, and I am sorry for the inconvenience. However, you can start making the required changes today and you have until February 07, 2025 to finish this work. And afterwards your tests will be better. |
Drive by question: I use |
Real life disagrees. I hope it's ok if I push here a bit, since your planned removal will have a massive impact on phpunit consumers. Let me pick the removal of https://stackoverflow.com/questions/75389000/replace-phpunit-method-withconsecutive Not a single answer here is a "good" solution (at the time of this writing). They're not asserting properly, and things like that. So, what happens here currently? Phpunit decided to drop functionality without giving neither an alternative, nor an advise on how it could be substituted in a relatively good way. Working solutions are code wise clearly more ugly and kinda brain melt, and have the tendency to not get details right. Result: Tests get worse. Not better. The mocking framework is a heavily used construct in the wild. People do use the mock builder to tests parts of classes. There are many reasons to do that. For instance, when you're adding a regression test to an old-stable branch where you can't just refactor the system under test along the way. Phpunit tries to force an opinionated idea of how code should look with this change. Removing the mock builder not only means changing tests, but often forces to refactor the system under test. However, refactoring decisions should be taken by project maintainers, not forced by phpunit. Phpunit does not even state "The mock codebase is such an evil mess, we simply can't maintain all this" or something like that. This change feels like a "We don't like your tests (and your code) when you need to use this, so we take this away from you". Here is what will happen from my point of view, when this materializes: Just like with withConsecutive(), people will become very creative on how to solve the removal of the mock builder. I think many devs understand tests are important, but they still hate them. When phpunit breaks something like that, devs will try to accommodate somehow, so many people will go with the least amount of work required to make it green again. The result will be tests that are worse than before in real life, since they got things wrong which phpunit got quite right before. What do you think? I'd really love to get more insight on this from phpunit point of view since I think the impact is huge, as well as your ideas on appropriate alternatives in case this strategy is followed. |
I have stated over and over again that parts of PHPUnit's test double functionality are bad, either internally which makes them hard to maintain, or externally which leads to hard to understand test code. These changes will be made primarily to reduce the burden of the people working on PHPUnit. For instance, the Mock Builder API was added back when
Such an old stable branch should not be updated to a new version of PHPUnit.
I hope you believe me when I say that I am not doing these changes out of spite and because I enjoy causing problems. Do I have opinions? Sure. Are those opinions reflected in the software that I write? Also: sure. But these opinions are the result of experience: I have yet to see a test that uses partial mocks, or mocks of abstract classes, or I am convinced the functionality that is scheduled to be removed does more harm than good (with the exception of (I also think that PHPUnit's test double functionality should only support interfaces, but I am convinced that dropping support for doubling classes would be too much.) |
Do I understand you correctly, @lolli42, that you only consider partial mocking as critical functionality that should be kept? If that is the case, I think that we can keep it (and clean up its implementation after the other functionality has been removed). No promises (yet), I have to research this. But that research has to wait until after my vacation. In the meantime, I would like to hear your feedback on #5203 which is a PR that aims to restore the functionality of |
Can you please elaborate what you use In the meantime, I have reverted the deprecation of |
Thank you Sebastian! I hope you have / had some good easter vacation. Looking through our usages, it seems as if disabling constructor with Other details like getMockForAbstractClass() are more seldom and usually indicate an ugly construct in cases where it is mocked away as dependency. We should refactor those to avoid them with higher priority, for instance by refactoring towards an interface. The other use case of this is to test some detail method of the abstract directly - those can be easily avoided using a fixture class that extends the abstract, and is thus not a huge issue. I'm unsure about All in all, What do you think? |
Thank you for your feedback, I hoped as much. I have some ideas for making partial mocking better. For now it stays as it is. |
These deprecations ...
TestCase::createPartialMock()
#5239TestCase::createTestProxy()
#5240TestCase::getMockForAbstractClass()
#5241TestCase::getMockFromWsdl()
#5242TestCase::getMockForTrait()
#5243TestCase::getObjectForTrait()
#5244... also affect the methods of
PHPUnit\Framework\MockObject\MockBuilder
to configure the respective functionality.The functionality configurable by
PHPUnit\Framework\MockObject\MockBuilder
that remains (not disabling the original constructor, not disabling the orginal clone constructor, cloning arguments passed to doubled methods, allowing the doubling on unknown types, and disabling automatic return value generation) should also be deprecated.The
TestCase::getMockBuilder()
method will be deprecated and then removed:@deprecated
annotation to the method declaration)The text was updated successfully, but these errors were encountered: