Deprecate Prophecy integration

[sebastianbergmann]

5 years ago, with version 4.5, PHPUnit started to bundle phpspec/prophecy as an alternative to PHPUnit's own test double functionality.

In the context of the above, "bundling" means that

• phpspec/prophecy is listed as a dependency in PHPUnit's composer.json file
• phpunit.phar, the PHAR distribution of PHPUnit, contains Prophecy as well as its dependencies
• PHPUnit\Framework\TestCase provides the prophesize() method to create a test double using Prophecy (allowing TestCase to automatically verify Prophecy expectations at the end of the test while also providing integration with PHPUnit's risky test analysis)

5 years ago, I wrote in an article

at some point in the future we will encourage developers to use Prophecy's API instead of the "classic" PHPUnit API

This has not happended so far and I now doubt that it will.

phpspec/prophecy depends on the sebastian/comparator and sebastian/recursion-context components that are used by PHPUnit. And since PHPUnit currently depends on phpspec/prophecy we have circular dependencies.

These circular dependencies are especially annoying whenever I bump the major version of sebastian/comparator and sebastian/recursion-context because @ciaranmcnulty needs to make the same change in phpspec/prophecy at the same time (otherwise PHPUnit breaks).

It is because of this circular dependency that I am beginning to think that the tight integration of Prophecy into PHPUnit causes more harm than good.

I would like to deprecate PHPUnit\Framework\TestCase::prophesize() (and any associated functionality) in PHPUnit 9.X and remove it in PHPUnit 10.

[sebastianbergmann]

Sketch of what unbundling would entail:

diff --git a/build.xml b/build.xml
index 054b1d1e2..88bba98a0 100644
--- a/build.xml
+++ b/build.xml
@@ -284,54 +284,12 @@
             </fileset>
         </copy>
 
-        <copy file="${basedir}/vendor/phpdocumentor/reflection-common/LICENSE" tofile="${basedir}/build/tmp/phar/phpdocumentor-reflection-common/LICENSE"/>
-        <copy todir="${basedir}/build/tmp/phar/phpdocumentor-reflection-common">
-            <fileset dir="${basedir}/vendor/phpdocumentor/reflection-common/src">
-                <include name="**/*.php" />
-            </fileset>
-        </copy>
-
-        <copy file="${basedir}/vendor/phpdocumentor/reflection-docblock/LICENSE" tofile="${basedir}/build/tmp/phar/phpdocumentor-reflection-docblock/LICENSE"/>
-        <copy todir="${basedir}/build/tmp/phar/phpdocumentor-reflection-docblock">
-            <fileset dir="${basedir}/vendor/phpdocumentor/reflection-docblock/src">
-                <include name="**/*.php" />
-            </fileset>
-        </copy>
-
-        <copy file="${basedir}/vendor/phpdocumentor/type-resolver/LICENSE" tofile="${basedir}/build/tmp/phar/phpdocumentor-type-resolver/LICENSE"/>
-        <copy todir="${basedir}/build/tmp/phar/phpdocumentor-type-resolver">
-            <fileset dir="${basedir}/vendor/phpdocumentor/type-resolver/src">
-                <include name="**/*.php" />
-            </fileset>
-        </copy>
-
-        <copy file="${basedir}/vendor/phpspec/prophecy/LICENSE" tofile="${basedir}/build/tmp/phar/phpspec-prophecy/LICENSE"/>
-        <copy todir="${basedir}/build/tmp/phar/phpspec-prophecy">
-            <fileset dir="${basedir}/vendor/phpspec/prophecy/src">
-                <include name="**/*.php" />
-            </fileset>
-        </copy>
-
-        <copy file="${basedir}/vendor/symfony/polyfill-ctype/LICENSE" tofile="${basedir}/build/tmp/phar/symfony-polyfill-ctype/LICENSE"/>
-        <copy todir="${basedir}/build/tmp/phar/symfony-polyfill-ctype">
-            <fileset dir="${basedir}/vendor/symfony/polyfill-ctype">
-                <include name="**/*.php" />
-            </fileset>
-        </copy>
-
         <copy file="${basedir}/vendor/theseer/tokenizer/LICENSE" tofile="${basedir}/build/tmp/phar/theseer-tokenizer/LICENSE"/>
         <copy todir="${basedir}/build/tmp/phar/theseer-tokenizer">
             <fileset dir="${basedir}/vendor/theseer/tokenizer/src">
                 <include name="**/*.php" />
             </fileset>
         </copy>
-
-        <copy file="${basedir}/vendor/webmozart/assert/LICENSE" tofile="${basedir}/build/tmp/phar/webmozart-assert/LICENSE"/>
-        <copy todir="${basedir}/build/tmp/phar/webmozart-assert">
-            <fileset dir="${basedir}/vendor/webmozart/assert/src">
-                <include name="**/*.php" />
-            </fileset>
-        </copy>
     </target>
 
     <target name="-phar-build" depends="-phar-determine-version">
diff --git a/composer.json b/composer.json
index de6951944..64bc98069 100644
--- a/composer.json
+++ b/composer.json
@@ -33,7 +33,6 @@
         "myclabs/deep-copy": "^1.9.1",
         "phar-io/manifest": "^1.0.3",
         "phar-io/version": "^2.0.1",
-        "phpspec/prophecy": "^1.8.1",
         "phpunit/php-code-coverage": "^8.0.1",
         "phpunit/php-file-iterator": "^3.0",
         "phpunit/php-invoker": "^3.0",
diff --git a/src/Framework/TestCase.php b/src/Framework/TestCase.php
index 9a91415d2..db69c7131 100644
--- a/src/Framework/TestCase.php
+++ b/src/Framework/TestCase.php
@@ -42,10 +42,6 @@
 use PHPUnit\Util\PHP\AbstractPhpProcess;
 use PHPUnit\Util\Test as TestUtil;
 use PHPUnit\Util\Type;
-use Prophecy\Exception\Prediction\PredictionException;
-use Prophecy\Prophecy\MethodProphecy;
-use Prophecy\Prophecy\ObjectProphecy;
-use Prophecy\Prophet;
 use SebastianBergmann\Comparator\Comparator;
 use SebastianBergmann\Comparator\Factory as ComparatorFactory;
 use SebastianBergmann\Diff\Differ;
@@ -230,11 +226,6 @@ abstract class TestCase extends Assert implements SelfDescribing, Test
      */
     private $snapshot;
 
-    /**
-     * @var \Prophecy\Prophet
-     */
-    private $prophet;
-
     /**
      * @var bool
      */
@@ -1051,9 +1042,6 @@ public function runBare(): void
         } catch (AssertionFailedError $e) {
             $this->status        = BaseTestRunner::STATUS_FAILURE;
             $this->statusMessage = $e->getMessage();
-        } catch (PredictionException $e) {
-            $this->status        = BaseTestRunner::STATUS_FAILURE;
-            $this->statusMessage = $e->getMessage();
         } catch (\Throwable $_e) {
             $e                   = $_e;
             $this->status        = BaseTestRunner::STATUS_ERROR;
@@ -1061,7 +1049,6 @@ public function runBare(): void
         }
 
         $this->mockObjects = [];
-        $this->prophet     = null;
 
         // Tear down the fixture. An exception raised in tearDown() will be
         // caught and passed on when no exception was raised before.
@@ -1119,10 +1106,6 @@ public function runBare(): void
 
         // Workaround for missing "finally".
         if (isset($e)) {
-            if ($e instanceof PredictionException) {
-                $e = new AssertionFailedError($e->getMessage());
-            }
-
             $this->onNotSuccessfulTest($e);
         }
     }
@@ -1838,24 +1821,6 @@ protected function getObjectForTrait($traitName, array $arguments = [], $traitCl
         );
     }
 
-    /**
-     * @param null|string $classOrInterface
-     *
-     * @throws \Prophecy\Exception\Doubler\ClassNotFoundException
-     * @throws \Prophecy\Exception\Doubler\DoubleException
-     * @throws \Prophecy\Exception\Doubler\InterfaceNotFoundException
-     *
-     * @psalm-param class-string|null $classOrInterface
-     */
-    protected function prophesize($classOrInterface = null): ObjectProphecy
-    {
-        if (\is_string($classOrInterface)) {
-            $this->recordDoubledType($classOrInterface);
-        }
-
-        return $this->getProphet()->prophesize($classOrInterface);
-    }
-
     /**
      * Creates a default TestResult object.
      *
@@ -1929,22 +1894,6 @@ private function verifyMockObjects(): void
                 $this->shouldInvocationMockerBeReset($mockObject)
             );
         }
-
-        if ($this->prophet !== null) {
-            try {
-                $this->prophet->checkPredictions();
-            } finally {
-                foreach ($this->prophet->getProphecies() as $objectProphecy) {
-                    foreach ($objectProphecy->getMethodProphecies() as $methodProphecies) {
-                        foreach ($methodProphecies as $methodProphecy) {
-                            \assert($methodProphecy instanceof MethodProphecy);
-
-                            $this->numAssertions += \count($methodProphecy->getCheckedPredictions());
-                        }
-                    }
-                }
-            }
-        }
     }
 
     private function handleDependencies(): bool
@@ -2206,9 +2155,7 @@ private function createGlobalStateSnapshot(bool $backupGlobals): Snapshot
             $blacklist->addClassNamePrefix('SebastianBergmann\Template');
             $blacklist->addClassNamePrefix('SebastianBergmann\Timer');
             $blacklist->addClassNamePrefix('PHP_Token');
-            $blacklist->addClassNamePrefix('Symfony');
             $blacklist->addClassNamePrefix('Doctrine\Instantiator');
-            $blacklist->addClassNamePrefix('Prophecy');
 
             foreach ($this->backupStaticAttributesBlacklist as $class => $attributes) {
                 foreach ($attributes as $attribute) {
@@ -2282,15 +2229,6 @@ private function compareGlobalStateSnapshotPart(array $before, array $after, str
         }
     }
 
-    private function getProphet(): Prophet
-    {
-        if ($this->prophet === null) {
-            $this->prophet = new Prophet;
-        }
-
-        return $this->prophet;
-    }
-
     /**
      * @throws \SebastianBergmann\ObjectEnumerator\InvalidArgumentException
      */
diff --git a/src/Util/Blacklist.php b/src/Util/Blacklist.php
index ed726077d..f41860f51 100644
--- a/src/Util/Blacklist.php
+++ b/src/Util/Blacklist.php
@@ -15,11 +15,7 @@
 use PharIo\Manifest\Manifest;
 use PharIo\Version\Version as PharIoVersion;
 use PHP_Token;
-use phpDocumentor\Reflection\DocBlock;
-use phpDocumentor\Reflection\Project;
-use phpDocumentor\Reflection\Type;
 use PHPUnit\Framework\TestCase;
-use Prophecy\Prophet;
 use SebastianBergmann\CodeCoverage\CodeCoverage;
 use SebastianBergmann\CodeUnitReverseLookup\Wizard;
 use SebastianBergmann\Comparator\Comparator;
@@ -37,7 +33,6 @@
 use SebastianBergmann\Type\TypeName;
 use SebastianBergmann\Version;
 use TheSeer\Tokenizer\Tokenizer;
-use Webmozart\Assert\Assert;
 
 /**
  * @internal This class is not covered by the backward compatibility promise for PHPUnit
@@ -72,9 +67,6 @@ final class Blacklist
         // phpdocumentor/type-resolver
         Type::class => 1,
 
-        // phpspec/prophecy
-        Prophet::class => 1,
-
         // phpunit/phpunit
         TestCase::class => 2,

[theofidry]

@sebastianbergmann just a few questions :)

1. If prophecy could get rid of those dependencies removing the circular dependency issue, would your stance on the matter change?

2. Would you consider a prophecy "premium support"? For example in the coming version of infection, if you install infection (with Composer) and configure it with phpspec as a testing framework instead of PHPUnit, and run it, infection will propose you to install the phpspec adapter (that is not shipped by default) if you forgot about it which makes the experience a bit more friendly. With the PHAR, the phpspec adapter is installed by default still though

3. My main concern would be today, how can I easily upgrade? Realistically I'm not going to try to bother to replace all my prophecy usage in my applications but instead will look for a way to integrate prophecy easily in my PHPUnit test suite. Do you think there would be a way to add a bridge of some sorts to make it more friendly to use?

Note on the last point for anyone else reading this, the current way would be to change:

<?php

class UserTest extends PHPUnit\Framework\TestCase
{
    public function testPasswordHashing()
    {
        $hasher = $this->prophesize('App\Security\Hasher');
        $user   = new App\Entity\User($hasher->reveal());

        $hasher->generateHash($user, 'qwerty')->willReturn('hashed_pass');

        $user->setPassword('qwerty');

        $this->assertEquals('hashed_pass', $user->getPassword());
    }
}

to this:

<?php

class UserTest extends PHPUnit\Framework\TestCase
{
    private $prophet;

    public function testPasswordHashing()
    {
        $hasher = $this->prophet->prophesize('App\Security\Hasher');
        $user   = new App\Entity\User($hasher->reveal());

        $hasher->generateHash($user, 'qwerty')->willReturn('hashed_pass');

        $user->setPassword('qwerty');

        $this->assertEquals('hashed_pass', $user->getPassword());
    }

    protected function setUp()
    {
        $this->prophet = new \Prophecy\Prophet;
    }

    protected function tearDown()
    {
        $this->prophet->checkPredictions();
    }
}

This is not that huge of a deal mind you, but I personally find this a bit error-prone: it's easy to forget checkPredictions() for example.

Maybe the following could be added?

namespace PHPUnit\Framework;

abstract class ProphecyTestCase TestCase
{
    private $prophet;

    protected function setUp()
    {
        $this->prophet = new \Prophecy\Prophet;
    }

    protected function tearDown()
    {
        $this->prophet->checkPredictions();
    }

    final protected function prophesize($classOrInterface = null): ObjectProphecy
    {
        if ($this->prophet === null) {
            $this->prophet = new Prophet;
        }

        return $this->prophet;
    }
}

[theofidry]

Hm actually if there is a ProphecyTestCase to provide, I guess this could be added to prophecy directly instead of PHPUnit...

[ciaranmcnulty]

Makes sense to me to unbundle; if people like it they only need to add the one dependency.

I would worry that currently there aren't good, easy to find Prophecy docs so it'd be good to retain those pages in the phpunit docs until that happens

[sebastianbergmann]

@ciaranmcnulty It's not just "adding the dependency", though. Without an integration such as the one provided right now, PHPUnit will mark a test as risky if it only has expectations performed through Prophecy.

@theofidry A ProphecyTestCase, either provided as part of Prophecy or from a separate package, would need to hook into PHPUnit somehow to make the risky test check work correctly. I am not sure that this would be possible as of right now.

[lyrixx]

Could we still use this if we install it manually? I'm using it a lot 😬

[stof]

A ProphecyTestCase, either provided as part of Prophecy or from a separate package, would need to hook into PHPUnit somehow to make the risky test check work correctly. I am not sure that this would be possible as of right now.

Then, making sure it is possible should be a prerequisite of this unbundling.

After that, we could resurrect https://github.com/phpspec/prophecy-phpunit

[marcinwarzybok]

sad news. I love phpunit+prophecy :(

[jaapio]

I'm wondering if it will be possible to provide a way to use phpunit as a phar with the extension of prophecy. I do prefer the phar installs because it reduces the risks of conflicting dependencies like you are experiencing with prophecy and phpunit.

If it is possible the migration path seems feasible

[skoop]

I am assuming there has already been communication between you and @ciaranmcnulty about whether this circular dependency can be solved? If that is the only bump in the road, it would make sense to try and fix that.

I'd hate to see Prophecy integration go. Better yet, I'd love a tighter integration (if at all possible). I use Prophecy on all projects and $this->prophesize() is so strong in my muscle memory that any time I have to type $this->p I automatically start with roph before realizing I wanted something else ;)

If you will indeed deprecate the integration, would there be a chance of adding hooks to PHPUnit to ensure Prophecy or a glue package to hook into PHPUnit to at least prevent the risky test warning?

[sebastianbergmann]

I have made changes to PHPUnit\Framework\TestCase (locally, not pushed) to make the following work:

<?php declare(strict_types=1);
/*
 * This file is part of PHPUnit.
 *
 * (c) Sebastian Bergmann <sebastian@phpunit.de>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */
namespace PHPUnit\Framework;

use Prophecy\Exception\Doubler\DoubleException;
use Prophecy\Exception\Doubler\InterfaceNotFoundException;
use Prophecy\Exception\Prediction\PredictionException;
use Prophecy\Prophecy\MethodProphecy;
use Prophecy\Prophecy\ObjectProphecy;
use Prophecy\Prophet;

abstract class ProphecyTestCase extends TestCase
{
    /**
     * @var Prophet
     */
    private $prophet;

    /**
     * @throws DoubleException
     * @throws InterfaceNotFoundException
     *
     * @psalm-param class-string|null $type
     */
    protected function prophesize(?string $classOrInterface = null): ObjectProphecy
    {
        if (\is_string($classOrInterface)) {
            $this->recordDoubledType($classOrInterface);
        }

        return $this->getProphet()->prophesize($classOrInterface);
    }

    protected function verifyMockObjects(): void
    {
        parent::verifyMockObjects();

        if ($this->prophet !== null) {
            try {
                $this->prophet->checkPredictions();
            } catch (PredictionException $e) {
                throw new AssertionFailedError($e->getMessage());
            } finally {
                foreach ($this->prophet->getProphecies() as $objectProphecy) {
                    foreach ($objectProphecy->getMethodProphecies() as $methodProphecies) {
                        foreach ($methodProphecies as $methodProphecy) {
                            \assert($methodProphecy instanceof MethodProphecy);

                            $this->addToAssertionCount(\count($methodProphecy->getCheckedPredictions()));
                        }
                    }
                }
            }
        }
    }

    private function getProphet(): Prophet
    {
        if ($this->prophet === null) {
            $this->prophet = new Prophet;
        }

        return $this->prophet;
    }
}

[sebastianbergmann]

@stof @ciaranmcnulty Where should ProphecyTestCase live?

[sebastianbergmann]

This discussion / these changes might also be of interest to @davedevelopment, @robertbasic, @padraic et. al. for mockery/mockery as well as to @mlively for mlively/Phake.

[sebastianbergmann]

After that, we could resurrect https://github.com/phpspec/prophecy-phpunit

@stof Yes, I think that would be best.

[sebastianbergmann]

@stof @ciaranmcnulty I have forked your phpspec/prophecy-phpunit repository that is currently archived. I have committed the implementation of ProphecyTestCase shown in #4141 (comment) together with some housekeeping to my fork.

There are currently two open issues:

• https://github.com/sebastianbergmann/prophecy-phpunit/issues/1 is about the test for failing spies failing
• https://github.com/sebastianbergmann/prophecy-phpunit/issues/2 is about adding more documentation

I will work on https://github.com/sebastianbergmann/prophecy-phpunit/issues/2 ASAP but probably need help with https://github.com/sebastianbergmann/prophecy-phpunit/issues/1.

Can you please unarchive phpspec/prophecy-phpunit so that I can send a pull request? I do not want to own this code. I will, of course, continue to help with making this work as a replacement for PHPUnit's bundled integration of Prophecy.

[sebastianbergmann]

@TomasVotruba Eventually a rector would be nice that looks for classes that extend PHPUnit\Framework\TestCase and call $this->prophesize(). The parent class of such a class needs to be changed from PHPUnit\Framework\TestCase to Prophecy\PhpUnit\ProphecyTestCase.

[TomasVotruba]

@sebastianbergmann Sure, that can be done.
Just create an issue here with targeted PHPUnit version and before/after code: https://github.com/rectorphp/rector/issues/new/choose
I'll handle it

[lyrixx]

IMHO a trait would be simpler

[sebastianbergmann]

IMHO a trait would be simpler

A trait cannot override verifyMockObjects().

[lyrixx]

My issue is that in Symfony, the usually extends KernelTestCase (and this one extends Phpunit\Framework\TestCase). So if it's not a trait, a test will not be able to use prophecy anymore

[skoop]

My issue is that in Symfony, the usually extends KernelTestCase (and this one extends Phpunit\Framework\TestCase). So if it's not a trait, a test will not be able to use prophecy anymore

I'm pretty sure it would be possible to work with the Symfony team on getting support for the new ProphecyTestCase. I can imagine the combination of a KernelTestCase and a ProphecyKernelTestCase. I'm sure this can be solved.

[sebastianbergmann]

The pull request for phpspec/prophecy-phpunit is here.

[sebastianbergmann]

I'm wondering if it will be possible to provide a way to use phpunit as a phar with the extension of prophecy.

Sure; phpspec/prophecy-phpunit "just" needs to be made available as a PHAR. How this can be done is illustrated in the phpunit-example-extension.

I do prefer the phar installs

As do I :-)

[mfn]

Sorry to jump in here:

I do prefer the phar installs

As do I :-)

Is there a "package distribution" for phpunits phar similar to e.g. phpstan?

I would love to use the phar, but not having it in composer.json has the risk of missing updates and easily getting out of date.

Is there something like this (or did I just miss it)?

thanks!

[stof]

@mfn I think you should create a separate issue to discuss that, as it is not about deprecating prophecy anymore.

[theseer]

Installing PHPUnit or other tools as phar is not a problem: phive can do that for you (see https://phar.io for details).

Dealing with extensions is currently a problem as that opens up dependency resolving. Something that phive's infrastructure currently does not have the meta data for. That means for extensions we have to come up with a means to enable phive to resolve what version of an extension is compatible with what version of, for instance, PHPUnit - prefereably without downloading all versions first ;)

I do have some work in progress code for that but it's more like an experiment at the moment and as much as I hate it, open source work does not have priority at the moment...

[temp]

Did someone already build an extension-phar that can be loaded as Sebastian stated? @stof maybe this can be added to phpspec/prophecy-phpunit?

[temp]

Ok, got it working. Was thinking in the wrong direction first, got errors because I already added prophecy to the phar - but this should only be the case for a phpunit-10 compatible version, since phpunit-9 already has prophecy included... but works nicely, I used the build-mechanism from Sebastians phpunit-example-extension.

[TomasVotruba]

Just in case it was lost in PR references, the @rectorphp migration is ready:
rectorphp/rector#3117

tl;dr;

composer require rector/rector --dev
vendor/bin/rector process /tests --set phpunit91