lithium\test\Unit->expectException not working as expected #280

Closed
clexmond opened this Issue Jan 17, 2012 · 8 comments

3 participants

@clexmond
Contributor

In most (maybe all?) testing frameworks, expecting an exception that doesn't fire should result in a test failure. Lithium's current implementation correctly avoids dumping a stack trace and breaking on an expected exception, but it also fails silently if the expected exception is not fired.

Furthermore, expectException is being used incorrectly in several places throughout the core lithium test cases. One such example is in lithium\tests\cases\action\ControllerTest->testMethodInvocation where the line throwing the exception is followed by several more asserts which will never run (snippet posted, there are actually more):

$postsController = new MockPostsController();
$this->expectException('/Unhandled media type/');
$result = $postsController(null, array('action' => 'index', 'args' => array(true)));

$this->assertTrue(is_a($result, 'lithium\action\Response'));
$this->assertEqual($result->body, '');

$headers = array('Content-type' => 'text/html; charset=UTF-8');
$this->assertEqual($result->headers, $headers);

If the test cases are written as desired, adding a lithium\test\Unit->fail method and checking for exceptions in try catch blocks would be a good replacement. Otherwise, expectException needs to be treated as its own assert and explicitly fail if the expected exception isn't thrown. Furthermore, the tests would need to be rewritten to make sure no asserts occur after expectExceptions as they will clearly never run.

@jperras
Member
jperras commented Jan 17, 2012

tests would need to be rewritten to make sure no asserts occur after expectExceptions as they will clearly never run.

Agreed. 👍

@clexmond
Contributor

After thinking some about this issue there are a few different options I'd like to get some input on from whomever would be responsible for merging this back to trunk.

  1. Unit->expectException is called, followed by the code that should throw the exception. The test method would need to end at this point. Unit->expectException would add a failed test if the exception doesn't match. If no exception is thrown Unit->_reportException would add the thrown exception to a Unit property and Unit->_runTestMethod would check that the expected and actual exceptions match. If they don't, a failed test would be triggered. Alternatively, a Unit->fail method could be added just before the end of the test method to handle that logic less magically and expectException would just handle the case in which the incorrect exception was thrown.

  2. Replace expectException altogether with try / catch blocks. A Unit->fail method would also be useful here to place inside the try block after the code that should throw an exception. Checking the exception type / message would be done in the catch.

  3. Implement a Unit->assertException which will take the expected exception and a closure as an argument. The method would execute the closure within a try catch internally and basically handle all the try / catch logic in (2).

If (3) works it's personally my favorite as it's the most similar to the method used for every other type of test.

@jperras
Member
jperras commented Jan 18, 2012

I don't see any real problem in implementing no.1 as well as adding a test\Unit::fail() method; this would allow people to write more explicit tests with try/catch blocks as you described. Having that test\Unit::fail() method would probably come in handy in the future as well.

I see what you mean in no.3, and I think it could work. Perhaps if we name it something more along the lines of throwsException(Exception $e, $callable) ?

@clexmond
Contributor

No. 3 definitely seems the most lithiumy (if that's a word) making use of closures. I'll play around a little with it, I just want to make sure it doesn't run into any scope issues and doesn't end up creating a lot more code.

@clexmond clexmond added a commit to clexmond/lithium that referenced this issue Jan 19, 2012
@clexmond clexmond Breaking test for issue #280. b3d1f96
@clexmond
Contributor

The test case doesn't address the issue of having asserts come after a expectException but I don' think it should as that's really improper use. Working an an assertException method which I'm really liking so far.

@clexmond clexmond added a commit to clexmond/lithium that referenced this issue Jan 19, 2012
@clexmond clexmond Added a fail method and unit tests, issue #280. e8c5a5e
@clexmond
Contributor

If we go with these changes, expectException will need to be replaced throughout something like 40+ files and then cleaned up in test/Unit. Thoughts on having that be a separate issue?

@jperras
Member
jperras commented Jan 19, 2012

I'll review the code today, and try to figure out the best course of action. If you have any ideas/suggestions, please let me know ;-).

@clexmond
Contributor

It might be a good idea to leave expectException and the failing test in for the next release and just replace the instances of expectException in test cases. Make a comment in the change notes that expectException will be deprecated in the following release and to move to using assertException.

I still think actually replacing expectException in test cases should be done in a new issue as it will be a good bit of work combing through and figuring out in each instance what the original author wanted to happen in each case.

@nateabele nateabele closed this in 51e6139 Feb 11, 2012
@davidpersson davidpersson added a commit to davidpersson/lithium that referenced this issue Sep 29, 2014
@davidpersson davidpersson Deprecating expectException in favor of assertException. Refs #280 and
…#327.

Removing tests that can no longer be run. But leaving other tests
in place.

assertException can be used much more safely, one can't forget that
code after a thrown exception expected with expectException isn't
run.
2ec829f
@davidpersson davidpersson added a commit that referenced this issue Sep 29, 2014
@davidpersson davidpersson Deprecating expectException in favor of assertException. Refs #280 and
…#327.

Removing tests that can no longer be run. But leaving other tests
in place.

assertException can be used much more safely, one can't forget that
code after a thrown exception expected with expectException isn't
run.
a22e68d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment