Testing an exception is not thrown #171

Closed
xmontero opened this Issue Jan 17, 2014 · 27 comments

Comments

Projects
None yet
9 participants

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 commented Mar 27, 2014

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

Collaborator

ThomasWeinert commented Mar 27, 2014

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 commented Mar 27, 2014

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 :-)

I am with @ThomasWeinert on this one.

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.

kimegede referenced this issue in cakephp/cakephp Aug 24, 2014

Merged

Cache test update #4374

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

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);
  }

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.

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.

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 commented Dec 19, 2014

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.

Collaborator

whatthejeff commented Dec 19, 2014

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.

@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).

Collaborator

whatthejeff commented Dec 19, 2014

That's very much similar to the solution I wrote.

Yes, but I have a preference against using no-op assertions like assertTrue(true).

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.

Every test that passes implicitly verifies that no unexpected exceptions were raised. It's largely redundant to do this manually because PHPUnit is already doing it for you by default.

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).

Assert methods (and by extension, stand-alone argument validation methods) are really the only time I would manually verify that an exception isn't raised and that's only because they have no valuable side effect or return value.

Every test that passes implicitly verifies that no unexpected exceptions were raised. It's largely redundant to do this manually because PHPUnit is already doing it for you by default.

This is not true when PHPUnit is run in strict mode, which is the problem we are having. While the test does indeed pass, PHPUnit complains about it being risky due to a lack of assertions.

Maybe we should just execute and then add a dummy test, which we can decorate for readibility, like for example:

$sut->runSomethingThatDoesNotReturnValuesAndShouldNotRaiseException( $riskyValue );
$this->assertTrue( $riskyAlertMutedByAssertingNoExceptionWasRaised = true );

which is a bit silly, but will mute the "risky" warning on one side, while failing if the method raises an exception. The assignation (not comparison) inside is just to explain why we test assertTrue( true );

We are just "muting" because we "know what we are doing".

I am not sure if this "muting" of the "risky" warning makes any sense. Does it?

Collaborator

whatthejeff commented Dec 19, 2014

@MatthewHerbst That excerpt was about code with meaningful return values / side effects.

@xmontero What if there was just a way of adding a comment to suppress risky tests? For example:

/**
* @suppressWarning Risky
*/
function testRisky() {
    runSomethingThatDoesNotReturnValuesAndShouldNotRaiseException($validInput)
}

I like it! I can clearly see it in action!

In my original question, the testValidArgumentDoesNotThrowException with data provider of [2, 10, 48] would have that comment, and problem solved!

Even, the same way we can comment "expected exception" or just call $this->setExpectedException( 'whateverException' );

we maybe, should add a $this->supressWarning( 'Risky' ); if one's style is more calling meethods than adding annotations.

Maybe instead a 'Risky' string, a set of constants of the warnings that can be muted.

If everyone feels like this makes sense and has chances to be merged, I would love to fork and contribute, but I don't know the PHPUnit code. If someone helps me, I'd be more than happy to contribute in the code.

I am not a fan of annotations such as @suppressWarning.

@sebastianbergmann Would you mind please giving some context as to why? Or if you still believe no change is needed for this issue?

I also prefer code over annotations.

@sebastianbergmann This is why I suggested to add the method $this->supressWarning( 'Risky' ); the same way we can set the expected exception via method call, or we can mark a test incomplete with $this->markTestIncomplete();

Makes sense for you?

Let me rephrase then: I am not a fan of suppressing warnings etc.

@sebastianbergmann What would you suggest if we want to test that call to a method that does not return anything does not throw any exception?

@sebastianbergmann To add to @xmontero's question, specifically, if we call that test method in Strict mode.

J7mbo commented Jan 7, 2015

@sebastianbergmann Warning suppressions can be perfectly valid, and often useful, when the dev using them knows what they're doing and why. Turning failed file_get_contents() calls into custom exceptions for a clean object api, for example.

fzaninotto referenced this issue in fzaninotto/Faker Jul 12, 2015

Merged

Update Generator.php #632

@enov enov added a commit to kohana/core that referenced this issue Oct 9, 2015

@enov enov Test Kohana_Exception::log to fail silently when Kohana::log is unass…
…igned

If Kohana_Exception::log fails with an exception, the test would not pass
sebastianbergmann/phpunit-documentation#171
687be6b

@enov enov added a commit to kohana/core that referenced this issue Oct 9, 2015

@enov enov Test Kohana_Exception::log fails silently when Kohana::log is NULL
If Kohana_Exception::log fails with an exception, the test would not pass
sebastianbergmann/phpunit-documentation#171
9fc3062

@enov enov added a commit to kohana/core that referenced this issue Dec 10, 2015

@enov enov Test Kohana_Exception::log fails silently when Kohana::log is NULL
If Kohana_Exception::log fails with an exception, the test would not pass
sebastianbergmann/phpunit-documentation#171
ed9a2da

jvillafanez referenced this issue in owncloud/core Jan 17, 2017

Merged

[stable9.1] Skip null groups in group manager #26871

4 of 9 tasks complete

Xerkus commented May 27, 2017

I found valid use case for testing that exception is not thrown:
I am testing that object is properly initialized by calling getters with declared return types. If object was not initialized fully, getter will throw type error.

Currently, i settled for

$tested->getter(); // something that must be set by constructor
self::assertTrue(true, 'Return type ensures this assertion is never reached on failure');
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment