Deprecate TestCase::getMockBuilder()

[sebastianbergmann]

These deprecations ...

• Deprecate TestCase::createPartialMock() #5239
• Deprecate TestCase::createTestProxy() #5240
• Deprecate TestCase::getMockForAbstractClass() #5241
• Deprecate TestCase::getMockFromWsdl() #5242
• Deprecate TestCase::getMockForTrait() #5243
• Deprecate TestCase::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:

• soft deprecation in PHPUnit 10.1 (add @deprecated annotation to the method declaration)
• deprecation in PHPUnit 11 (using the method will trigger a deprecation)
• removal in PHPUnit 12

[lolli42]

Could you give some more insight on this?

We're using getMockBuilder() directly and indirectly ~1200 times in our project (~18k phpunit based tests), it will be a massive burden when phpunit drops this.

I guess various of them could be substituted by createMock(), but we have some use cases that will be much harder to refactor:

• Some of our classes still have ugly constructors that are hard to get rid of. We still want to test detail methods of those. Tests then tend to create a mock that suppress the constructor, but keep other parts as test subject.

• Sometimes we only partially mock a test subject class: Test an upper method, but mock a sub method of the class, then check called arguments.

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.

[sebastianbergmann]

If I understand the issue correctly, it means: Removed phpunit support for anything "partial". Is that correct?

That is correct.

it will be a massive burden when phpunit drops this.

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.

[mfn]

Drive by question: I use label:type/deprecation on github search to stay on top of issues you document here ( 👍🏼 btw), but I missed this one so far because it has a different label. Just wanted to mention it, maybe the label should be there, maybe not 🤷🏼

[lolli42]

And afterwards your tests will be better.

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 withConsecutive() as example to make my point: This helper may be ugly and kinda hard to understand, which is probably why you wanted to get rid of it, but people use it. People are now looking for substitutions with phpunit 10. Google for "phpunit withConsecutive() deprecated":

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.
Even big projects like drupal get this wrong first, see drupal/drupal@2400990 (they reverted later again).
A (hopefully) working solution needs to look somehow similar to this: TYPO3/typo3@b2aa4c2

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.

[sebastianbergmann]

Phpunit does not even state "The mock codebase is such an evil mess, we simply can't maintain all this" or something like that.

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 getMock() existed and had 10+ optional parameters and almost everyone needed to pass true for one of the last ones to disable the original constructor. Unfortunately, I did not immediately come up with the idea of creating what is now createMock(), to which later createStub() was added. The Mock Builder API should have been deprecated when createMock() was introduced, but instead it continued to grow more or less unchecked.

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

Such an old stable branch should not be updated to a new version of PHPUnit.

Phpunit tries to force an opinionated idea of how code should look with this change.

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 withConsecutive() that is easy to understand. The fact that you need "magic features" like these is an indicator that the code you are testing is not testable (enough).

I am convinced the functionality that is scheduled to be removed does more harm than good (with the exception of createTestProxy() which I still think is a neat idea but I have never seen it used in the wild outside of the project that sparked the idea to implement this feature).

(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.)

[sebastianbergmann]

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 withConsecutive() in a (hopefully) better way.

[sebastianbergmann]

We're using getMockBuilder() directly and indirectly ~1200 times in our project (~18k phpunit based tests), it will be a massive burden when phpunit drops this.

Can you please elaborate what you use getMockBuilder() for exactly? Thanks!

In the meantime, I have reverted the deprecation of getMockBuilder() and createPartialMock() in bce1957.

[lolli42]

Thank you Sebastian! I hope you have / had some good easter vacation.

Looking through our usages, it seems as if disabling constructor with disableOriginalConstructor() and onlyMethods() are the two crucial partial-mock details we rely on. I'd say this is the most "sane" usage of partial mocks: Testing detail methods of bigger classes that can't be / shouldn't be refactored currently.

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 disableOriginalClone(), but think we could work around usages if needed. We do use addMethods(), but those look like leftovers we should clean up: There shouldn't be many reasons to dynamically add methods to a class, and if so, a fixture class could / should be used instead. We don't use disableAutoload().

All in all, getMockBuilder()->disableOriginalConstructor()->onlyMethods()->getMock() is a frequently used path for us, then defining expectations and/or returns on mocked methods. It feels to me as if this is the most useful construct when dealing with test subjects that can't be refactored otherwise, while other details are less important or so ugly that they should be refactored with higher priority.

What do you think?

[sebastianbergmann]

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.