Testing an exception is not thrown

[xmontero]

Here, http://phpunit.de/manual/current/en/writing-tests-for-phpunit.html it says that you can easily test an exception is thrown. But maybe you want to test that an exception is not thrown.

For example, a funcion accepts a number x and must be even. If it is odd raises an InvalidArgumentException.

You might want a a testInvalidArgumentThrowsException with dataprovider [111, 3, 77] as well as testValidArgumentDoesNotThrowException with data provider of [2, 10, 48]

Despite I can test exception with the /** xxx */ comment, I try not thrown via a try/catch.

If there is any way to test "not thrown" I suggest to include in that documentation page. If there is no cleaner way, I also suggest to include it in that documentation.

[J7mbo]

Any update on this, it would make a nice little addition for readability at least.

[ThomasWeinert]

I don't see the need. You should test that the method works, testing the logic that is executed with valid arguments and that exceptions are thrown in specific cases.

[J7mbo]

My current use case may need a different approach, but the reason I searched for this capability and found this github post is because: I am writing custom assertions using Behat/Mink + PHPUnit for web acceptance testing. I am wrapping WebAssert provided by mink, because Mink throws exceptions, when I want nice test failures with messages instead.

My assertElementExists() wraps elementExists(), which throws an exception if it fails and returns nothing if it doesn't, and does the following:

try
{
    $this->assertNull($webAssert->elementExists('xpath', $element));
}
catch (ElementNotFoundException $e) // The exception thrown by WebAssert::elementExists
{
    $this->fail(sprintf("Failed asserting that element: %s does exist on page: %s",
        $element, $this->session->getCurrentUrl()
    ));
}

Just thinking, it would be totally awesome if I could (instead of assertNull), do:

/**
 * @param The exception we don't want
 * @param optional The message in case the exception is thrown
 */
$this->assertExceptionNotThrown("ElementNotFoundException", sprintf(
    "Failed asserting that element: %s does exist on page: %s",
    $element, $this->session->getCurrentUrl()
));

I'm not just asking for this out of the blue, that's my use case and I would love this capability. However if there's a better way of doing this I'm always open to suggestions :-)

[sebastianbergmann]

I am with @ThomasWeinert on this one.

[aMoniker]

This would actually be nice for running tests in strict mode that succeed if they do not throw exceptions. Otherwise the test is marked as 'risky' since it doesn't have any assertions.

[mariusbalcytis]

I definately see the need for this type of assertions in some cases. If the method's solely purpose is to validate something and throw exception if it's not valid, we must also test the cases with valid values. Otherwise, just throwing an exception without even checking the value would pass all our tests.

I use this on the end of the test case to assert no exception is thrown. This works as expected with strict mode, too.

$this->addToAssertionCount(1);  // does not throw an exception

[MatthewHerbst]

I agree with @mariusbalcytis here. I just ran into that exact problem, where I have a Validate class that does nothing but throw InvalidArgumentExceptions. It has no return values from the functions within it. The solution I came up with is:

  /**
  * @covers ../data/objects/Validate::isString
  */
  public function testIsStringNoExceptionArgumentValid() {
    try {
      Validate::isString("I am a string.");
    } catch (InvalidArgumentException $notExpected) {
      $this->fail();
    }

    $this->assertTrue(TRUE);
  }

[xmontero]

Ok, so, guys, we are mixing 3 unconnected things here:

One debate is: Q1) should a method to test an exception is NOT thrown exist (yes/no)?

Another debate is: in the case it does not exist, Q2) shoud an explanation exist in the documentation, such as explaining that there is NOT any method for that, so if you want to test exception-not-thrown you MUST implement your own logic with ifs or try-catches (yes/no)?

And finally, a third debate, arised during this issue is: Q3) is testing "exception not thrown" an anti-pattern and we SHOULD stick ONLY to testing return values and positive exceptions (yes/no)?

As for Q1, in my initial text, I never opened this issue to request such a feature. My text "If there is any way to test "not thrown" I suggest to include in that documentation page. If there is no cleaner way, I also suggest to include it in that documentation." With that text, I just wanted to say: "if it already exists, just mention it - if it does not exist, please make it explicit that it does not exist".

As for Q2, I agree that if Sebastian thinks there is no need for such a function, then it is not needed. He must know much bettern than me why. And I don't want to enter in such debate as I don't have the knowledge to argue in favour of the oposite of what he says. But still... I strongly suggest (reopen issue?) saying something in the documentation "don't look for a function to ensure a funcion does NOT throw an exception because it does not exist" - seems that I'm not the only one looking for it.

As for Q3, in the case you guys think that seeking for this asserting is an anti-pattern, I ALSO suggest to state that in the documentation: "Don't look for it because it does not exist AND you SHOULD NOT try to overcome this lack, as if you do it means you are not testing well".

So I strongly suggest to modify the documentation, so it tells people like me what's up, stopping us from (unsuccessfully) looking and looking and looking for something.

[MatthewHerbst]

I agree with what you are saying. However, I do think there should be a formal way of proving that a function that has no return, and might throw an exception if it fails, works as intended.

[xmontero]

If it is not an anti-pattern, there should of course be that function.

If it is an anti-pattern, we should understand why it is an anti-pattern.

At this moment, my personal view agrees with you: I don't see an anti-pattern and think it should be the function.

Nevertheless, I have been in unit-testing for only one year, so I realize maybe my scope and knowledge is still limited.

[J7mbo]

It's not an anti pattern. I wrote this, and then realised myself and @MatthewHerbst are doing the same thing. Imagine a Validator interface, and a ValidatorAggregate that contains multiple Validators.

Calling ValidatorAggregate::validate($data) loops through all of the Validator objects and calls validate($data) on them, one-by-one.

The responsibility of each validator is to throw a ValidationException if something is invalid with a reason. The validators doesn't return anything - their job is to validate. Note: we are not getting into the discussion about what one considers "exceptional" or not.

If a set of objects only throw exceptions, I'd like to be able to know one wasn't thrown. My objects usually don't have multiple return value types and have a clear, simple object API.

[whatthejeff]

If it is an anti-pattern, we should understand why it is an anti-pattern.

Generally speaking, negative assertions can be problematic because they aren't very specific. You usually want a test that will only pass if a very specific thing happens. When you reverse that structure, you likely increase the chances for false positives.

The following is normally sufficient for a method that is expected to return null:

public function testElementExists($element) {
    $this->assertNull($webAssert->elementExists('xpath', $element));
}

If any exception is thrown, the test will fail as expected.

Sometimes using try/catch does make the intent more explicit, though. I know it's a little unconventional, but I've used the following style when testing some assertion frameworks (like PHPUnit):

/**
 * @covers       ::assertEquals
 * @dataProvider assertEqualsSucceedsProvider
 */
public function testAssertEqualsSucceeds($expected, $actual, $delta = 0.0, $canonicalize = false)
{
    $exception = null;
    try {
        $this->comparator->assertEquals($expected, $actual, $delta, $canonicalize);
    } catch (ComparisonFailure $exception) {}

    $this->assertNull($exception, 'Unexpected ComparisonFailure');
}

It's a bit more code than the previous example, but it gives the casual reader a better idea of how the library works and what we are really testing. This style of try/catch may seem a little awkward, but I like to have setup code at the top and assertions at the bottom. Otherwise the tests are a little harder to parse at a glance, especially when assertions are nested inside of branching logic.

I don't really see much value in adding specialized assertions for these types of tests, though. It's only a few more keystrokes and it's arguably only applicable in a handful of situations.

[MatthewHerbst]

@J7mbo Yes, we are doing the exact same thing in regards to the Validator :)

@whatthejeff That's very much similar to the solution I wrote. However, I feel as though we shouldn't have to write a try/catch or prove that a function had no return, to prove that an exception was not thrown.

It's only a few more keystrokes but it would make test legibility much greater I feel. I actually think there are many situations where this could occur, but that people often simply skimp on many of them (such as argument validation).