Prefer assertException over expectException

[jchook]

Q	A
PHPUnit version	6.5.7
PHP version	7.1.16
Installation Method	PHAR

Goal: to test exceptions.

After reading through SO I found #1798, which was closed in a routine issue clean-up. As suggested, I am re-opening this issue.

Please forgive any ignorance I have about the design decisions of the expectException family of methods, but I still find this particular suggestion to be a good direction on having a callback-based exception testing pattern.

/**
 * @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));
    }
}

While I realize this example is far from a complete replacement, I think we could build from this pattern to give users more control over exception testing, and a more familiar assertion pattern that doesn't require calling $this->fail(), etc.

Here is a theoretical usage example (of a currently non-existent assertException):

$this->assertException(MyException::class, 'doStuff', 
  function($exception, $phpUnit) {
    $phpUnit->assertEquals('Some message', $exception->getMessage());
    $phpUnit->assertEquals(123, $exception->getCode());
    $phpUnit->assertEquals('custom value', $exception->getCustomThing());
  }
);

[sebastianbergmann]

I wrote everything I have to say on the subject in https://thephp.cc/news/2016/02/questioning-phpunit-best-practices.

IMO, expectException() and friends are all that is needed.

[jchook]

Thanks for the link.

[jchook]

As you mention in your post, the suggestion has been brought up, dismissed, and implemented as a plugin by others.

Quoting your link:

The argument that is made in favor of this approach is that it allows checks for multiple exceptions within a single test. In my opinion, though, this is an argument against it. A unit test should be fine-grained and only test a single aspect of one object. Why would you need to check for different exceptions in a single test?

Here are a few additional arguments surrounding the case:

1. Currently we are forced to use the confessedly less desired "return or fail", etc techniques to test any other aspects of an exception beyond the message, code, and class.

2. The assert syntax is ubiquitous in PHPUnit, except exceptions. This is a clear and isolated break in expectations for the syntax.

3. It's how Facebook's Jest, Chai, Codeception, NUNIT, and many others do it. Alternatives such as annotations and calling $this->fail() are seen as less optimal by the community.

4. IMO, testing a single "aspect" of one object isn't always 1-to-1 with testing a single exception of an object. Forcing users to separate tests by exception is sometimes overkill, especially if it requires @depends, etc.

[Pictor13]

Pardon me if I take this up again.

I especially support the point #​4.

When the same aspect is tested, it should be possible to test it in the same test method. Splitting helps specificity and readability but that should be an optional benefit, not a requirement.
What @jchook quoted is totally true, but is just a practice, a recommendation, to prevent inexperienced developers from polluting a single test method with tens of unrelated-asserts. As long as the target concern is the same then it should be possible to group asserts/statements.

I see and acknowledge the reasoning behind it about enforcing best practices, but I see the current limit 1exceptedException-1method being driven mostly by technical implementation (yet, I dunno how much work & hassle is needed to implement an assertException).
Removing that limit wouldn't break any good practice, and would provide developers an improvement, putting on them the responsibility of their own technical choices.

Also I think also point #​2 has its reason to be.

The expected syntax has arbitrary changed just for exceptions, that are just classes in the end; while at the moment they are handled like something requiring a special treatment; requiring even a different syntax (EDIT: although I see that there are other syntax exceptions, like expectOutputString).

[jchook]

@Pictor13 If you're interested I implemented $this->assertThrows() as a PHP trait.

I'd be happy to submit a PR if @sebastianbergmann would like.

Also @keradus interested to hear what you have to add.

[gingerCodeNinja]

So, I have some code that throws FooException($message, $bar). I want to test the code that throws this exception, that it passes in the correct $bar, but I can't do that with expectException/expectExceptionMessage.

I was hoping on having a callback that can test FooException ->getBar() matches my expectation, like I can do for assertions. Seems silly to have to use a plugin for something that is so core to PHP.

Since PHPUnit does not support this right now, and I can't easily use a plugin (enterprise software etc), I guess I will have to not use expectException, and wrap the method under test with try/catch in my actual test class, catch the exception and run my assertions.

[eithed]

I can't use expectException when there's a cleanup step involved and I have to remember that everything after line that fulfils expectException will not execute, which is counter-intuitive. As such this:

public function test_duplicate_data_is_not_saved()
{
    $this->expectException(DuplicateEntryException::class);
    $this->service->insert('foo');
    
    // throws DuplicateEntryException
    $this->service->insert('foo');

    // clear data
    $this->service->delete('foo');
}

will leave me with foo entry, even though test succeeds and I think that delete line fires (because test passes). I've to do following for it to work as expected:

public function test_duplicate_data_is_not_saved()
{
    $this->service->insert('foo');
    
    try {
        $this->service->insert('foo');
    } catch (\Exception $e) {
        $this->assertInstanceOf(DuplicateEntryException::class, $e);
    } finally {
        $this->service->delete('foo');
    }
}

One needs to remember as well that tearDown won't trigger in the first scenario.

[Pictor13]

Why would tearDown not be triggered? You sure? (note: I didn't try it)
That's the correct method to use to cleanup or reset your service.

Also, your alternative is correct:try/catch/finally is the only way I would expect any code to be still executed after an exception has been thrown.

[renepupil]

I wrote everything I have to say on the subject in https://thephp.cc/news/2016/02/questioning-phpunit-best-practices.

@sebastianbergmann This link is dead.

[sebastianbergmann]

https://thephp.cc/articles/questioning-phpunit-best-practices

[renepupil]

I would like to invite you all to question best practices like this. Nothing can be set in stone if we want to evolve PHP and its ecosystem of tools, frameworks, and libraries.

@sebastianbergmann Maybe it wasn't made clear from previous comments, I would like to challenge the approach: I think most of us don't want to test multiple exceptions in a test case, but check attributes of the exceptions, or events that should happen in case of the exception.

Example: I expect an API log on error:

        $exception = null;
        try {
            $now = new DateTimeImmutable('now');

            $this->authenticate(clientSecret: 'invalid');
        } catch (Throwable $e) {
            $exception = $e;
        }

        $this->assertInstanceOf(ClientException::class, $exception);
        $this->assertLogEntry($now, false, 'Client authentication failed');

If you think this code is against best practices, can you suggest how to test such a condition in a "good" way?

[mfn]

For integration testing of APIs I came to the exact same conclusion. There's often other things to assert after an exception, next to the exception, and for API stuff integration tests are really important. So much can happen in different cases, pure unit won't cut it.

TL;DR: for this case I've been using something like this to great success for years:

    public static function assertExceptionCaptured(
        callable $fn,
        string $expectedExceptionClass,
        string $expectedExceptionMessage = null
    ): Throwable {
        $exception = null;

        try {
            $fn();
        } catch (Throwable $exception) {
            // Deliberately empty
        }

        if (null === $exception) {
            throw new ExpectationFailedException(
                "Expected to catch exception of type $expectedExceptionClass but none was caught"
            );
        }

        $exceptionClass = $exception::class;

        $msg = "Expected to catch $expectedExceptionClass but caught "
            . $exception::class . ' instead: '
            . $exception->getMessage()
            . "\n"
            . $exception->getTraceAsString()
            . "\n";
        self::assertSame($expectedExceptionClass, $exceptionClass, $msg);

        if (null !== $expectedExceptionMessage) {
            self::assertSame($expectedExceptionMessage, $exception->getMessage());
        }

        return $exception;
    }