More than one invocation handler has been configured triggered on a valid test case

[morozov]

Q	A
PHPUnit version	9.2.2
PHP version	7.4.7
Installation Method	Composer

Summary

There is an existing test that enforces that the class will call a specific method at certain moments but not more than the expected number of times (ref). This is implemented via a combination of self::at() and self::exactly().

As of #4258, PHPUnit warns about having more than one matcher for a given method invocation.

How to reproduce

<?php

use PHPUnit\Framework\TestCase;

interface T
{
    public function test() : void;
}

class TestTest extends TestCase
{
    public function testMatchers() : void
    {
        $mock = $this->createMock(T::class);
        $mock->expects(self::at(0))
            ->method('test');
        $mock->expects(self::at(1))
            ->method('test');
        $mock->expects(self::exactly(2))
            ->method('test');

        $mock->test();
        $mock->test();
    }
}

The question

What is the proper way to ensure that the method is called at certain positions and the exact number of times? Removing the self::exactly() clause will make it possible to call $mock->test() for the third time which would violate the contract but will not fail the test.

[morozov]

UPD: it was not meant to be reported as a bug. It's more of a support question.

[sebastianbergmann]

CC @jaapio

[jaapio]

Hi @morozov,

What you probably didn't notice in this test is that you are creating 3 invoke mocks.
1 accepts a call at position 0
2 accepts a call at position 1
3 forcing exactly 2 calls.

1 and 2 are called once. number 3 is called twice as you would expect. However if the latter would give you a return value that you expect things are getting unreliable. That's why the warning was introduced. In the linked test there is also a check on the arguments. which makes this situation even worse in combination with return types.

The best solution I know ATM is use

class TestTest extends TestCase
{
    public function testMatchers() : void
    {
        $mock = $this->createMock(T::class);
        $mock->expects(self::exactly(2))
            ->method('test')->withConsecutive(
                 ['call_1_argument1'],
                 ['call_2_argument1'],
        )

        $mock->test();
        $mock->test();
    }
}

In your example, the call to at() doesn't make any sense. The sequence doesn't matter in this situation.

I do need to check if this is also the case for the linked doctrine test. Which seems a bit more complicated. I might have overlooked a scenario. However, I doubt if this is really testing what you are expecting it to test. Which was not the case in the example you gave.

I will come back to you when I found out more.

[jaapio]

@sebastianbergmann It seems that I overlooked a scenario :-( which makes my patch completely invalid for this scenario. When I created the patch I didn't realize that there was a stranger at() in the matchers. The whole implementation was modified most likely with the introduction of at() back in 2006. Causing al kind of strange behavior on the existing mock invoke matchers. The at() expectation is the only matcher that works with the multiple matches.

The code below works with phpunit 9.1 but doesn't with the PR I submitted.

        $mock = $this->getMockBuilder(\stdClass::class)
            ->addMethods(['foo'])
            ->getMock();

        $mock->expects($this->at(0))
            ->method('foo')
            ->with('bar')
            ->willReturn('1');

        $mock->expects($this->at(1))
            ->method('foo')
            ->with('foo')
            ->willReturn('2');

        $mock->expects($this->exactly(2))->method('foo');

        self::assertSame('1', $mock->foo('bar'));
        self::assertSame('2', $mock->foo('foo'));

This is a BC break. Which was undetected until now.

At first, it looked like a quick win to make the mocks more reliable. If I had more time I would suggest deprecating the at() modifier. In favor of a better solution to ensure the order of calls. But I do bearly have enough time at the moment to work on my own open-source projects. So I won't be able to build a new mechanism to cover this case.

Besides that, I would like to make a statement to the users of at(). There is a big warning in the docs of phpunit that you are coupling your test to strict to the actual implementation. Seen the implementation of mocks in phpunit I would highly recommend avoiding the usage of this matcher.
You might get used to the way phpunit behaves with test doubles. But this modifier is an edge case. Al kind of constructions you are building with at() can not work without it. Your test will be a mess when you are going to refactor things. Or when more test doubles are introduced. Since at() is counting ALL test doubles.

Long story short, we should revert the patch of #4258 or accept this BC break.

[morozov]

Thank you for the proposed workaround and the research you've done, @jaapio!

Personally, I share the sentiment about the riskiness and sometimes the questionable value of using at() in tests. As a PHPUnit user, I would totally accept it being deprecated in favor of improving the PHPUnit internals quality.

[sebastianbergmann]

@jaapio Can you send a pull request against 9.2 that reverts what needs reverting? Thanks!

[sebastianbergmann]

@jaapio Nevermind, you're busy, I'll take care of this.

[nathanphan]

Thanks guys for the great work. Could you please give me some insights on how to replace this matcher at()? thanks

[zviryatko]

@nathanphan you can make it that way:

        $mock->expects($this->any())
            ->method('foo')
            ->withConsecutive([
                ['bar'],
                ['baz'],
            ])
            ->willReturnOnConsecutiveCalls('1', '2');