Refactor test result cache persistence to not rely on serialize() and…

… unserialize()
sebastianbergmann · May 3, 2021 · 3e3aecd · 3e3aecd
1 parent 39a44c2
commit 3e3aecd
Show file tree

Hide file tree

Showing 12 changed files with 74 additions and 190 deletions.
diff --git a/ChangeLog-8.5.md b/ChangeLog-8.5.md
@@ -2,6 +2,12 @@
 
 All notable changes of the PHPUnit 8.5 release series are documented in this file using the [Keep a CHANGELOG](https://keepachangelog.com/) principles.
 
+## [8.5.16] - 2021-MM-DD
+
+### Changed
+
+* The test result cache (the storage for which is implemented in `PHPUnit\Runner\DefaultTestResultCache`) no longer uses PHP's `serialize()` and `unserialize()` function for persistence. It now uses a versioned JSON format instead that is independent of PHP implementation details (see [#3581](https://github.com/sebastianbergmann/phpunit/issues/3581) and [#4662](https://github.com/sebastianbergmann/phpunit/pull/4662) for examples why this is a problem). When PHPUnit tries to load the test result cache from a file that does not exist, or from a file that does not contain data in JSON format, or from a file that contains data in a JSON format version other than the one uses by the currently running PHPUnit version, then this is considered to be a "cache miss". An empty `DefaultTestResultCache` object is created in this case. This should also prevent PHPUnit from crashing when trying to load a test result cache file created by a different version of PHPUnit (see [#4580](https://github.com/sebastianbergmann/phpunit/issues/4580) for example).
+
 ## [8.5.15] - 2021-03-17
 
 ### Fixed
@@ -130,6 +136,7 @@ All notable changes of the PHPUnit 8.5 release series are documented in this fil
 * [#3967](https://github.com/sebastianbergmann/phpunit/issues/3967): Cannot double interface that extends interface that extends `\Throwable`
 * [#3968](https://github.com/sebastianbergmann/phpunit/pull/3968): Test class run in a separate PHP process are passing when `exit` called inside
 
+[8.5.16]: https://github.com/sebastianbergmann/phpunit/compare/8.5.15...8.5.16
 [8.5.15]: https://github.com/sebastianbergmann/phpunit/compare/8.5.14...8.5.15
 [8.5.14]: https://github.com/sebastianbergmann/phpunit/compare/8.5.13...8.5.14
 [8.5.13]: https://github.com/sebastianbergmann/phpunit/compare/8.5.12...8.5.13

diff --git a/src/Runner/DefaultTestResultCache.php b/src/Runner/DefaultTestResultCache.php
@@ -10,40 +10,31 @@
 namespace PHPUnit\Runner;
 
 use const DIRECTORY_SEPARATOR;
+use const LOCK_EX;
 use function assert;
-use function defined;
-use function dirname;
 use function file_get_contents;
 use function file_put_contents;
 use function in_array;
+use function is_array;
 use function is_dir;
 use function is_file;
-use function is_float;
-use function is_int;
-use function is_string;
-use function serialize;
-use function sprintf;
-use function unserialize;
-use PHPUnit\Util\ErrorHandler;
-use PHPUnit\Util\Filesystem;
-use Serializable;
+use function json_decode;
+use function json_encode;
 
 /**
  * @internal This class is not covered by the backward compatibility promise for PHPUnit
  */
-final class DefaultTestResultCache implements Serializable, TestResultCache
+final class DefaultTestResultCache implements TestResultCache
 {
     /**
-     * @var string
+     * @var int
      */
-    public const DEFAULT_RESULT_CACHE_FILENAME = '.phpunit.result.cache';
+    private const VERSION = 1;
 
     /**
-     * Provide extra protection against incomplete or corrupt caches.
-     *
-     * @var int[]
+     * @psalm-var list<int>
      */
-    private const ALLOWED_CACHE_TEST_STATUSES = [
+    private const ALLOWED_TEST_STATUSES = [
         BaseTestRunner::STATUS_SKIPPED,
         BaseTestRunner::STATUS_INCOMPLETE,
         BaseTestRunner::STATUS_FAILURE,
@@ -53,83 +44,41 @@ final class DefaultTestResultCache implements Serializable, TestResultCache
     ];
 
     /**
-     * Path and filename for result cache file.
-     *
+     * @var string
+     */
+    private const DEFAULT_RESULT_CACHE_FILENAME = '.phpunit.result.cache';
+
+    /**
      * @var string
      */
     private $cacheFilename;
 
     /**
-     * The list of defective tests.
-     *
-     * <code>
-     * // Mark a test skipped
-     * $this->defects[$testName] = BaseTestRunner::TEST_SKIPPED;
-     * </code>
-     *
-     * @var array<string, int>
+     * @psalm-var array<string, int>
      */
     private $defects = [];
 
     /**
-     * The list of execution duration of suites and tests (in seconds).
-     *
-     * <code>
-     * // Record running time for test
-     * $this->times[$testName] = 1.234;
-     * </code>
-     *
-     * @var array<string, float>
+     * @psalm-var array<string, float>
      */
     private $times = [];
 
     public function __construct(?string $filepath = null)
     {
         if ($filepath !== null && is_dir($filepath)) {
-            // cache path provided, use default cache filename in that location
             $filepath .= DIRECTORY_SEPARATOR . self::DEFAULT_RESULT_CACHE_FILENAME;
         }
 
         $this->cacheFilename = $filepath ?? $_ENV['PHPUNIT_RESULT_CACHE'] ?? self::DEFAULT_RESULT_CACHE_FILENAME;
     }
 
-    /**
-     * @throws Exception
-     */
-    public function persist(): void
-    {
-        $this->saveToFile();
-    }
-
-    /**
-     * @throws Exception
-     */
-    public function saveToFile(): void
+    public function setState(string $testName, int $state): void
     {
-        if (defined('PHPUNIT_TESTSUITE_RESULTCACHE')) {
+        if (!in_array($state, self::ALLOWED_TEST_STATUSES, true)) {
             return;
         }
 
-        if (!Filesystem::createDirectory(dirname($this->cacheFilename))) {
-            throw new Exception(
-                sprintf(
-                    'Cannot create directory "%s" for result cache file',
-                    $this->cacheFilename
-                )
-            );
-        }
-
-        file_put_contents(
-            $this->cacheFilename,
-            serialize($this)
-        );
-    }
-
-    public function setState(string $testName, int $state): void
-    {
-        if ($state !== BaseTestRunner::STATUS_PASSED) {
-            $this->defects[$testName] = $state;
-        }
+        $this->defects[$testName] = $state;
     }
 
     public function getState(string $testName): int
@@ -149,85 +98,46 @@ public function getTime(string $testName): float
 
     public function load(): void
     {
-        $this->clear();
-
         if (!is_file($this->cacheFilename)) {
             return;
         }
 
-        $cacheData = @file_get_contents($this->cacheFilename);
-
-        // @codeCoverageIgnoreStart
-        if ($cacheData === false) {
-            return;
-        }
-        // @codeCoverageIgnoreEnd
-
-        $cache = ErrorHandler::invokeIgnoringWarnings(
-            static function () use ($cacheData) {
-                return @unserialize($cacheData, ['allowed_classes' => [self::class]]);
-            }
+        $data = json_decode(
+            file_get_contents($this->cacheFilename),
+            true
         );
 
-        if ($cache === false) {
+        if ($data === null) {
             return;
         }
 
-        if ($cache instanceof self) {
-            /* @var DefaultTestResultCache $cache */
-            $cache->copyStateToCache($this);
-        }
-    }
-
-    public function copyStateToCache(self $targetCache): void
-    {
-        foreach ($this->defects as $name => $state) {
-            $targetCache->setState($name, $state);
+        if (!isset($data['version'])) {
+            return;
         }
 
-        foreach ($this->times as $name => $time) {
-            $targetCache->setTime($name, $time);
+        if ($data['version'] !== self::VERSION) {
+            return;
         }
-    }
 
-    public function clear(): void
-    {
-        $this->defects = [];
-        $this->times   = [];
-    }
+        assert(isset($data['defects']) && is_array($data['defects']));
+        assert(isset($data['times']) && is_array($data['times']));
 
-    public function serialize(): string
-    {
-        return serialize([
-            'defects' => $this->defects,
-            'times'   => $this->times,
-        ]);
+        $this->defects = $data['defects'];
+        $this->times   = $data['times'];
     }
 
-    /**
-     * @param string $serialized
-     */
-    public function unserialize($serialized): void
+    public function persist(): void
     {
-        $data = unserialize($serialized);
-
-        if (isset($data['times'])) {
-            foreach ($data['times'] as $testName => $testTime) {
-                assert(is_string($testName));
-                assert(is_float($testTime));
-                $this->times[$testName] = $testTime;
-            }
-        }
-
-        if (isset($data['defects'])) {
-            foreach ($data['defects'] as $testName => $testResult) {
-                assert(is_string($testName));
-                assert(is_int($testResult));
-
-                if (in_array($testResult, self::ALLOWED_CACHE_TEST_STATUSES, true)) {
-                    $this->defects[$testName] = $testResult;
-                }
-            }
-        }
+        file_put_contents(
+            $this->cacheFilename,
+            json_encode(
+                [
+                    'version' => self::VERSION,
+                    'defects' => $this->defects,
+                    'times'   => $this->times,
+                ]
+            ),
+            LOCK_EX
+        );
     }
 }
diff --git a/tests/_files/DataproviderExecutionOrderTest_result_cache.txt b/tests/_files/DataproviderExecutionOrderTest_result_cache.txt
@@ -1 +1 @@
-C:37:"PHPUnit\Runner\DefaultTestResultCache":2119:{a:2:{s:7:"defects";a:5:{s:60:"tests/end-to-end/regression/GitHub/3396/issue-3396-test.phpt";i:3;s:88:"MultiDependencyExecutionOrderTest::testAddNumbersWithADataprovider with data set "1+1=3"";i:3;s:92:"MultiDependencyExecutionOrderTest::testAddMoreNumbersWithADataprovider with data set "1+1=3"";i:3;s:85:"DataproviderExecutionOrderTest::testAddNumbersWithADataprovider with data set "1+1=3"";i:3;s:89:"DataproviderExecutionOrderTest::testAddMoreNumbersWithADataprovider with data set "1+1=3"";i:3;}s:5:"times";a:17:{s:60:"tests/end-to-end/regression/GitHub/3396/issue-3396-test.phpt";d:0.115;s:63:"MultiDependencyExecutionOrderTest::testFirstTestThatAlwaysWorks";d:0;s:88:"MultiDependencyExecutionOrderTest::testAddNumbersWithADataprovider with data set "1+2=3"";d:0;s:88:"MultiDependencyExecutionOrderTest::testAddNumbersWithADataprovider with data set "2+1=3"";d:0;s:88:"MultiDependencyExecutionOrderTest::testAddNumbersWithADataprovider with data set "1+1=3"";d:0.003;s:69:"MultiDependencyExecutionOrderTest::testTestInTheMiddleThatAlwaysWorks";d:0;s:92:"MultiDependencyExecutionOrderTest::testAddMoreNumbersWithADataprovider with data set "1+2=3"";d:0;s:92:"MultiDependencyExecutionOrderTest::testAddMoreNumbersWithADataprovider with data set "2+1=3"";d:0;s:92:"MultiDependencyExecutionOrderTest::testAddMoreNumbersWithADataprovider with data set "1+1=3"";d:0;s:60:"DataproviderExecutionOrderTest::testFirstTestThatAlwaysWorks";d:0.002;s:85:"DataproviderExecutionOrderTest::testAddNumbersWithADataprovider with data set "1+2=3"";d:0;s:85:"DataproviderExecutionOrderTest::testAddNumbersWithADataprovider with data set "2+1=3"";d:0;s:85:"DataproviderExecutionOrderTest::testAddNumbersWithADataprovider with data set "1+1=3"";d:0.001;s:66:"DataproviderExecutionOrderTest::testTestInTheMiddleThatAlwaysWorks";d:0;s:89:"DataproviderExecutionOrderTest::testAddMoreNumbersWithADataprovider with data set "1+2=3"";d:0;s:89:"DataproviderExecutionOrderTest::testAddMoreNumbersWithADataprovider with data set "2+1=3"";d:0;s:89:"DataproviderExecutionOrderTest::testAddMoreNumbersWithADataprovider with data set "1+1=3"";d:0;}}}
+{"version":1,"defects":{"tests\/end-to-end\/regression\/GitHub\/3396\/issue-3396-test.phpt":3,"MultiDependencyExecutionOrderTest::testAddNumbersWithADataprovider with data set \"1+1=3\"":3,"MultiDependencyExecutionOrderTest::testAddMoreNumbersWithADataprovider with data set \"1+1=3\"":3,"DataproviderExecutionOrderTest::testAddNumbersWithADataprovider with data set \"1+1=3\"":3,"DataproviderExecutionOrderTest::testAddMoreNumbersWithADataprovider with data set \"1+1=3\"":3},"times":{"tests\/end-to-end\/regression\/GitHub\/3396\/issue-3396-test.phpt":0.115,"MultiDependencyExecutionOrderTest::testFirstTestThatAlwaysWorks":0,"MultiDependencyExecutionOrderTest::testAddNumbersWithADataprovider with data set \"1+2=3\"":0,"MultiDependencyExecutionOrderTest::testAddNumbersWithADataprovider with data set \"2+1=3\"":0,"MultiDependencyExecutionOrderTest::testAddNumbersWithADataprovider with data set \"1+1=3\"":0.003,"MultiDependencyExecutionOrderTest::testTestInTheMiddleThatAlwaysWorks":0,"MultiDependencyExecutionOrderTest::testAddMoreNumbersWithADataprovider with data set \"1+2=3\"":0,"MultiDependencyExecutionOrderTest::testAddMoreNumbersWithADataprovider with data set \"2+1=3\"":0,"MultiDependencyExecutionOrderTest::testAddMoreNumbersWithADataprovider with data set \"1+1=3\"":0,"DataproviderExecutionOrderTest::testFirstTestThatAlwaysWorks":0.002,"DataproviderExecutionOrderTest::testAddNumbersWithADataprovider with data set \"1+2=3\"":0,"DataproviderExecutionOrderTest::testAddNumbersWithADataprovider with data set \"2+1=3\"":0,"DataproviderExecutionOrderTest::testAddNumbersWithADataprovider with data set \"1+1=3\"":0.001,"DataproviderExecutionOrderTest::testTestInTheMiddleThatAlwaysWorks":0,"DataproviderExecutionOrderTest::testAddMoreNumbersWithADataprovider with data set \"1+2=3\"":0,"DataproviderExecutionOrderTest::testAddMoreNumbersWithADataprovider with data set \"2+1=3\"":0,"DataproviderExecutionOrderTest::testAddMoreNumbersWithADataprovider with data set \"1+1=3\"":0}}
diff --git a/tests/end-to-end/execution-order/_files/MultiDependencyTest_result_cache.txt b/tests/end-to-end/execution-order/_files/MultiDependencyTest_result_cache.txt
@@ -1 +1 @@
-C:37:"PHPUnit\Runner\DefaultTestResultCache":289:{a:2:{s:7:"defects";a:1:{s:29:"MultiDependencyTest::testFive";i:1;}s:5:"times";a:5:{s:28:"MultiDependencyTest::testOne";d:0;s:28:"MultiDependencyTest::testTwo";d:0;s:30:"MultiDependencyTest::testThree";d:0;s:29:"MultiDependencyTest::testFour";d:0;s:29:"MultiDependencyTest::testFive";d:0;}}}
+{"version":1,"defects":{"MultiDependencyTest::testFive":1},"times":{"MultiDependencyTest::testOne":0,"MultiDependencyTest::testTwo":0,"MultiDependencyTest::testThree":0,"MultiDependencyTest::testFour":0,"MultiDependencyTest::testFive":0}}
diff --git a/tests/end-to-end/execution-order/_files/TestWithDifferentDurations.phpunit.result.cache.txt b/tests/end-to-end/execution-order/_files/TestWithDifferentDurations.phpunit.result.cache.txt
@@ -1 +1 @@
-C:37:"PHPUnit\Runner\DefaultTestResultCache":199:{a:2:{s:7:"defects";a:0:{}s:5:"times";a:3:{s:35:"TestWithDifferentDurations::testOne";d:1.000;s:35:"TestWithDifferentDurations::testTwo";d:0.500;s:37:"TestWithDifferentDurations::testThree";d:1.500;}}}
+{"version":1,"defects":[],"times":{"TestWithDifferentDurations::testOne":1,"TestWithDifferentDurations::testTwo":0.5,"TestWithDifferentDurations::testThree":1.5}}
diff --git a/tests/end-to-end/execution-order/cache-result.phpt b/tests/end-to-end/execution-order/cache-result.phpt
@@ -28,4 +28,4 @@ Time: %s, Memory: %s
 
 OK, but incomplete, skipped, or risky tests!
 Tests: 5, Assertions: 3, Skipped: 2.
-C:37:"PHPUnit\Runner\DefaultTestResultCache":%d:{a:2:{s:7:"defects";a:2:{s:29:"MultiDependencyTest::testFour";i:1;s:30:"MultiDependencyTest::testThree";i:1;}s:5:"times";a:5:{s:29:"MultiDependencyTest::testFive";d:%f;s:29:"MultiDependencyTest::testFour";d:%f;s:30:"MultiDependencyTest::testThree";d:%f;s:28:"MultiDependencyTest::testTwo";d:%f;s:28:"MultiDependencyTest::testOne";d:%f;}}}
+{"version":1,"defects":{"MultiDependencyTest::testFour":1,"MultiDependencyTest::testThree":1},"times":{"MultiDependencyTest::testFive":%f,"MultiDependencyTest::testFour":%f,"MultiDependencyTest::testThree":%f,"MultiDependencyTest::testTwo":%f,"MultiDependencyTest::testOne":%f}}
diff --git a/tests/end-to-end/execution-order/defects-first-order-via-cli.phpt b/tests/end-to-end/execution-order/defects-first-order-via-cli.phpt
@@ -3,7 +3,7 @@ phpunit --order-by=defects ./tests/_files/MultiDependencyTest.php
 --FILE--
 <?php declare(strict_types=1);
 $tmpResultCache = \tempnam(sys_get_temp_dir(), __FILE__);
-\file_put_contents($tmpResultCache, file_get_contents(__DIR__ . '/_files/MultiDependencyTest_result_cache.txt'));
+\copy(__DIR__ . '/_files/MultiDependencyTest_result_cache.txt', $tmpResultCache);
 
 $_SERVER['argv'][] = '--do-not-cache-result';
 $_SERVER['argv'][] = '--no-configuration';

diff --git a/tests/end-to-end/execution-order/order-by-duration-via-cli.phpt b/tests/end-to-end/execution-order/order-by-duration-via-cli.phpt
@@ -2,9 +2,8 @@
 phpunit --order-by=duration ./tests/end-to-end/execution-order/_files/TestWithDifferentDurations.php
 --FILE--
 <?php declare(strict_types=1);
-
 $tmpResultCache = tempnam(sys_get_temp_dir(), __FILE__);
-file_put_contents($tmpResultCache, file_get_contents(__DIR__ . '/_files/TestWithDifferentDurations.phpunit.result.cache.txt'));
+\copy(__DIR__ . '/_files/TestWithDifferentDurations.phpunit.result.cache.txt', $tmpResultCache);
 
 $_SERVER['argv'][] = '--do-not-cache-result';
 $_SERVER['argv'][] = '--no-configuration';

diff --git a/tests/end-to-end/execution-order/order-by-duration-via-phpunit-xml.phpt b/tests/end-to-end/execution-order/order-by-duration-via-phpunit-xml.phpt
@@ -2,9 +2,8 @@
 phpunit --configuration=order-by-duration.phpunit.xml
 --FILE--
 <?php declare(strict_types=1);
-
 $tmpResultCache = tempnam(sys_get_temp_dir(), __FILE__);
-file_put_contents($tmpResultCache, file_get_contents(__DIR__ . '/_files/TestWithDifferentDurations.phpunit.result.cache.txt'));
+\copy(__DIR__ . '/_files/TestWithDifferentDurations.phpunit.result.cache.txt', $tmpResultCache);
 
 $phpunitXmlConfig = __DIR__ . '/_files/order-by-duration.phpunit.xml';
 

diff --git a/tests/end-to-end/regression/GitHub/3380/issue-3380-test.phpt b/tests/end-to-end/regression/GitHub/3380/issue-3380-test.phpt
@@ -3,7 +3,7 @@ https://github.com/sebastianbergmann/phpunit/issues/3380
 --FILE--
 <?php declare(strict_types=1);
 $tmpResultCache = tempnam(sys_get_temp_dir(), __FILE__);
-file_put_contents($tmpResultCache, file_get_contents(__DIR__ . '/../../../../_files/DataproviderExecutionOrderTest_result_cache.txt'));
+\copy(__DIR__ . '/../../../../_files/DataproviderExecutionOrderTest_result_cache.txt', $tmpResultCache);
 
 $_SERVER['argv'][] = '--do-not-cache-result';
 $_SERVER['argv'][] = '--no-configuration';

diff --git a/tests/end-to-end/regression/GitHub/3396/issue-3396-test.phpt b/tests/end-to-end/regression/GitHub/3396/issue-3396-test.phpt
@@ -3,7 +3,7 @@ https://github.com/sebastianbergmann/phpunit/issues/3396
 --FILE--
 <?php declare(strict_types=1);
 $tmpResultCache = tempnam(sys_get_temp_dir(), __FILE__);
-file_put_contents($tmpResultCache, file_get_contents(__DIR__ . '/../../../../_files/DataproviderExecutionOrderTest_result_cache.txt'));
+\copy(__DIR__ . '/../../../../_files/DataproviderExecutionOrderTest_result_cache.txt', $tmpResultCache);
 
 $_SERVER['argv'][] = '--do-not-cache-result';
 $_SERVER['argv'][] = '--no-configuration';