Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate Process Will crash if backtrace contains unserialisable object. #282

Closed
jonathanDaigle opened this issue Jun 29, 2011 · 5 comments

Comments

@jonathanDaigle
Copy link

When a test fail or an error occurs in "separate process", A PHPUnit_Framework_TestFailure object will be serialised to be returned to the parent process.

PHPUnit_Framework_TestFailure, contains an exception, that contains a backtrace witch contains a bunch of input data.

If that data is not serialisable or unserialisable a problem will ocur.

For example, if unserializing some of that data causes a fatal error. PHPUnit will crash without any output (because the unserialise is silenced by a @).

@jonathanDaigle
Copy link
Author

I Do have a simple & easy patch for this. Unfortunately there is a downside with it. any custom listeners wont have access to the real original backtrace, so they may report a wrong backtrace.

However i believe that all built-in "listeners" wont suffer from that issue since they rely on PHPUnit_Framework_TestFailure to render the output of the backtrace. Witch is patched to pre-render it in the child process.

If you can live with that here is a patch that will help (Apply On the File : PHPUnit/Framework/TestFailure.php) ...

Index: TestFailure.php
===================================================================
--- TestFailure.php     (revision 170887)
+++ TestFailure.php     (revision 170886)
@@ -55,7 +55,7 @@
  * @link       http://www.phpunit.de/
  * @since      Class available since Release 2.0.0
  */
-class PHPUnit_Framework_TestFailure implements Serializable
+class PHPUnit_Framework_TestFailure
 {
     /**
      * @var    PHPUnit_Framework_Test
@@ -114,10 +114,6 @@
      */
     public static function exceptionToString(Exception $e)
     {
-        if (true === isset($e->exceptionToString)) {
-            return $e->exceptionToString;
-        }
-
         if ($e instanceof PHPUnit_Framework_SelfDescribing) {
             if ($e instanceof PHPUnit_Framework_ExpectationFailedException) {
                 $comparisonFailure = $e->getComparisonFailure();
@@ -186,7 +182,6 @@
             $buffer = get_class($e) . ': ' . $e->getMessage() . "\n";
         }

-        $e->exceptionToString = $buffer;
         return $buffer;
     }

@@ -230,62 +225,4 @@
     {
         return ($this->thrownException() instanceof PHPUnit_Framework_AssertionFailedError);
     }
-
-    /**
-     * Return data to be keep before serialisation
-     *
-     * GammaE Patch : When running in separate process.
-     * $this class will be serialised In "child process" and unserialised In
-     * Parent process.
-     * Unfortunately $this object contains an exception, that contains a bactrace
-     * Tha contain input values that may not be serialisable / unserialisable.
-     * To a avoid this roblem when create a new exception on deserialisation.
-     * The dowside effect is that the real backtrace is lost.
-     *
-     * @return string
-     */
-    public function serialize()
-    {
-        self::exceptionToString($this->thrownException);
-
-        $data = array($this->failedTest,
-                      $this->thrownException->exceptionToString,
-                      get_class($this->thrownException),
-                      $this->thrownException->getMessage(),
-                      $this->thrownException->getCode(),
-                      $this->thrownException->getFile(),
-                      $this->thrownException->getLine());
-
-        return serialize($data);
-    }
-
-    /**
-     * Unserialisation method
-     *
-     * GammaE Patch : When running in separate process.
-     * $this class will be serialised In "child process" and unserialised In
-     * Parent process.
-     * Unfortunately $this object contains an exception, that contains a bactrace
-     * Tha contain input values that may not be serialisable / unserialisable.
-     * To a avoid this roblem when create a new exception on deserialisation.
-     * The dowside effect is that the real backtrace is lost.
-     *
-     * @param  string $serialised Serialised properties
-     * @return void
-     */
-    public function unserialize($serialised)
-    {
-        $data = unserialize($serialised);
-
-        $exceptionClass = $data[2];
-        if (false === class_exists($exceptionClass, true)) {
-            $exceptionClass = 'Exception';
-        }
-
-        $this->failedTest = $data[0];
-
-        $this->thrownException                    = new $exceptionClass($data[3], $data[4]);
-        $this->thrownException->exceptionToString = $data[1];
-        $this->thrownException->isUnserialised    = true;
-    }
 }

@hpbuniat
Copy link
Contributor

I had some similar problems with a this case (Zend-Frameworks Zend_Locale_Data can have SimpleXMLElement as $_ldml property which is not serializable).

The most occurrences should be solvable by implementing __sleep/__wakeup to handle not-serializeable data correctly - e.g. converting SimpleXML to string. This should give you also more convenience with storing your data, e.g. in a session.

I would say, phpunit should not take care of this.

@jonathanDaigle
Copy link
Author

Here is am bugfixed version of the path. It now preserve the original File & Line number of the exception.

Also this patch fixes an other issu : php errors running in separate procees now have a full trace to help debuging.
(They use to only have a message, no file & no line numbers.)

Index: TestFailure.php
===================================================================
--- TestFailure.php     (revision 172738)
+++ TestFailure.php     (revision 170886)
@@ -55,7 +55,7 @@
  * @link       http://www.phpunit.de/
  * @since      Class available since Release 2.0.0
  */
-class PHPUnit_Framework_TestFailure implements Serializable
+class PHPUnit_Framework_TestFailure
 {
     /**
      * @var    PHPUnit_Framework_Test
@@ -114,10 +114,6 @@
      */
     public static function exceptionToString(Exception $e)
     {
-        if (true === isset($e->exceptionToString)) {
-            return $e->exceptionToString;
-        }
-
         if ($e instanceof PHPUnit_Framework_SelfDescribing) {
             if ($e instanceof PHPUnit_Framework_ExpectationFailedException) {
                 $comparisonFailure = $e->getComparisonFailure();
@@ -179,14 +175,13 @@
         }

         else if ($e instanceof PHPUnit_Framework_Error) {
-            $buffer = $e->__toString() . "\n";
+            $buffer = $e->getMessage() . "\n";
         }

         else {
-            $buffer = get_class($e) . ': ' . $e->__toString() . "\n";
+            $buffer = get_class($e) . ': ' . $e->getMessage() . "\n";
         }

-        $e->exceptionToString = $buffer;
         return $buffer;
     }

@@ -230,71 +225,4 @@
     {
         return ($this->thrownException() instanceof PHPUnit_Framework_AssertionFailedError);
     }
-
-    /**
-     * Return data to be keep before serialisation
-     *
-     * GammaE Patch : When running in separate process.
-     * $this class will be serialised In "child process" and unserialised In
-     * Parent process.
-     * Unfortunately $this object contains an exception, that contains a bactrace
-     * Tha contain input values that may not be serialisable / unserialisable.
-     * To a avoid this roblem when create a new exception on deserialisation.
-     * The dowside effect is that the real backtrace is lost.
-     *
-     * @return string
-     */
-    public function serialize()
-    {
-        self::exceptionToString($this->thrownException);
-
-        $data = array($this->failedTest,
-                      $this->thrownException->exceptionToString,
-                      get_class($this->thrownException),
-                      $this->thrownException->getMessage(),
-                      $this->thrownException->getCode(),
-                      $this->thrownException->getFile(),
-                      $this->thrownException->getLine());
-
-        return serialize($data);
-    }
-
-    /**
-     * Unserialisation method
-     *
-     * GammaE Patch : When running in separate process.
-     * $this class will be serialised In "child process" and unserialised In
-     * Parent process.
-     * Unfortunately $this object contains an exception, that contains a bactrace
-     * Tha contain input values that may not be serialisable / unserialisable.
-     * To a avoid this roblem when create a new exception on deserialisation.
-     * The dowside effect is that the real backtrace is lost.
-     *
-     * @param  string $serialised Serialised properties
-     * @return void
-     */
-    public function unserialize($serialised)
-    {
-        $data = unserialize($serialised);
-
-        $exceptionClass = $data[2];
-        if (false === class_exists($exceptionClass, true)) {
-            $exceptionClass = 'Exception';
-        }
-        $this->failedTest = $data[0];
-
-        $e                    = new $exceptionClass($data[3], $data[4]);
-        $e->exceptionToString = $data[1];
-        $e->isUnserialised    = true;
-        $reflectionObj = new ReflectionObject($e);
-        $refFile       = $reflectionObj->getProperty('file');
-        $refLine       = $reflectionObj->getProperty('line');
-        $refFile->setAccessible(true);
-        $refLine->setAccessible(true);
-        $refFile->setValue($e, $data[5]);
-        $refLine->setValue($e, $data[6]);
-
-        $this->thrownException = $e;
-    }
 }

@jonathanDaigle
Copy link
Author

hpbuniat : i am sorry but i don't agree.

It is not your code responsibility to make shure its backtrace is serialisable.

Any way that would simply be impossible, some stuff just cant be. Resources (like a PDO connection) are impossible to serialises.

@edorian
Copy link
Contributor

edorian commented Nov 11, 2011

Closing as duplicate/closely related of #254. I'm not sure that patch is the correct way of going about things but we consider it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants