Phpunit: expectExceptionPrevious()

[marcospassos]

| Q | A
| --------------------| ---------------
| PHPUnit version | 8.1
| PHP version | 7.x
| Installation Method | Composer / PHAR

I want to propose introducing an assertion to verify the cause of an exception, something like the JUnit'sexpectCause method (see example).

public function testExceptionCause() : void
{
    $this->expectException(OperationFailedException::class);
    $this->expectExceptionCause(\OverflowException::class);

    SomeOperationAction::execute();
}

The current workaround involves a lot of boilerplate code:

public function testExceptionCause() : void
{
    $this->expectException(OperationFailedException::class);

    try {
        SomeOperationAction::execute();
    } catch (OperationFailedException $exception) {
        self::assertInstanceOf(\OverflowException::class, $exception->getPrevious());

        throw $exception;
    }

    SomeOperationAction::execute();
}

I'd be glad to provide a PR if welcome.

[sebastianbergmann]

Do I understand correctly that expectExceptionPrevious() would basically perform just an instanceof check on the return value of getPrevious()? Or do we need to walk the chain all the way up until getPrevious() returns null?

[marcospassos]

Do I understand correctly that expectExceptionCause() would basically perform just an instanceof check on the return value of getPrevious()?

It works in the exact same way as expectException, but in regard to the previous exception (getPrevious()). Basically, we will have a new ExceptionCauseConstraint similar to the Exception constraint.

[marcospassos]

Please let me know if you are ok the new constraint and naming so I'll move forward with it.

[sebastianbergmann]

I think expectPreviousException() makes sense, but expectExceptionPrevious() would be consistent with the existing methods. I think the constraint should be named PreviousException or ExceptionPrevious. Not sure which one is better, but, of course, method name and class name should be consistent ;-)

Feel free to send a pull request. Thanks!

[remorhaz]

If we introduce expectPreviousException(), we will need expectPreviousExceptionCode/Message/etc() as well, then someone wants to assert previous of previous, and so on. I think it's better to provide tool to test any exception in chain - just like we can expect any method call in mock object. Something like this:

self::expectChainedException(self::at(1))
    ->isInstanceOf(\OverflowException::class)
    ->hasMessage('Error description')
    ->hasCode(1);

[sebastianbergmann]

In that case I am against adding expectExceptionPrevious(). I do not see a use case for it to begin with, to be honest.

[remorhaz]

In our code we often have a need for testing a previous exception - it's common case of throwing a new exception in try/catch block, and we need to test if the original exception was passed to new exception constructor. In unit tests we will probably never need to look back for more than one position in chain, so my proposal is a bit of overengineering.
But there is a real need to assert the nearest chained exception. Another solution could be just passing the exception to the callback, thus letting the user to assert whatever he wants:

$callback = function (\Throwable $e): bool {
    return $e->getPrevious() instanceof \OverflowException;
};
self::expectExceptionCallback($callback)

I think this solution is even better becase it allows to simply assert the exact instance of chained exception - it's quite useful when the exception was thrown in mocked method via willThrow.

[sebastianbergmann]

This is not a common case that must be supported in a convenient way by PHPUnit. Simply use try/catch in your test code directly without using a feature from PHPUnit and assert what you want to assert on the exception object.

[remorhaz]

You're the master, but I think we should avoid using try/catch in tests because if broken code fails to throw exception at all then we just don't get into corresponding catch block and undergone the risk of false-positive.

[torzak]

Please, can this be reconsidered ?

We all agree that throwing a new exception that hosts the previous one is common practice,
this is why \Exception::__construct signature look like that:

([ string $message = "" [, int $code = 0 [, Throwable $previous = NULL ]]] )
````

This is also a common practice to check in a single unit test that an exception is thrown with the excepted type and arguments:

- interface/class -> ```expectException()```
- message -> ```expectExceptionMessage()```
- code -> ```expectExceptionCode()```
- previous -> **?**

Does it really make sense to have all the PHPUnit methods needed to check the arguments passed to
an exception, EXCEPTED ONE ...  ? ;p


### Concrete code example

```php
class Client {  

    //...
    //...

    /**
    * Sends a request to the XXXXXXX API server.
    *
    * @throws NetworkError on any network failures.
    * @throws ErrorResponse or child class when API server returns an error response. 
    * @see Readme.md for exceptions tree.
    */
    public function get(
        string $target,
        string $method,
        array $params = []
    ): Response {    
        try {
            $response = $this->httpClient->get($this->getApiEndPoint(), [
                'query' => $this->buildQueryString($target, $method, $params),
                'timeout' => $this->timeout,
                'on_stats' => function (TransferStats $stats) {
                    $this->logger->debug(__CLASS__, [$stats]);
                },
            ]);
        } catch (GuzzleException $e) {
            throw new NetworkError($e->getMessage(), $e->getCode(), $e);
        }

        return $this->responseFactory->create((string) $response->getBody());
    }
}
````

### risky unit test

```php

/**
 * @testdox risk of false positive
 */
public function test_throw_networkError_risky()
{
    $httpClient = $this->getMockBuilder(\GuzzleHttp\Client::class)->addMethods(['get'])->getMock();
    $httpClient->expects($this->once(1))->method('get')->willThrowException(
        new \GuzzleHttp\Exception\RequestException(
            'I\'m unit tested !',
            new \GuzzleHttp\Psr7\Request('GET', 'quux'))
    );

    $client = new Client(new Credentials('foo','bar'), $httpClient);

    // risky: False positive if the code doesn't throws
    try{
        $client->get('baz','qux');
    }
    catch (\Exception $e) {
        $this->assertInstanceOf(\GuzzleHttp\Exception\RequestException::class, $e->getPrevious());    
        $this->assertInstanceOf(Exception\NetworkError::class, $e);
        $this->assertStringContainsString('I\'m unit tested !', $e->getMessage());
    }
}

Ugly unit test

/**
* @testdox Ugly
*/
public function test_throw_networkError_ugly()
{
    $httpClient = $this->getMockBuilder(\GuzzleHttp\Client::class)->addMethods(['get'])->getMock();
    $httpClient->expects($this->once(1))->method('get')->willThrowException(
        new \GuzzleHttp\Exception\RequestException(
            'I\'m unit tested !',
            new \GuzzleHttp\Psr7\Request('GET', 'quux'))
    );

    $client = new Client(new Credentials('foo','bar'), $httpClient);

    $this->expectException(NetworkError::class);
    $this->expectExceptionMessage('I\'m unit tested !');

    //@see https://github.com/sebastianbergmann/phpunit/issues/3537 
    //to understand what this try block does here!

    try{ // <- ugly
        $client->get('baz','qux');
    }
    catch (\Exception $e) {
        $this->assertInstanceOf(\GuzzleHttp\Exception\RequestException::class, $e->getPrevious());    

        throw $e; // <- ugly
    }
}

lovable unit test

    /**
    * @testdox throws networkError::class on any network failure.
    */
    public function test_Pleaseeeeeeeeeeeeeee()
    {
        $httpClient = $this->getMockBuilder(\GuzzleHttp\Client::class)->addMethods(['get'])->getMock();
        $httpClient->expects($this->once(1))->method('get')->willThrowException(
            new \GuzzleHttp\Exception\RequestException(
                'I\'m unit tested !',
                new \GuzzleHttp\Psr7\Request('GET', 'quux'))
        );

        $client = new Client(new Credentials('foo','bar'), $httpClient);

        $this->expectException(NetworkError::class);
        $this->expectExceptionMessage('I\'m unit tested !');
        $this->expectExceptionPrevious(\GuzzleHttp\Exception\RequestException::class);

        $client->get('baz','qux');
    }
}

Hope this changes your mind ...

Anyway I take this opportunity to thank you for all that you do for the community.