Process isolation TestResult contains serialized test class upon test failure/exception

[sun]

If a test is executed in a separate process and it passes, then the serialized $result is a simple instance of PHPUnit_Framework_TestResult.

But if it fails, then the TestResult::$failures property contains a TestFailure object,

1. whose $failedTest property contains the complete instance of the test class,
   and
2. whose $thrownException property contains an instance of ExpectationFailedException, which in turn contains a full backtrace that includes another instance of the test class.

A full serialization of the TestCase is problematic, because the class instance can have properties that cannot be serialized - or - which will be unserialized into the scope of the parent process (e.g., a full service container, including instantiated database connections).

To see it in action, Inject a var_dump($result); right before the serialization in the process isolation template + execute a test that fails:

class PHPUnit_Framework_TestResult#3 (26) {
  protected $passed => array(0) {}
  protected $errors => array(0) {}
  protected $deprecatedFeatures => array(0) {}
  protected $failures => array(1) {
    [0] => class PHPUnit_Framework_TestFailure#3716 (2) {

      protected $failedTest => class Drupal\views\Tests\Handler\ArgumentDateTest#4 (44) {
        protected $preserveGlobalState => bool(false)
        ...

      protected $thrownException => class PHPUnit_Framework_ExpectationFailedException#2968 (9) {
        protected $comparisonFailure => NULL
        protected $message => ...
        ...
        private $trace => array(15) {
          ...
          [5] => array(6) {
            'class' => string(35) "Drupal\views\Tests\ViewUnitTestBase"
            'args' => array(5) {
              [0] => class Drupal\views\ViewExecutable#2575 (55) {
                public $storage => class Drupal\views\Entity\View#3655 (21) {
                ...
              ...
           ...

Now, the part I don't understand: Why does TestFailure hold objects to begin with?

1. $failedTest is only consumed once in TestFailure::toString(), to call the corresponding method on TestCase.
2. $thrownException is only consumed to generate string representations of the exception's message + backtrace.

Both operations could be performed ahead of time upon instantiation of TestFailure, so that it no longer references any objects and just simply holds a string representation of the test failure.

Also positive impact on memory: By removing the object references, they can be destructed sooner. Currently, they're kept around + add up until test results are printed. - In case of process isolation, all involved objects of the child process are newly instantiated in the parent process, clobbering CPU + memory (for no purpose).

The response in similar issues has been that this would be required to back up global state with enabled process isolation. — However, I can't get behind that idea. In my mind, process isolation is one-directional, not bi-directional. Global state is injected into the child process only, not vice-versa. The whole point of process isolation is that the child process should not affect the parent process, so unserializing any kind of user data/objects into the parent process defeats that purpose (because unserialize() is an unholy magic beast).

Proposed solution

1. Remove instantiated (child process) objects from TestFailure #1352 Remove the properties referencing injected objects from TestFailure, make it hold a string representation of the failure only.

I'm not familiar with this part of the code yet, but given some initial pointers, I could try to get my hands dirty.

Related issues

• Separate Process Will crash if backtrace contains unserialisable object. #282 seemingly reported this problem in the past, but suggested a slightly weird solution.
• Code paths that create a PHPUnit_Framework_Warning end up serializing/unserializing globals unconditionally #7 fixed a similar problem for Warning already, but the solution does not apply here.
• Serialization of 'Closure' is not allowed #451 is about serialization of Closures in process isolation, which might have the same root cause.

API changes

The only backwards-incompatible API change is in TestFailure:

diff src/Framework/TestFailure.php
     /**
-     * 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;
     }

However, the only call existed in ResultPrinter:

diff src/TextUI/ResultPrinter.php
     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(
...
-            $testName
+            $defect->getTestName()

If that's deemed to be an integral part of the API, then I assume this fix will have to wait for the next minor release. (Hopefully not major 😉)

---

Original intro:
I'm a Drupal core developer, and I want to rebase 900+ functional integration tests from its custom testing framework (originally a fork of Simpletest) to PHPUnit. Due to many fatal errors, I don't even get a test result that would allow me to debug similarities in failures, so that's why process isolation is highly useful.

I don't plan to keep it enabled in the end, but a future change to (Drupal) APIs might run into similar mass-failure conditions, in which case process isolation would likely help to pinpoint the root causes again, so that's why I'm looking into these bugs.

[sun]

For completeness, #282 (comment) suggested an alternative approach; to implement __sleep()/__wakeup(), so as to filter out problematic instance properties. But that's inapplicable, for three reasons:

1. Private properties of TestCase are not in the visible scope and thus will not be serialized. In any case, user-space tests should not be aware of PHPUnit's internals.
2. Custom test base classes may cater for themselves, but every subclass would have to manually cater for its own properties all over again.
3. The class instances are still unserialized into the parent process environment (which, meanwhile, is the most problematic aspect from my perspective).

[sun]

Started work on a PR. Regression test fails cleanly. Getting rid of the test class instance is trivial. Getting rid of the exception is a bit more tricky, because ResultPrinter::printDefectTrace() expects a fully functional Exception stack + stacktrace. Not sure how to resolve that yet, but I'll study further. Ideas/pointers would be appreciated.

The proposed solution also appears to be the proper, belated fix for #74 — which introduced PHPUnit_Util_PHP::getException(). That extraneous measure only exists, because we're trying to re-instantiate arbitrary classes of the child process in the parent process. A __PHP_Incomplete_Class can only exist in the parent process. Otherwise the child process would exit with a fatal error already.

As a result, I'm fairly sure that this is the root cause for many other issues.

[whatthejeff]

Upfront: Sorry, folks - I'm aware that I'm currently littering the queue with bug reports & fixes for functionality that isn't fully supported (e.g., process isolation) and for platforms that many people don't really care about (e.g., Windows)… 😉

No need to be sorry. We appreciate whatever help we can get :)

[sun]

because unserialize() is an unholy magic beast

Sorry, belated background: http://heine.familiedeelstra.com/security/unserialize

Unserializing arbitrary classes with arbitrary data in a different execution context can have unexpected side-effects. I'm not saying that I managed to exploit this, but it is a risk of its own.

---

Meanwhile, #1352 contains the initial proposal. Conceptually, I borrowed the idea of Symfony's FlattenException, which essentially exists for a very similar purpose: Avoiding object references and their serialization.

I almost succeeded, until I noticed that the entire Runner + Text_UI + Printer architecture always expects instances of PHPUnit_Framework_AssertionFailedError, which happen to extend Exception currently… Anything that is an Exception has a trace, and every trace has class instances… 🐼

Not sure how to proceed.

[sun]

Oh great, @fabpot tinkered about this problem in the past already:
http://fabien.potencier.org/article/9/php-serialization-stack-traces-and-exceptions

For starters, implementing that solution directly on AssertionFailedError resolves the problem (see #1352). The TestFailure now only contains the test name string (instead of full class instance) + the thrown exception, but without the original stack trace. Instead, we only serialize a cleaned copy of the stack trace (without args).

This makes the regression test pass + fixes some fatal errors in my tests already. However, it only works for assertion failures, but not when a test throws an exception. I'll see whether moving the code into the base PHPUnit_Framework_Exception class helps to resolve that.

Lastly, Serializable only exists since PHP 5.4.0. I'll check whether __sleep() works, too.

[sun]

Alrighty, the last puzzle piece was to

1. Introduce an ExceptionWrapper class, which performs the same operation for arbitrary exceptions thrown by user code (!= PHPUnit_Framework_Exception)
2. Catch + convert any unexpectedly thrown exceptions inside a test run.

⭐ …and as if fixing just the bug wasn't fantastic enough already, also note the following:

diff 4.1 ... 4.1+1352
 $ ./phpunit tests/Regression/GitHub/1351.phpt
 PHPUnit 4.1.4-13-g90bb8a5 by Sebastian Bergmann.

-F
+.

-Time: 1.3 seconds,  Memory: 2.50Mb
+Time: 1.22 seconds, Memory: 2.25Mb

(just a single test)

Last remaining steps are (1) PHP 5.3 compatibility and (2) investigating the expectation mismatches of existing tests.

[sun]

#1352 now has the fully implemented, polished, and final code.
(FWIW, the PR is not meant to be merged as-is; I did not clean up commit history upfront this time.)

The only remaining test failures are on hhvm-nightly. There seems to be a difference in how Exception stacks are handled; it outputs extraneous information in test results (minor). I don't know how to debug/fix that, as I don't have hhvm locally (…is there even a build for Windows?)

The only backwards-incompatible API change is in TestFailure:

diff src/Framework/TestFailure.php
     /**
-     * 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;
     }

However, the only call existed in ResultPrinter:

diff src/TextUI/ResultPrinter.php
     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(
...
-            $testName
+            $defect->getTestName()

If that's deemed to be an integral part of the API, then I assume this fix will have to wait for the next minor release. (Hopefully not major 😉)

[whatthejeff]

@sun There's no HHVM support for Windows at the moment. I can look into the HHVM failures if you want.

[sun]

@whatthejeff That would be great - I wouldn't know how to move forward otherwise.

[whatthejeff]

@sun Alright, I'll look into it shortly.

[sun]

Oy! I just fixed an unexpected fatal error caused by a PDOException::getCode() type mismatch sun@53ad508 — that fixed hhvm-nightly at the same time 👍

So this passed on all platforms now :)

[whatthejeff]

Very cool! I'll review this as soon as possible :)

[whatthejeff]

@sun Did you still want to cleanup the commit history?

[sun]

Yes, thanks, definitely. Thought I'd leave the full history in place this time, so all refactoring + debugging steps can be introspected by interested parties.

Let me clean up right now; will only take a few minutes.

[sun]

Done. :)

[whatthejeff]

This looks great to me, @sun. Since it is a pretty big change and does include a small BC break, I'm going to run it by @sebastianbergmann before I merge it.

[sun]

Interesting, @Agrumas. I assume there are only a few in the wild, but yes, custom Result printer implementations may be affected by the TestFailure::failedTest() → getTestName() change.

The PR was only merged into master thus far. I guess this means it can't go into 4.1, but only into the current alpha, 4.2 …?

If so, we need to adjust two version numbers in ExceptionWrapper:

diff src/Framework/ExceptionWrapper.php
+ * @since      File available since Release 4.1.5
…
+ * @since      Class available since Release 4.1.5

[sun]

Briefly looking into Codeception/Codeception#1248, its TestEvent architecture seems to heavily rely on the test class to be available in TestFailure.

Let me check whether it is possible to add it back in some way (e.g., by only setting the property in the parent process [in which results are printed]).

[whatthejeff]

@sun

Interesting, @Agrumas. I assume there are only a few in the wild, but yes, custom Result printer implementations may be affected by the TestFailure::failedTest() → getTestName() change.

Yeah, I maintain a few printers, so I thought this might be a possibility. This is why we have alphas though :).

The PR was only merged into master thus far. I guess this means it can't go into 4.1, but only into the current alpha, 4.2 …?

Since this was merged into master, it will go into 4.3. I was planning on updating the comments today.

[sun]

Given that #1359 restored BC, wondering whether this could be merged into 4.1 and/or 4.2?

[whatthejeff]

@sun I thought about this as well. I'm hesitant to push something out in a patch-level release that adds new public methods to a class like PHPUnit_Framework_TestCase (#1360) since it's so heavily subclassed. The primary goal of our stable releases should be not to break existing tests.