Phpunit: 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, and it's how we expect it to be done. 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.