Improvements on withConsecutive with return

[jaapio]

As a developer using the build-in mocks of PHPUnit can be complicated. One of the things I do have to look up aways is the way to have withConsecutive mocks. On the other hand, when using the willReturnOnConsecutiveCalls in combination willReturnConsecutive is a real issue.

Stubs do have a somewhat better method called willReturnMap but that one is using a less complicated check to match the arguments.

In an example where we have something like this:

        $this->client->expects($this->exactly($calles))
            ->method('doRequest')
            ->withConsecutive(
                [
                    /* cut checks for simplification */
                    /* cut checks for simplification */
                ],
                [
                    /* cut checks for simplification */
                    /* cut checks for simplification */
                ]
            )->willReturnOnConsecutiveCalls(
                new Reponse(),
                new Reponse(),
            );

The problem here is that there is a relation between willReturnOnConsecutiveCalls and withConsecutive but it is not really clear to everyone that this relation is here. Especially when the number of checks done in the withConsecutive arrays is growing, it gets unreadable. because the lines of argument matchers are not fitting into your screen anymore.

Using willReturnMap is not an option in this case, because the subject under test has some internal complexity that needs to be tested. And the arguments of doRequest are objects so checking needs a callback.

In the past, I created a manual test double that was implementing a certain interface and just stores the calls, like some kind of spy object. But in the end, it would be a nice feature to have this in the mocks so those extra classes are not required anymore.

What I would like to propose is a combination of willReturnMap and withConsecutive and willReturnOnConsecutiveCalls or maybe an extension of willReturnMap that will allow a developer to add Constraints into the return map. I'm a bit in doubt if this will work properly since the new method will mix the concepts of parameterMatchers and stubs for the return types. Which I dislike in willReturnMap

Another approach would be more like a builder type of implementation. So we could use the existing calls to create a mock.

        $mock->expects($this->once())->method('doRequest')->with()->willReturn();
        $mock->expects($this->once())->method('doRequest')->with()->willReturn();

This doesn't work currently because the matcher will throw an error. A more suitable solution would be something like the example below. The order of calls to with and willReturn would set the expected order of calls just like withConsecutive and willReturnOnConsecutiveCalls

        $mock->expects($this->once())->method('doRequest')
        ->with()->willReturn()
        ->with()->willReturn();
   

I think this will make it easier for new developers to start using the mocks.

If you agree with me that the suggestion above should be implemented I will try to get a PR ready.

[jaapio]

@sebastianbergmann while I was doing some research on how to implement this I found the flowing lines of code.
\PHPUnit\Framework\MockObject\InvocationHandler::invoke

        foreach ($this->matchers as $match) {
            try {
                if ($match->matches($invocation)) {
                    $value = $match->invoked($invocation);

                    if (!$hasReturnValue) {
                        $returnValue    = $value;
                        $hasReturnValue = true;
                    }
                }
            } catch (\Exception $e) {
                $exception = $e;
            }
        }

I'm almost sure that this part is not directly on top of your mind. Since the last changes were just style changes. Nothing has been changed here since 2006. I would not remember why I wrote that this way. But what this part of the InvocationHandler is doing is trying to find the correct match by method name. The problem here is this:

    public function testMultipleWithParametersWillReturnLatestDefined(): void
    {
        $mock = $this->getMockBuilder(stdClass::class)
            ->setMethods(['foo'])
            ->getMock();

        $mock->expects($this->any())
            ->method('foo')
            ->willReturnOnConsecutiveCalls()
            ->with('bar')
            ->willReturn('first');


        $mock->expects($this->any())
            ->method('foo')
            ->with('foo')
            ->willReturn('second');

        $this->assertSame('first', $mock->foo('bar'));
        $this->assertSame('second', $mock->foo('foo'));
    }

Both of the asserts above will fail. because the loop in \PHPUnit\Framework\MockObject\InvocationHandler::invoke will always loop over all registered matchers. If the method name matches the mockmethod will be invoked. But since both calls are matching foo but they do not have the same argument, this will always fail.

foo('bar') will fail on the second matcher
foo('foo') will fail on the first matcher.

Everyone searching for this will find out that withConsecutive would "fix" this issue. But this is somewhat unexpected. The whole reason why I opened this issue.

But now I'm not sure what I should do ATM, so I would like to have some advice from someone more familiar with this part of PHPUnit.

From my point of view, I think we should collect matchers for the same method into a single aggregate that can verify the parameters of each call. if none of them is matching an error will be reported with the expected calls.

Does that sound ok?

[sebastianbergmann]

I did not write that code to begin with, so even back in 2006 I was not familiar with it :-/

No, it does not make sense the way it is right now. Changing it to make sense very likely breaks backward compatibility. So I think we should deprecate withConsecutive() in PHPUnit 9 and remove it in PHPUnit 10. This requires, of course, that we have a viable replacement. It sounds to me like you (and @dbrekelmans in #4026) have an idea of what this replacement should look like. I look forward to your pull request :-)

[dbrekelmans]

@jaapio Let me know if you need help implementing this. I'd be happy to lend a hand.

[jaapio]

I have way too much on my plate with other side-projects. So I won't be able to work on this. If anyone else is able to work on this feel free to provide a PR. If you want to discuss something with me about my suggestion, feel free to contact me at any time.

See you around.

[dbrekelmans]

@sebastianbergmann I think this issue can be closed based on #4026 (comment) and #4564