Skip to content
This repository

Exception handling fix #88

Closed
wants to merge 1 commit into from

7 participants

Jordi Boggiano Lukas Kahwe Smith Sebastian Bergmann Daniel Convissor Michael M Slusarz Christian Hammers Matthew Caruana Galizia
Jordi Boggiano

See the commit description, this is a really annoying side effect which has led to bad code making it through the tests, and then failing in real conditions.

Jordi Boggiano setExpectedException should not catch exceptions coming from PHPUnit'…
…s error handler

Too often tests are believed to work because they catch an Exception, while they are in fact just catching a PHP warning/notice/error that has been converted to an Exception by PHPUnit
e9a352e
Sebastian Bergmann

Closed by 4ad409a.

Victor Karamzin visor referenced this pull request from a commit December 14, 2010
Sebastian Bergmann Close #88. 54d1cad
Jason Ardell ardell referenced this pull request from a commit in apinstein/phpunit December 14, 2010
Sebastian Bergmann Close #88. 4ad409a
Daniel Convissor

This change is problematic. Now one can't write a simple test case for PHP core functions (like DateTime) that throw plain old Exceptions. Let alone, this change breaks BC, so now existing code out there that did $this->setExpectedException('Exception'); no longer works and we all have to rewrite the tests with try/catch blocks.

Michael M Slusarz

Agreed... this commit was not thought out very well. And it doesn't make any sense in relation to the problem it allegedly solves. When I explictly say I am expecting an Exception to be thrown, I'm expecting just that: an Exception to be thrown. This shouldn't catch an ErrorException, an Extended_Exception, or anything else. So I don't see the issue. (I am guessing PHPUnit is not throwing plain Exceptions when it catches things like syntax/runtime errors.)

Not to mention this is blatantly counterintuitive. And not mentioned anywhere in the documentation.

Sebastian Bergmann

You are beating a dead horse. Expecting the generic Exception class is possible again in the master branch (which will become PHPUnit 3.7). That does not mean, however, that using the generic Exception class in your code is suddenly a good idea.

Christian Hammers

Any chance that this fix of the fix gets into the next 3.6.x release?

Sebastian Bergmann

No.

Matthew Caruana Galizia

@lathspell You could always use the Standard PHP Library Exceptions if you think writing exception classes is overkill. They're all allowed by PHPUnit, even pre-3.7.

Greg Lamb greglamb referenced this pull request from a commit in greglamb/phpunit December 14, 2010
Sebastian Bergmann Close #88. f9466a6
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Dec 13, 2010
Jordi Boggiano setExpectedException should not catch exceptions coming from PHPUnit'…
…s error handler

Too often tests are believed to work because they catch an Exception, while they are in fact just catching a PHP warning/notice/error that has been converted to an Exception by PHPUnit
e9a352e
This page is out of date. Refresh to see the latest.
1  PHPUnit/Framework/TestCase.php
@@ -740,6 +740,7 @@ protected function runTest()
740 740
         catch (Exception $e) {
741 741
             if (!$e instanceof PHPUnit_Framework_IncompleteTest &&
742 742
                 !$e instanceof PHPUnit_Framework_SkippedTest &&
  743
+                !$e instanceof PHPUnit_Framework_Error &&
743 744
                 is_string($this->expectedException) &&
744 745
                 $e instanceof $this->expectedException) {
745 746
                 if (is_string($this->expectedExceptionMessage) &&
15  Tests/Framework/TestCaseTest.php
@@ -56,6 +56,7 @@
56 56
 require_once dirname(dirname(__FILE__)) . DIRECTORY_SEPARATOR . '_files' . DIRECTORY_SEPARATOR . 'Success.php';
57 57
 require_once dirname(dirname(__FILE__)) . DIRECTORY_SEPARATOR . '_files' . DIRECTORY_SEPARATOR . 'ThrowExceptionTestCase.php';
58 58
 require_once dirname(dirname(__FILE__)) . DIRECTORY_SEPARATOR . '_files' . DIRECTORY_SEPARATOR . 'ThrowNoExceptionTestCase.php';
  59
+require_once dirname(dirname(__FILE__)) . DIRECTORY_SEPARATOR . '_files' . DIRECTORY_SEPARATOR . 'TriggerWarningTestCase.php';
59 60
 require_once dirname(dirname(__FILE__)) . DIRECTORY_SEPARATOR . '_files' . DIRECTORY_SEPARATOR . 'WasRun.php';
60 61
 
61 62
 $GLOBALS['a']  = 'a';
@@ -157,6 +158,20 @@ public function testExceptionInTest()
157 158
         $this->assertTrue($test->tearDown);
158 159
     }
159 160
 
  161
+    public function testRealExceptionIsCaughtByExpectedException()
  162
+    {
  163
+        $test   = new TriggerWarningTestCase('testRealExceptionIsCaught');
  164
+        $result = $test->run();
  165
+        $this->assertTrue($result->wasSuccessful(), 'Normal Exceptions should be caught by the setExpectedException method');
  166
+    }
  167
+
  168
+    public function testExceptionFromWarningNotCaughtByExpectedException()
  169
+    {
  170
+        $test   = new TriggerWarningTestCase('testWarningAsExceptionIsNotCaught');
  171
+        $result = $test->run();
  172
+        $this->assertFalse($result->wasSuccessful(), 'A warning as an exception shouldn\'t be caught by the setExpectedException method');
  173
+    }
  174
+
160 175
     public function testExceptionInAssertPostConditions()
161 176
     {
162 177
         $test   = new ExceptionInAssertPostConditionsTest('testSomething');
18  Tests/_files/TriggerWarningTestCase.php
... ...
@@ -0,0 +1,18 @@
  1
+<?php
  2
+class TriggerWarningTestCase extends PHPUnit_Framework_TestCase
  3
+{
  4
+    public function testRealExceptionIsCaught()
  5
+    {
  6
+        $this->setExpectedException('Exception');
  7
+        throw new Exception('', 1);
  8
+    }
  9
+
  10
+    /**
  11
+     * @errorHandler enabled
  12
+     */
  13
+    public function testWarningAsExceptionIsNotCaught()
  14
+    {
  15
+        $this->setExpectedException('Exception');
  16
+        trigger_error("Catch as exception", E_USER_NOTICE);
  17
+    }
  18
+}
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.