From 60e949cbbf06c46cb92cb8dc026588d10a7b8f35 Mon Sep 17 00:00:00 2001 From: sun Date: Fri, 25 Jul 2014 00:25:43 +0200 Subject: [PATCH 1/4] Added regression test for #1351. --- tests/Regression/GitHub/1351.phpt | 38 ++++++++++++++++++ .../GitHub/1351/ChildProcessClass1351.php | 4 ++ .../Regression/GitHub/1351/Issue1351Test.php | 39 +++++++++++++++++++ 3 files changed, 81 insertions(+) create mode 100644 tests/Regression/GitHub/1351.phpt create mode 100644 tests/Regression/GitHub/1351/ChildProcessClass1351.php create mode 100644 tests/Regression/GitHub/1351/Issue1351Test.php diff --git a/tests/Regression/GitHub/1351.phpt b/tests/Regression/GitHub/1351.phpt new file mode 100644 index 00000000000..fe95d3ad946 --- /dev/null +++ b/tests/Regression/GitHub/1351.phpt @@ -0,0 +1,38 @@ +--TEST-- +GH-1351: Test result does not serialize test class in process isolation +--FILE-- + +--EXPECTF-- +PHPUnit %s by Sebastian Bergmann. + +F.E. + +Time: %s, Memory: %sMb + +There was 1 error: + +1) Issue1351Test::testExceptionPre +RuntimeException: Expected rethrown exception. +%A +Caused by +LogicException: Expected exception. +%A + +-- + +There was 1 failure: + +1) Issue1351Test::testFailurePre +Expected failure. +%A +FAILURES! +Tests: 4, Assertions: 5, Failures: 1, Errors: 1. \ No newline at end of file diff --git a/tests/Regression/GitHub/1351/ChildProcessClass1351.php b/tests/Regression/GitHub/1351/ChildProcessClass1351.php new file mode 100644 index 00000000000..24c05376667 --- /dev/null +++ b/tests/Regression/GitHub/1351/ChildProcessClass1351.php @@ -0,0 +1,4 @@ +instance = new ChildProcessClass1351(); + $this->assertFalse(TRUE, 'Expected failure.'); + } + + public function testFailurePost() + { + $this->assertNull($this->instance); + $this->assertFalse(class_exists('ChildProcessClass1351', false), 'ChildProcessClass1351 is not loaded.'); + } + + /** + * @runInSeparateProcess + */ + public function testExceptionPre() + { + $this->instance = new ChildProcessClass1351(); + try { + throw new LogicException('Expected exception.'); + } catch (LogicException $e) { + throw new RuntimeException('Expected rethrown exception.', 0, $e); + } + } + + public function testExceptionPost() + { + $this->assertNull($this->instance); + $this->assertFalse(class_exists('ChildProcessClass1351', false), 'ChildProcessClass1351 is not loaded.'); + } +} From 6529e4030228d14e6e99c40b95d8478f5625134b Mon Sep 17 00:00:00 2001 From: sun Date: Fri, 25 Jul 2014 03:36:49 +0200 Subject: [PATCH 2/4] Fix #1351: Isolated TestResult contains serialized test class upon failure. --- src/Framework/Error.php | 2 +- src/Framework/Exception.php | 62 ++++++++++++++ src/Framework/ExceptionWrapper.php | 125 +++++++++++++++++++++++++++++ src/Framework/TestFailure.php | 23 +++--- src/Framework/TestResult.php | 3 + src/TextUI/ResultPrinter.php | 33 ++------ src/Util/Filter.php | 9 ++- 7 files changed, 216 insertions(+), 41 deletions(-) create mode 100644 src/Framework/ExceptionWrapper.php diff --git a/src/Framework/Error.php b/src/Framework/Error.php index 65ced265cfd..afac325f55a 100644 --- a/src/Framework/Error.php +++ b/src/Framework/Error.php @@ -54,7 +54,7 @@ * @link http://www.phpunit.de/ * @since Class available since Release 2.2.0 */ -class PHPUnit_Framework_Error extends Exception +class PHPUnit_Framework_Error extends PHPUnit_Framework_Exception { /** * Constructor. diff --git a/src/Framework/Exception.php b/src/Framework/Exception.php index 832b3a19b4f..4d25d3f4315 100644 --- a/src/Framework/Exception.php +++ b/src/Framework/Exception.php @@ -44,6 +44,25 @@ */ /** + * Base class for all PHPUnit Framework exceptions. + * + * Ensures that exceptions thrown during a test run do not leave stray + * references behind. + * + * Every Exception contains a stack trace. Each stack frame contains the 'args' + * of the called function. The function arguments can contain references to + * instantiated objects. The references prevent the objects from being + * destructed (until test results are eventually printed), so memory cannot be + * freed up. + * + * With enabled process isolation, test results are serialized in the child + * process and unserialized in the parent process. The stack trace of Exceptions + * may contain objects that cannot be serialized or unserialized (e.g., PDO + * connections). Unserializing user-space objects from the child process into + * the parent would break the intended encapsulation of process isolation. + * + * @see http://fabien.potencier.org/article/9/php-serialization-stack-traces-and-exceptions + * * @package PHPUnit * @subpackage Framework * @author Sebastian Bergmann @@ -54,4 +73,47 @@ */ class PHPUnit_Framework_Exception extends RuntimeException implements PHPUnit_Exception { + /** + * @var array + */ + protected $serializableTrace; + + public function __construct($message = '', $code = 0, Exception $previous = null) + { + parent::__construct($message, $code, $previous); + + $this->serializableTrace = $this->getTrace(); + foreach ($this->serializableTrace as $i => $call) { + unset($this->serializableTrace[$i]['args']); + } + } + + /** + * Returns the serializable trace (without 'args'). + * + * @return array + */ + public function getSerializableTrace() + { + return $this->serializableTrace; + } + + /** + * @return string + */ + public function __toString() + { + $string = PHPUnit_Framework_TestFailure::exceptionToString($this); + + if ($trace = PHPUnit_Util_Filter::getFilteredStacktrace($this)) { + $string .= "\n" . $trace; + } + + return $string; + } + + public function __sleep() + { + return array_keys(get_object_vars($this)); + } } diff --git a/src/Framework/ExceptionWrapper.php b/src/Framework/ExceptionWrapper.php new file mode 100644 index 00000000000..312745c52bb --- /dev/null +++ b/src/Framework/ExceptionWrapper.php @@ -0,0 +1,125 @@ +. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in + * the documentation and/or other materials provided with the + * distribution. + * + * * Neither the name of Sebastian Bergmann nor the names of his + * contributors may be used to endorse or promote products derived + * from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN + * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + * + * @package PHPUnit + * @subpackage Framework + * @author Sebastian Bergmann + * @copyright 2001-2014 Sebastian Bergmann + * @license http://www.opensource.org/licenses/BSD-3-Clause The BSD 3-Clause License + * @link http://www.phpunit.de/ + * @since File available since Release 4.1.5 + */ + +/** + * Wraps Exceptions thrown by code under test. + * + * Re-instantiates Exceptions thrown by user-space code to retain their original + * class names, properties, and stack traces (but without arguments). + * + * Unlike PHPUnit_Framework_Exception, the complete stack of previous Exceptions + * is processed. + * + * @package PHPUnit + * @subpackage Framework + * @author Daniel F. Kudwien + * @copyright 2001-2014 Sebastian Bergmann + * @license http://www.opensource.org/licenses/BSD-3-Clause The BSD 3-Clause License + * @link http://www.phpunit.de/ + * @since Class available since Release 4.1.5 + */ +class PHPUnit_Framework_ExceptionWrapper extends PHPUnit_Framework_Exception +{ + /** + * @var string + */ + protected $classname; + + /** + * @var PHPUnit_Framework_ExceptionWrapper|null + */ + protected $previous; + + public function __construct(Exception $e) + { + parent::__construct($e->getMessage(), $e->getCode()); + $this->classname = get_class($e); + $this->file = $e->getFile(); + $this->line = $e->getLine(); + + $this->serializableTrace = $e->getTrace(); + foreach ($this->serializableTrace as $i => $call) { + unset($this->serializableTrace[$i]['args']); + } + + if ($e->getPrevious()) { + $this->previous = new self($e->getPrevious()); + } + } + + /** + * @return string + */ + public function getClassname() + { + return $this->classname; + } + + /** + * @return PHPUnit_Framework_ExceptionWrapper + */ + public function getPreviousWrapped() + { + return $this->previous; + } + + /** + * @return string + */ + public function __toString() + { + $string = PHPUnit_Framework_TestFailure::exceptionToString($this); + + if ($trace = PHPUnit_Util_Filter::getFilteredStacktrace($this)) { + $string .= "\n" . $trace; + } + + if ($this->previous) { + $string .= "\nCaused by\n" . $this->previous; + } + + return $string; + } +} diff --git a/src/Framework/TestFailure.php b/src/Framework/TestFailure.php index 9da0b739efe..835ea91cad5 100644 --- a/src/Framework/TestFailure.php +++ b/src/Framework/TestFailure.php @@ -57,9 +57,9 @@ class PHPUnit_Framework_TestFailure { /** - * @var PHPUnit_Framework_Test + * @var string */ - protected $failedTest; + private $testName; /** * @var Exception @@ -74,7 +74,11 @@ class PHPUnit_Framework_TestFailure */ public function __construct(PHPUnit_Framework_Test $failedTest, Exception $thrownException) { - $this->failedTest = $failedTest; + if ($failedTest instanceof PHPUnit_Framework_SelfDescribing) { + $this->testName = $failedTest->toString(); + } else { + $this->testName = get_class($failedTest); + } $this->thrownException = $thrownException; } @@ -87,8 +91,7 @@ public function toString() { return sprintf( '%s: %s', - - $this->failedTest->toString(), + $this->testName, $this->thrownException->getMessage() ); } @@ -125,6 +128,8 @@ public static function exceptionToString(Exception $e) } } elseif ($e instanceof PHPUnit_Framework_Error) { $buffer = $e->getMessage() . "\n"; + } elseif ($e instanceof PHPUnit_Framework_ExceptionWrapper) { + $buffer = $e->getClassname() . ': ' . $e->getMessage() . "\n"; } else { $buffer = get_class($e) . ': ' . $e->getMessage() . "\n"; } @@ -133,13 +138,13 @@ public static function exceptionToString(Exception $e) } /** - * Gets the failed test. + * Returns the name of the failing test (including data set, if any). * - * @return PHPUnit_Framework_Test + * @return string */ - public function failedTest() + public function getTestName() { - return $this->failedTest; + return $this->testName; } /** diff --git a/src/Framework/TestResult.php b/src/Framework/TestResult.php index f373b082f83..7a34bc1cb78 100644 --- a/src/Framework/TestResult.php +++ b/src/Framework/TestResult.php @@ -695,7 +695,10 @@ public function run(PHPUnit_Framework_Test $test) } elseif ($e instanceof PHPUnit_Framework_SkippedTestError) { $skipped = true; } + } catch (PHPUnit_Framework_Exception $e) { + $error = true; } catch (Exception $e) { + $e = new PHPUnit_Framework_ExceptionWrapper($e); $error = true; } diff --git a/src/TextUI/ResultPrinter.php b/src/TextUI/ResultPrinter.php index 3a316c19bed..adf82c2de3b 100644 --- a/src/TextUI/ResultPrinter.php +++ b/src/TextUI/ResultPrinter.php @@ -260,20 +260,12 @@ protected function printDefect(PHPUnit_Framework_TestFailure $defect, $count) */ protected function printDefectHeader(PHPUnit_Framework_TestFailure $defect, $count) { - $failedTest = $defect->failedTest(); - - if ($failedTest instanceof PHPUnit_Framework_SelfDescribing) { - $testName = $failedTest->toString(); - } else { - $testName = get_class($failedTest); - } - $this->write( sprintf( "\n%d) %s\n", $count, - $testName + $defect->getTestName() ) ); } @@ -283,26 +275,11 @@ protected function printDefectHeader(PHPUnit_Framework_TestFailure $defect, $cou */ protected function printDefectTrace(PHPUnit_Framework_TestFailure $defect) { - $this->write($defect->getExceptionAsString()); - - $trace = PHPUnit_Util_Filter::getFilteredStacktrace( - $defect->thrownException() - ); - - if (!empty($trace)) { - $this->write("\n" . $trace); - } - - $e = $defect->thrownException()->getPrevious(); - - while ($e) { - $this->write( - "\nCaused by\n" . - PHPUnit_Framework_TestFailure::exceptionToString($e). "\n" . - PHPUnit_Util_Filter::getFilteredStacktrace($e) - ); + $e = $defect->thrownException(); + $this->write((string) $e); - $e = $e->getPrevious(); + while ($e = $e->getPrevious()) { + $this->write("\nCaused by\n" . $e); } } diff --git a/src/Util/Filter.php b/src/Util/Filter.php index b36ce7d75e7..5a0ae4feb19 100644 --- a/src/Util/Filter.php +++ b/src/Util/Filter.php @@ -82,12 +82,15 @@ public static function getFilteredStacktrace(Exception $e, $asString = true) $eTrace = $e->getSyntheticTrace(); $eFile = $e->getSyntheticFile(); $eLine = $e->getSyntheticLine(); + } elseif ($e instanceof PHPUnit_Framework_Exception) { + $eTrace = $e->getSerializableTrace(); + $eFile = $e->getFile(); + $eLine = $e->getLine(); } else { if ($e->getPrevious()) { - $eTrace = $e->getPrevious()->getTrace(); - } else { - $eTrace = $e->getTrace(); + $e = $e->getPrevious(); } + $eTrace = $e->getTrace(); $eFile = $e->getFile(); $eLine = $e->getLine(); } From 1aefe3ab2078a002e499c3c3f9cee9436753e143 Mon Sep 17 00:00:00 2001 From: sun Date: Sat, 26 Jul 2014 20:33:25 +0200 Subject: [PATCH 3/4] Fixed PDOException::getCode() string vs. int mismatch. --- src/Framework/ExceptionWrapper.php | 5 ++++- tests/Regression/GitHub/1351.phpt | 10 +++++++--- tests/Regression/GitHub/1351/Issue1351Test.php | 9 +++++++++ 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/Framework/ExceptionWrapper.php b/src/Framework/ExceptionWrapper.php index 312745c52bb..1105afa6ece 100644 --- a/src/Framework/ExceptionWrapper.php +++ b/src/Framework/ExceptionWrapper.php @@ -74,7 +74,10 @@ class PHPUnit_Framework_ExceptionWrapper extends PHPUnit_Framework_Exception public function __construct(Exception $e) { - parent::__construct($e->getMessage(), $e->getCode()); + // PDOException::getCode() is a string. + // @see http://php.net/manual/en/class.pdoexception.php#95812 + parent::__construct($e->getMessage(), (int) $e->getCode()); + $this->classname = get_class($e); $this->file = $e->getFile(); $this->line = $e->getLine(); diff --git a/tests/Regression/GitHub/1351.phpt b/tests/Regression/GitHub/1351.phpt index fe95d3ad946..1244d6d2bdc 100644 --- a/tests/Regression/GitHub/1351.phpt +++ b/tests/Regression/GitHub/1351.phpt @@ -14,11 +14,11 @@ PHPUnit_TextUI_Command::main(); --EXPECTF-- PHPUnit %s by Sebastian Bergmann. -F.E. +F.E.E Time: %s, Memory: %sMb -There was 1 error: +There were 2 errors: 1) Issue1351Test::testExceptionPre RuntimeException: Expected rethrown exception. @@ -27,6 +27,10 @@ Caused by LogicException: Expected exception. %A +2) Issue1351Test::testPhpCoreLanguageException +PDOException: SQLSTATE[HY000]: General error: 1 no such table: php_wtf +%A + -- There was 1 failure: @@ -35,4 +39,4 @@ There was 1 failure: Expected failure. %A FAILURES! -Tests: 4, Assertions: 5, Failures: 1, Errors: 1. \ No newline at end of file +Tests: 5, Assertions: 5, Failures: 1, Errors: 2. \ No newline at end of file diff --git a/tests/Regression/GitHub/1351/Issue1351Test.php b/tests/Regression/GitHub/1351/Issue1351Test.php index 9dac75defce..6008b13bd7e 100644 --- a/tests/Regression/GitHub/1351/Issue1351Test.php +++ b/tests/Regression/GitHub/1351/Issue1351Test.php @@ -36,4 +36,13 @@ public function testExceptionPost() $this->assertNull($this->instance); $this->assertFalse(class_exists('ChildProcessClass1351', false), 'ChildProcessClass1351 is not loaded.'); } + + public function testPhpCoreLanguageException() + { + // User-space code cannot instantiate a PDOException with a string code, + // so trigger a real one. + $connection = new PDO('sqlite::memory:'); + $connection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); + $connection->query("DELETE FROM php_wtf WHERE exception_code = 'STRING'"); + } } From abc60f8ae82477529f470260b62e13347b73ea07 Mon Sep 17 00:00:00 2001 From: sun Date: Sat, 26 Jul 2014 03:05:28 +0200 Subject: [PATCH 4/4] Fixed bogus expectation in ExceptionStackTest. Catching a PHPUnit exception and rethrowing it as a non-PHPUnit exception does not make any sense. --- tests/TextUI/exception-stack.phpt | 3 +-- tests/_files/ExceptionStackTest.php | 4 +--- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/tests/TextUI/exception-stack.phpt b/tests/TextUI/exception-stack.phpt index 9d81da414f2..6a078c353fc 100644 --- a/tests/TextUI/exception-stack.phpt +++ b/tests/TextUI/exception-stack.phpt @@ -19,7 +19,7 @@ Time: %s, Memory: %sMb There were 2 errors: 1) ExceptionStackTest::testPrintingChildException -ExceptionStackTestException: Child exception +PHPUnit_Framework_Exception: Child exception message Failed asserting that two arrays are equal. --- Expected @@ -31,7 +31,6 @@ Failed asserting that two arrays are equal. ) -%s:%i %s:%i Caused by diff --git a/tests/_files/ExceptionStackTest.php b/tests/_files/ExceptionStackTest.php index fd86c8cf0e4..cd067b0ba9f 100644 --- a/tests/_files/ExceptionStackTest.php +++ b/tests/_files/ExceptionStackTest.php @@ -1,6 +1,4 @@ assertEquals(array(1), array(2), 'message'); } catch (PHPUnit_Framework_ExpectationFailedException $e) { $message = $e->getMessage() . $e->getComparisonFailure()->getDiff(); - throw new ExceptionStackTestException("Child exception\n$message", 101, $e); + throw new PHPUnit_Framework_Exception("Child exception\n$message", 101, $e); } }