Expected exception semantics

[mindplay-dk]

The semantics of expected exception testing could be better.

When you call setExpectedException() is just overwrites an internal test-condition, which means you can't do more than one assertion within a single test.

What's worse though, if you have two statements after a call to setExpectedException(), and you're expecting the first statement to trigger an exception, but it doesn't, and instead the second statement, which shouldn't, triggers the same type of exception - your test comes up green, but may actually be covering up two bugs.

What's missing, is some way to indicate precisely which statement is expected to generate the exception - for example:

/**
 * Check for an expected exception, which must be thrown.
 *
 * @param mixed    $exceptionName
 * @param string   $exceptionMessage
 * @param int      $exceptionCode
 * @param callable $function
 *
 * @void
 */
public function assertException($exceptionName, $exceptionMessage, exceptionCode, $function)
{
    try {
        $function();
    } catch (Exception $e) {
        // ... check type/message/code and record assertion as passed/failed
        return;
    }
    // ... mark assertion as failed (not thrown)
}

Using a closure, you can designate precisely which statement is expected to throw - and, you can test for any number of expected exceptions in the same test body.

[mindplay-dk]

To provide a case example from a real unit test:

$this->setExpectedException(RuntimeException::class, 'unexpected type: array');
$route->url(['id' => array(), 'title' => 'Oops']); // will throw because of type violation
$route->url(['id' => 123, 'title' => 'No problem']); // won't throw

The trouble here, is that the test will pass, whether that second statement does or does not throw the same exception as the first statement. If I were to botch the test:

$this->setExpectedException(RuntimeException::class, 'unexpected type: array');
$route->url(['id' => array(), 'title' => 'Oops']); // will throw because of type violation
$route->url(['id' => array(), 'title' => 'Oops again']); // will also throw

This will also pass.

In other words, it doesn't matter if the second statement throws or not.

Providing a means of designating the code that is expected to throw, should provide the correct semantics for this.

Would you accept a PR?

[mindplay-dk]

Here's my interim solution, adding this to my test-case base class:

    /**
     * @param string $type
     * @param string|null $message
     * @param callable $function
     */
    protected function assertException($type, $message, callable $function)
    {
        $thrown = false;
        $reason = 'no exception thrown';

        try {
            call_user_func($function);
        } catch (Exception $e) {
            if ($e instanceof $type) {
                if ($message === null || strcasecmp($message, $e->getMessage()) === 0) {
                    $thrown = true;
                } else {
                    $reason = "unexpected message: \"{$e->getMessage()}\", expected: \"{$message}\"";
                }
            } else {
                $reason = "unexpected type: " . get_class($e);
            }
        }

        $this->assertTrue($thrown, "Expected exception: {$type}" . ($reason ? " ({$reason})" : ""));
    }

It's not ideal, giving a description like "failed asserting that true is false", and providing no means of giving a descriptive message for the assertion. Looks like adding an assertion "the right way" is quite a bit more involved, and I don't have time for it, so no PR for now, sorry...

[mindplay-dk]

Okay, we picked through the code and came up with this much simpler and more "frameworky" solution, piggybacking on the constraint classes that are already in there.

    /**
     * @param string $type
     * @param string|null $message
     * @param callable $function
     */
    protected function assertException($type, $message, callable $function)
    {
        $exception = null;

        try {
            call_user_func($function);
        } catch (Exception $e) {
            $exception = $e;
        }

        self::assertThat($exception, new PHPUnit_Framework_Constraint_Exception($type));

        if ($message !== null) {
            self::assertThat($exception, new PHPUnit_Framework_Constraint_ExceptionMessage($message));
        }
    }

What would it take to get in a PR? Unit test, documentation, the works?

[sebastianbergmann]

Dear contributor,

let me start by apologizing for not commenting and/or working on the issue you have reported or merging the pull request you have sent sooner.

PHPUnit 5.0 was released today. And today I am closing all open bug reports and pull requests for PHPUnit and its dependencies that I maintain. Please do not interpret the closing of this ticket as an insult or a lack of interest in your problem. I am sorry for any inconvenience this may cause.

If the topic of this ticket is still relevant then please open a new ticket or send a new pull request. If your ticket or pull request is about a defect then please check whether the issue still exists in PHPUnit 4.8 (which will received bug fixes until August 2016). If your ticket or pull request is about a new feature then please port your patch PHPUnit 5.0 before sending a new pull request.

I hope that today's extreme backlog grooming will allow me to respond to bug reports and pull requests in a more timely manner in the future.

Thank you for your understanding,
Sebastian