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 #4291 (comment) 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();
    }
}

[NamelessCoder]

I see that this is closed, but at the same time, I, like Alex Pravdin would like to know the best method for replacing at, as we're in the process of updating our code.

I believe that withConsecutive and willReturnOnConsecutiveCalls is the solution to aim for. The former should be called like $subject->method($method)->withConsecutive($firstArgumentsArray, $secondArgumentsArray) ; and the latter should be called like $subject->method($method)->willReturnOnConsecutiveCalls($firstReturn, $secondReturn); and so on. Other options for returns include willReturnMap and willReturnCallback which let you specify either an input-arguments-to-return-value map, or a callback function that takes the input argument(s) and returns something.

[GlennBags]

I see that this is closed, but at the same time, I, like Alex Pravdin would like to know the best method for replacing at, as we're in the process of updating our code.

I believe that withConsecutive and willReturnOnConsecutiveCalls is the solution to aim for. The former should be called like $subject->method($method)->withConsecutive($firstArgumentsArray, $secondArgumentsArray) ; and the latter should be called like $subject->method($method)->willReturnOnConsecutiveCalls($firstReturn, $secondReturn); and so on. Other options for returns include willReturnMap and willReturnCallback which let you specify either an input-arguments-to-return-value map, or a callback function that takes the input argument(s) and returns something.

OK, thanks for the feedback

[alex-pravdin-sn]

Any estimations of when withConsecutive and willReturnOnConsecutiveCalls methods will be documented?

[kdambekalns]

Any estimations of when withConsecutive and willReturnOnConsecutiveCalls methods will be documented?

The first has an example at https://phpunit.readthedocs.io/en/9.5/test-doubles.html?highlight=withConsecutive#mock-objects and the latter behaves like willReturn() (see https://phpunit.readthedocs.io/en/9.5/test-doubles.html?highlight=willReturn#stubs) but returning different values per call (as given.)

But yeah, documentation is sketchy in places, especially when digging deeper… 😬

[garethellis36]

I have a legacy project with more than 500 uses of the at() matcher. The tests are indeed brittle but the code under test is pretty stable (bug fixes only). The tests have been useful for detecting regressions with PHP upgrades. The next upgrade is 8.1, which requires PHPUnit 9, so I'm running up against this issue.

If I copy the phpunit matcher class and add an alias for at() in our base test class, am I likely to cause anything else to break? Doing 500 updates to replace the at() matcher by hand is rather daunting!