Add option for disabling return value generation for stubbed methods

[lstrojny]

The automatic return value inference makes it oftentimes harder to do wider refactorings across bigger code bases. This PR should serve as a discussion starter to add a mocking mode, that turns this feature off.

TODO

• [ ] What to do with createMock(), createConfiguredMock() etc. Idea would be to add createStrictMock(), createConfiguredStrictMock()

[sebastianbergmann]

Can you elaborate on the problem you are trying to solve? As far as I understand it right now, this only creates new problems (due to TypeErrors when stubbed methods that have a return type declaration are called but have no configured return value).

[lstrojny]

So right now once I create a mock but not set up expectations for a method, it tries to come up with a default (in PHPUnit\Framework\MockObject\Invocation::generateReturnValue()). Let’s imagine this diff in a SUT:

class Collaborator
{
    public function execute(Something $something)
    {
 -          if ($something->isSomething()) {
 +          if (!$something->isSomethingElse()) {
               // some additional behavior
          }
    }
}

My testcase will now silently test the opposite case and it will be quite hard to understand from the failing test what the cause is. I would much rather have the test telling me: "you forgot to set up an expectation for a isSomethingElse()". That way I know what the change is and how to change the test accordingly.

[sebastianbergmann]

Even if "automatic return value inference" is made optional it will 1) remain enabled by default and 2) no new convenience methods (createStrictMock(), ...) will be added. This option is the perfect use case for the Mock Builder API.

[lstrojny]

I agree that the automatic generation must stay the default but after seeing it play out in real world projects I don’t think it’s a good default (just like before createMock() the defaults for getMock() weren't good). I think test suites would greatly benefit from more explicit dependency setup so I think it would be worth thinking about providing convenience methods along the lines of createMock() and createConfiguredMock().

[lstrojny]

That being said, we can certainly start with making the behavior accessible through the mock builder only first and see where it goes. Either it becomes more popular and we want to add shortcuts later or it doesn’t.

Do you have any feedback on the naming or the route the implementation goes down?

[sebastianbergmann]

I need to think some more about the naming.

[lstrojny]

On naming, these are my two ideas right now:

• return value inference
• return value generation

[sebastianbergmann]

"return value generation" makes more sense to me.

[lstrojny]

Adjusted naming, added method to the MockObject interface and added a (hacky) way to handle __toString(), where we cannot throw an exception.

[codecov-io]

Codecov Report

Merging #405 into master will increase coverage by 0.05%. The diff coverage is 78.78%.

@@             Coverage Diff              @@
##             master     #405      +/-   ##
============================================
+ Coverage     72.53%   72.59%   +0.05%     
- Complexity      438      446       +8     
============================================
  Files            27       28       +1     
  Lines          1238     1266      +28     
============================================
+ Hits            898      919      +21     
- Misses          340      347       +7

Impacted Files	Coverage Δ	Complexity Δ
src/Generator.php	84.05% <100%> (+0.06%)	177 <37> (ø)	:arrow_down:
src/InvocationMocker.php	76.27% <100%> (+5.43%)	24 <1> (+2)	:arrow_up:
src/Matcher/DeferredError.php	50% <50%> (ø)	4 <4> (?)
src/MockBuilder.php	56.6% <62.5%> (+0.03%)	23 <2> (+2)	:arrow_up:

---

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 a498cb0...b8911fb. Read the comment docs.

[lstrojny]

Updates:

• Extracted inner class to DeferredError
• Updated all tests
• Sprinkled a few type hints

[lstrojny]

@sebastianbergmann ready to merge from my POV

[sebastianbergmann]

Thanks!