Phpunit: Deprecate at() matcher

[sebastianbergmann]

The behaviour of at() is confusing. Using at() leads to brittle tests at best and lying tests at worst. The documentation even states:

The $index parameter for the at() matcher refers to the index, starting at zero, in all method invocations for a given mock object. Exercise caution when using this matcher as it can lead to brittle tests which are too closely tied to specific implementation details.

Furthermore, the implementation of at() impedes improvements to other aspects of the test doubles support in PHPUnit, see https://github.com/sebastianbergmann/phpunit/issues/4291#issuecomment-644947020 for example.

We should therefore deprecate

• PHPUnit\Framework\TestCase::at() (as well as the PHPUnit\Framework\at() wrapper)
• PHPUnit\Framework\MockObject\Rule\InvokedAtIndex

in PHPUnit 9 and remove it in PHPUnit 10.

If we realize that a mechanism for specifying the order in which expected invocations occur then, and only then, shall we think about how to best implement this. A replacement for at() is explicitly outside the scope of this deprecation.

[Levivb]

I agree with the deprecation of this unholy method. Mostly withConsecutive and willReturnMap will suffice.

But what about this scenario?

        $this->pageBuilder
            ->expects($this->exactly(3))
            ->method('create')
            ->withConsecutive([$this->collection[0]], [$this->collection[1]], [$this->collection[2]]);

        $this->pageBuilder
            ->expects($this->at(1)) // the middle creation will error
            ->method('create')
            ->willThrowException($exception);

At which the second create call (index 1) should throw an exception?

Maybe something with $this->callback in combination with withConsecutive.. can't quite figure this one out

[allcode]

@Levivb This will work:

$this->pageBuilder
    ->expects($this->exactly(3))
    ->method('create')
    ->withConsecutive([$this->collection[0]], [$this->collection[1]], [$this->collection[2]]);
    ->willReturnOnConsecutiveCalls(
        null,
        $this->throwException(new RuntimeException('Some message')),
        null
    );

The only downside is that you'll have to define an explicit (and valid) return value for all calls.

[Levivb]

@allcode Thanks for your suggestion!

I did consider that. It's just that the method in question has typehint : void and it feels a bit awry to change the return type in production code to some nonsensical value just for the sake of satisfying a unit test

[ossinkine]

If we describe calling one method with different parameters, we don't need at method, but to describe the order of calling different methods, we need an alternative to the at method. For example, I want to check that the commit method is called after the begin method like this:

$this->connection->expects($this->at(1))->method('begin');
$this->connection->expects($this->at(2))->method('commit');

After removing at method I should check this myself something like this:

$transactionStarted = false;
$this->connection->expects($this->once())->method('begin')->willReturnCallback(function () use (&$transactionStarted) {
    $transactionStarted = true;
});
$this->connection->expects($this->once())->method('commit')->willReturnCallback(function () use (&$transactionStarted) {
    $this->assertTrue($transactionStarted);
});

In more complex tests, this code will become even uglier, so I think we definitely need an alternative to the at method for such cases.

[yyaremenko]

it would be extraordinary nice if you guys could create some sort of documentation about how to properly migrate from using this method (what methods to use instead of the deprecated one).

[pwellingelastique]

I could not agree more with @ossinkine, Sometimes I exactly want to test if a function is called on a specific position.

For instance I have this code:

class SomeClass
{
    /**
     * @var ContainerInterface
     */
    protected $container;

    public function __construct(ContainerInterface $container)
    {
        $this->container = $container;
    }

    function someName()
    {
        $input = [1, 2, 3];
        foreach ($input as $value) {
            if ($this->container->get('someService')->functionNameA($value)) {
                $this->container->get('someService')->functionNameB();
            }
        }
    }
}

class SomeService
{
    function functionNameA(int $value)
    {
        return $value == 2;
    }

    functionNameB()
    {
        // Do something
    }
}

If I wanted to test that functionNameB has been called as 3rd when testing this first method, I would have a very difficult time, since I want to test that it has been called for that specific value. (value == 2) and not value == 1 or value == 3
And that would also go for any other scenario I might think of

Having tested that it is called one thing, but knowing the order of execution is in some cases as important as knowing it has been called

Please reconsider

[artjomsimon]

@ossinkine what would happen if a client would call the begin and commit functions in the wrong order?
Would you throw an exception? Or is that a valid use case?

If throwing an exception, you could explicitly expect the exception to be thrown if you cann commit before begin.
If it's a valid use case, why test for the order? The returned value should be enough to test, no?

After all, you're testing your code, and how it reacts to being called with functions out-of-order.
You can't guarantee that a client will call the code in the correct order.

@pwellingelastique where does your $input = [1, 2, 3]; come from?
What if it becomes $input = [0, 1, 2, 3];, would you then want to change each ->at(n) to ->at(n+1)?
What if you introduce a dataProvider, testing different $inputs, like [-1, false, PHP_INT_MAX, null, true, 3]?
What are you testing here - how your code reacts to different inputs? Or the exact order in which some instructions are executed?

The idea of removing at() is to make people stop writing tests that are brittle. Tests that have to be changed when your actual code changes.

Your example is such a brittle example: Let's say you refactor and introduce a new function SomeService::functionNameBWhenAIsTrue() as a convenience function that does the if (...) check inside.

Now you have to change your tests for SomeClass. But you didn't change the logic in any way, you simply refactored the if () into SomeService. Changing the test for SomeClass should not be necessary.

Deprecating at(...) makes us think harder: What behavior am I actually testing when testing someName()?
And that's a Good Thing.

[ossinkine]

@artjomsimon My code calls the functions in certain order. I want to write a test which check the order is still correct. If I change my code where order was changed I want the test fails. If the code with wrong function calls run on production the exception will be thrown.

[artjomsimon]

@ossinkine it sounds like checking if function A was called is a responsibility of the class containing the two functions that need to be called in order:

class SomeService
{
    private bool $hasABeenCalled = false;

    function functionNameA(int $value)
    {
        $this->hasABeenCalled = true;
        return $value == 2;
    }

    functionNameB()
    {
        if ($this->hasABeenCalled === false) {
            throw new RuntimeException('You cannot call B before calling A. Call A first.');
        }
        // Do something
        $this->hasABeenCalled = false;
    }
}

Now, in your tests, you can test this behavior:

public function testSomeClassThrowsExceptionWhenCallingFunctionsInWrongOrder() : void {
    $this->expectException(RuntimeException::class);

    $service = new SomeService();
    $service->functionNameB();
    $service->functionNameA();
}

With the exception, you will make sure that anyone (you, your colleague, your end users, people who will work on your code after you leave) gets the RuntimeException when calling the functions in a wrong order.

Your Tests for the other class (SomeClass) should not test the implementation details for SomeService. That's what the tests for SomeService is for.
You don't need to test if all your other code calls service functions in the right order: If it were the case, an Exception will be thrown, and your test will fail anyway. So why check for the order of the function calls in SomeClass?

What if you refactor the SomeClass and use a new service that does the work in only one function call? You would have to change your test.. but you didn't change the logic, you just used another service! Changing the Tests should not be necessary.

Again, deprecating ->at(...) is showing design flaws in our code, and it's a very good thing.

[ossinkine]

@artjomsimon I don't want to test SomeService, this is third party class, it's all ok with this. I want to test my code which uses SomeService (which is mocked in tests) calls functionNameA and functionNameB in right order. For example:

class MyService
{
    private SomeService $service;
    public function __construct(SomeService $service)
    {
        $this->service = $service;
    }

    public function doSomething(): void
    {
        $this->functionNameA();
        $this->functionNameB();
    }
}

class MyServiceTest extends TestCase
{
    public function testSomething(): void
    {
        $service = $this->createMock(SomeService::class);
        $myService = new MyService($service);

        $service->expects($this->at(1))->method('functionNameA');
        $service->expects($this->at(2))->method('functionNameB');

        $myService->doSomething();
    }
}

[pwellingelastique]

I do agree that you do not want to test the lines of a method, and that if you change the method, your test would fail. But as @ossinkine states, I do not want to test the other service, but I want to be sure it has been called in a specific order (based on my scenario) or that the method has not been called at all. What if there is a 3 method called right after the if-statement in my example? I would want to be sure that only in case of $input == 2 (or to be more exact, $input === 2) service function B has been called, and not in all the other cases.

Having said that, writing too complex methods, and the difficulty of having these tested, does force you to rewrite them so they are less complex. But sometimes you just can't avoid complex methods where other methods (in other objects) are called in a specific order based on the input. And I do sometimes want to be very sure that a method that for instance sends out an email or sets a very important switch elsewhere, has been called if it has to be art the right moment.

Does my tested method have too much responsibilities? It might, but sometimes you just can't avoid that. And with the at() method, you have an extra tool just to make sure.

The outcome of my test might just be that some things are called in a specific order. That might just be the result of the called function. And when I change the lines of the function, which causes the order to change, when that shouldn't happen, my unit test should fail, just to prevent that. And if it is intentional, then I should also change my test.
So in my opinion, the at() method should not be removed, since it is a handy tool to just to monitor the behaviour.

[Pi-George]

How would something like this be re-written? I'm having some trouble with understanding the alternative syntax that should be replacing usage of at. Many of the mentioned functions aren't in the documentation as far as I can tell.

        $this->workspaceDao->expects($this->at(0))
            ->method('getWorkspace')
            ->with("1")
            ->willReturn($singleWorkspace);

        $this->workspaceDao->expects($this->at(1))
            ->method('getWorkspace')
            ->with("2")
            ->willReturn($singleWorkspace2);

        $this->keywordGroupDao->expects($this->at(0))
            ->method('getAllKeywordsInGroups')
            ->with(["1001", "1002", "1003"], TrackingPeriod::current($singleWorkspace->getTrackingFrequency()))
            ->willReturn([]);

[kdambekalns]

How would something like this be re-written?

The first two can be rewritten like this:

$this->workspaceDao->method('getWorkspace')
    ->withConsecutive(["1"], ["2"])
    ->willReturnOnConsecutiveCalls($singleWorkspace, $singleWorkspace2);

For the last it depends on how important the at(0) really is. Use atLeastOnce(), once(), excatly(…)as needed or simple leaveexpects(…)` out completely:

$this->keywordGroupDao->method('getAllKeywordsInGroups')
    ->with(["1001", "1002", "1003"], TrackingPeriod::current($singleWorkspace->getTrackingFrequency()))
    ->willReturn([]);

See https://github.com/doctrine/annotations/pull/345/files for some examples…

[kdambekalns]

Another example for the (common?) usecase of testing method call order:

$calls = [];
$subject = $this->getMockBuilder(Subject::class)->setMethods(['a', 'b', 'c'])->getMock();
$subject->expects(self::atLeastOnce())->method('a')->willReturnCallback(function () use (&$calls) { $calls[] = 'a'; });
$subject->expects(self::atLeastOnce())->method('b')->willReturnCallback(function () use (&$calls) { $calls[] = 'b'; });
$subject->expects(self::atLeastOnce())->method('c')->willReturnCallback(function () use (&$calls) { $calls[] = 'c'; });

$subject->someMethod();

self::assertEquals(['a', 'b', 'c'], $calls);

[alex-pravdin-sn]

To solve this issue I wrote my own matcher that counts only calls of the specified method, not all calls of the mock. This makes sense because other matchers like "method X expected once" or "method Y expected never" have the same meaning - they relate to the number of calls of the specified method, not to all calls on the mock. The current at behavior contradicts once, never, and others. So adding a matcher that counts only calls of the specific method worked for me for years.

The issue with the at method is multiplied by the absence of the ability to mock protected methods. Mocking separated code blocks could effectively decrease the requirement to count particular calls and endlessly edit big tests on minor changes in complex business logic.

[alex-pravdin-sn]

As a bright example of why a matcher that matches particular hits of the specified method is needed - try to test a class that implements an Iterator in addition to its own methods. Like Elastica\ResultSet. It leads to a nightmare with the standard matchers set especially when the number of testing iterations changes from time to time. But with a matcher that matches particular hits of particular methods - tests become really straight forward and more clear.