Skip to content
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

Add onlyMethods + addMethods and soft deprecate setMethods #3687

Closed

Conversation

DFoxinator
Copy link
Contributor

@DFoxinator DFoxinator commented May 9, 2019

The purpose of this PR is to provide important testing functionality when using setMethods() on a mock, to which there is currently no workaround to accomplish otherwise (as far as I can see, please correct me if I'm wrong). Right now, any time setMethods is used, the test using it is completely unprotected against any of the methods sent to setMethods not actually existing in the class. In practice, for the large majority of use-cases where setMethods is appropriate, this means that the tests using it are likely not functioning as the test writer expected because the methods in setMethods are not asserted in any way to actually exist, so you can easily just delete methods while refactoring code and have properly written tests still pass with no warning. In practice, this behavior is made even more confusing because so far every developer I've talked to mixes up the strictness of createMock (which tells you when you try to mock a method that didn't exist) with also offering that functionality when you use setMethods, which unfortunately is not the case.

This change provides a simple backwards-compatible way to address this issue, and provide the functionality that most casual PHPUnit users already think they are getting. Anyone who wants the stricter functionality can just search and replace setMethods with the stricter version that asserts the methods actually exist. Also, just to add, the name I added for this new method is kind of just one I came up with quickly - so feedback on that and anything else in this is definitely appreciated.

@codecov
Copy link

codecov bot commented May 9, 2019

Codecov Report

Merging #3687 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3687      +/-   ##
============================================
+ Coverage     82.38%   82.46%   +0.08%     
- Complexity     3802     3813      +11     
============================================
  Files           150      150              
  Lines         10054    10100      +46     
============================================
+ Hits           8283     8329      +46     
  Misses         1771     1771
Impacted Files Coverage Δ Complexity Δ
src/Framework/TestCase.php 80.18% <100%> (+0.25%) 324 <0> (+3) ⬆️
src/Framework/MockObject/MockBuilder.php 66.9% <100%> (+11.13%) 31 <8> (+8) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bf3d5a...9f4764f. Read the comment docs.

Repository owner deleted a comment from DFoxinator May 9, 2019
Repository owner deleted a comment from muglug May 9, 2019
Repository owner deleted a comment from muglug May 9, 2019
Repository owner deleted a comment from DFoxinator May 9, 2019
@psalm-shepherd
Copy link

Psalm didn’t find any errors!

@sebastianbergmann sebastianbergmann added the feature/test-doubles Stubs and Mock Objects label May 10, 2019
@DFoxinator DFoxinator force-pushed the setrealmethods branch 4 times, most recently from 3d94cda to e92338d Compare May 11, 2019 16:09
@sebastianbergmann
Copy link
Owner

sebastianbergmann commented May 13, 2019

any time setMethods is used, the test using it is completely unprotected against any of the methods sent to setMethods not actually existing in the class

The whole point of setMethods() is to configure methods that do not (yet) exist. Can you please provide a use case for the setRealMethods() method you are proposing here?

functionality that most casual PHPUnit users already think they are getting

Most PHPUnit users, casual or not, should not use the MockBuilder API to begin with but use createMock() etc.

@DFoxinator
Copy link
Contributor Author

DFoxinator commented May 13, 2019

The use case is when you need to mock some methods on a class but not all of them. As far as I know, you can't use createMock for that. I've always used createMock for all dependencies (where you don't need any functionality from the actual class), but for testing actual service classes (that have methods that need to be mocked) where those dependencies are passed in, createMock doesn't seem like an option.

So for any use case where you want to test a class but also mock out some methods (in my case, that's generally for mocking public methods on the service and also protected/trait methods that make external calls), from my understanding, you'd use MockBuilder and setMethods for that right now, and I've always seen that in practice in the codebases I've worked on. But the problem I've encountered here is using setMethods makes the test ineffective because there's no check to see if the methods being mocked are real/actually exist in the class. So in a recent case. I saw prod code that was broken because a test was properly mocking data using setMethods and a mock for the method, but the method itself had been removed from the code but was still being called from that service that was being tested.

@sebastianbergmann
Copy link
Owner

You are correct: createMock() does not support partial mocking. But createPartialMock() does. However, since that is just a wrapper for the Mock Builder API and because it uses setMethods() internally there is no check for method existence.

@DFoxinator
Copy link
Contributor Author

DFoxinator commented May 13, 2019

Yeah, that makes sense. It seems for this use case right now there really is no workaround, and in the codebase I work on, in the hundreds of places MockBuilder setMethods/createPartialMock is used, the behavior that was intended was the test would fail if any of the methods that were attempted to be mocked were removed from the code. The only use case we could think of where you wouldn't want that check tied to setMethods is for magic methods maybe, so hence the idea for this new setRealMethods for the majority of standard test cases that can't use createMock and require MockBuiler and some methods mocked but not all.

@sebastianbergmann
Copy link
Owner

I am not happy with the name setRealMethods(). For that matter, I am also not happy with setMethods().

How about:

  • Introduce addMethods() for adding additional methods that do not (yet) exist in the doubled interface or class
  • Introduce onlyMethods() for selecting which set of methods in the doubled interface or class should be doubled
  • Figure out whether addMethods() and onlyMethods() should be combinable (I don't think they should be)
  • Deprecate setMethods() in PHPUnit 8.x (soft)
  • Trigger a warning when names for methods that do not exist are passed to createPartialMock() in PHPUnit 8.x
  • Let createPartialMock() use onlyMethods() in PHPUnit 9
  • Deprecate setMethods() in PHPUnit 9 (hard)
  • Remove setMethods() in PHPUnit 10

@DFoxinator
Copy link
Contributor Author

@sebastianbergmann makes sense, sounds like a solid plan to me. I will try to update this PR later today based on that plan. Thanks!

@DFoxinator DFoxinator changed the title MockBuilder method to verify mock method existence Add onlyMethods + setMethods and soft deprecate setMethods May 15, 2019
@DFoxinator DFoxinator changed the title Add onlyMethods + setMethods and soft deprecate setMethods Add onlyMethods + addMethods and soft deprecate setMethods May 15, 2019
@DFoxinator DFoxinator force-pushed the setrealmethods branch 3 times, most recently from 9c4c1f7 to f3b6b02 Compare May 15, 2019 16:07
soilSpoon added a commit to soilSpoon/framework that referenced this pull request Dec 3, 2020
`MockBuilder::setMethods` is soft deprecated as of PHPUnit 9
So I refactor the test.

Please merge laravel#35474  after merging

sebastianbergmann/phpunit#3687
soilSpoon added a commit to soilSpoon/framework that referenced this pull request Dec 4, 2020
`MockBuilder::setMethods` is soft deprecated as of PHPUnit 9
So I refactor the test.

Please merge laravel#35474  after merging

sebastianbergmann/phpunit#3687
soilSpoon added a commit to soilSpoon/framework that referenced this pull request Dec 4, 2020
`MockBuilder::setMethods` is soft deprecated as of PHPUnit 9
So I refactor the test.

Please merge laravel#35474  after merging

sebastianbergmann/phpunit#3687
taylorotwell pushed a commit to laravel/framework that referenced this pull request Dec 4, 2020
`MockBuilder::setMethods` is soft deprecated as of PHPUnit 9
So I refactor the test.

Please merge #35474  after merging

sebastianbergmann/phpunit#3687
@acewebs
Copy link

acewebs commented Jan 8, 2021

All sounds good except case scenario:
What if I am trying to mock a non-existent class.

In this case setMethods would have allowed me to do so, whereas addMethods and onlyMethods will first check the methods against the class I have mocked, which will result in an Class "NonExistentClass" does not exist error.

Maybe a check can be added if $this->allowMockingUnknownTypes = true then the ReflectionClass check is bypassed

@stronk7
Copy link
Contributor

stronk7 commented Feb 21, 2021

Just linking from here to existing documentation change PR, because it seems that current version docs still are missing this stuff: sebastianbergmann/phpunit-documentation-english#205

Ciao :-)

@Levivb
Copy link

Levivb commented Mar 31, 2022

Is there an alternative to setMethodsExcept?

It's deprecated and links to this issue, but it seems there's no alternative?

@dylan-marty-boldcommerce

Is there an alternative to setMethodsExcept?

It's deprecated and links to this issue, but it seems there's no alternative?

Alternatives were tucked into some of the thoughts on a previous comment here: #3687 (comment)

  • [...] addMethods() for adding additional methods that do not (yet) exist in the doubled interface or class
  • [...] onlyMethods() for selecting which set of methods in the doubled interface or class should be doubled

I would imagine for most use cases, you're looking for onlyMethods instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/test-doubles Stubs and Mock Objects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet