Skip to content

PHPLIB-462: ensure compatibility with PHP 7.4 #665

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Aug 20, 2019
Merged

PHPLIB-462: ensure compatibility with PHP 7.4 #665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1c74f44
PHPLIB-462: Allow installing the latest PHPUnit release
alcaeus Aug 9, 2019
d7aed71
PHPLIB-462: Fix PHPUnit hook methods for different versions
alcaeus Aug 9, 2019
4fed85f
PHPLIB-462: Add compatibility layer for constraint changes
alcaeus Aug 9, 2019
8241809
PHPLIB-462: Add compatibility traits for the TestCase class
alcaeus Aug 9, 2019
4f7986a
PHPLIB-462: Replace assertInternalType with type-specific methods
alcaeus Aug 9, 2019
e0c6531
PHPLIB-462: require tests on PHP 7.4 to pass
alcaeus Aug 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ vendor/
# PHPUnit
phpunit.phar
phpunit.xml
.phpunit.result.cache
3 changes: 0 additions & 3 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@ env:
- COMPOSER_OPTIONS=

jobs:
allow_failures:
- php: "7.4snapshot"

include:

- stage: Smoke Testing
Expand Down
3 changes: 2 additions & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
"ext-mongodb": "^1.6"
},
"require-dev": {
"phpunit/phpunit": "^5.7.27 || ^6.4"
"phpunit/phpunit": "^5.7.27 || ^6.4 || ^8.3",
"symfony/phpunit-bridge": "^4.4@dev"
},
"autoload": {
"psr-4": { "MongoDB\\": "src/" },
Expand Down
11 changes: 3 additions & 8 deletions phpunit.xml.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,10 @@

<phpunit
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="http://schema.phpunit.de/4.3/phpunit.xsd"
backupGlobals="false"
backupStaticAttributes="false"
xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/8.3/phpunit.xsd"
beStrictAboutOutputDuringTests="true"
beStrictAboutChangesToGlobalState="true"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I correct that this option was introduced in 4.6 (see: blog post), even though it was never documented? I'm not sure about beStrictAboutOutputDuringTests, but there are some online references to that dating back to 2015 so I assume that's fine.

Just wanted to confirm that these will apply to all PHPUnit versions we use for testing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The XSD option was added in sebastianbergmann/phpunit@9101f51#diff-365e6053dce410a51198fc699549f0ae, which was first included in 4.0.0. The comments in the code indicate that the functionality itself was added in 3.8.0, so we should be safe here.

The reason this was added is that I regenerated a new configuration file using phpunit --generate-configuration to account for changes in the default configuration.

colors="true"
convertErrorsToExceptions="true"
convertNoticesToExceptions="true"
convertWarningsToExceptions="true"
stopOnFailure="false"
syntaxCheck="false"
bootstrap="tests/bootstrap.php"
>

Expand Down
5 changes: 4 additions & 1 deletion tests/ClientFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,18 @@
use MongoDB\Driver\BulkWrite;
use MongoDB\Driver\Command;
use MongoDB\Model\DatabaseInfo;
use Symfony\Bridge\PhpUnit\SetUpTearDownTrait;

/**
* Functional tests for the Client class.
*/
class ClientFunctionalTest extends FunctionalTestCase
{
use SetUpTearDownTrait;

private $client;

public function setUp()
private function doSetUp()
{
parent::setUp();

Expand Down
8 changes: 4 additions & 4 deletions tests/ClientTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public function testSelectCollectionInheritsOptions()
$this->assertSame(ReadConcern::LOCAL, $debug['readConcern']->getLevel());
$this->assertInstanceOf(\MongoDB\Driver\ReadPreference::class, $debug['readPreference']);
$this->assertSame(ReadPreference::RP_SECONDARY_PREFERRED, $debug['readPreference']->getMode());
$this->assertInternalType('array', $debug['typeMap']);
$this->assertIsArray($debug['typeMap']);
$this->assertSame(['root' => 'array'], $debug['typeMap']);
$this->assertInstanceOf(\MongoDB\Driver\WriteConcern::class, $debug['writeConcern']);
$this->assertSame(WriteConcern::MAJORITY, $debug['writeConcern']->getW());
Expand All @@ -90,7 +90,7 @@ public function testSelectCollectionPassesOptions()
$this->assertSame(ReadConcern::LOCAL, $debug['readConcern']->getLevel());
$this->assertInstanceOf(\MongoDB\Driver\ReadPreference::class, $debug['readPreference']);
$this->assertSame(ReadPreference::RP_SECONDARY_PREFERRED, $debug['readPreference']->getMode());
$this->assertInternalType('array', $debug['typeMap']);
$this->assertIsArray($debug['typeMap']);
$this->assertSame(['root' => 'array'], $debug['typeMap']);
$this->assertInstanceOf(\MongoDB\Driver\WriteConcern::class, $debug['writeConcern']);
$this->assertSame(WriteConcern::MAJORITY, $debug['writeConcern']->getW());
Expand Down Expand Up @@ -129,7 +129,7 @@ public function testSelectDatabaseInheritsOptions()
$this->assertSame(ReadConcern::LOCAL, $debug['readConcern']->getLevel());
$this->assertInstanceOf(\MongoDB\Driver\ReadPreference::class, $debug['readPreference']);
$this->assertSame(ReadPreference::RP_SECONDARY_PREFERRED, $debug['readPreference']->getMode());
$this->assertInternalType('array', $debug['typeMap']);
$this->assertIsArray($debug['typeMap']);
$this->assertSame(['root' => 'array'], $debug['typeMap']);
$this->assertInstanceOf(\MongoDB\Driver\WriteConcern::class, $debug['writeConcern']);
$this->assertSame(WriteConcern::MAJORITY, $debug['writeConcern']->getW());
Expand All @@ -152,7 +152,7 @@ public function testSelectDatabasePassesOptions()
$this->assertSame(ReadConcern::LOCAL, $debug['readConcern']->getLevel());
$this->assertInstanceOf(\MongoDB\Driver\ReadPreference::class, $debug['readPreference']);
$this->assertSame(ReadPreference::RP_SECONDARY_PREFERRED, $debug['readPreference']->getMode());
$this->assertInternalType('array', $debug['typeMap']);
$this->assertIsArray($debug['typeMap']);
$this->assertSame(['root' => 'array'], $debug['typeMap']);
$this->assertInstanceOf(\MongoDB\Driver\WriteConcern::class, $debug['writeConcern']);
$this->assertSame(WriteConcern::MAJORITY, $debug['writeConcern']->getW());
Expand Down
4 changes: 2 additions & 2 deletions tests/Collection/CollectionFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ public function testWithOptionsInheritsOptions()
$this->assertSame(ReadConcern::LOCAL, $debug['readConcern']->getLevel());
$this->assertInstanceOf(\MongoDB\Driver\ReadPreference::class, $debug['readPreference']);
$this->assertSame(ReadPreference::RP_SECONDARY_PREFERRED, $debug['readPreference']->getMode());
$this->assertInternalType('array', $debug['typeMap']);
$this->assertIsArray($debug['typeMap']);
$this->assertSame(['root' => 'array'], $debug['typeMap']);
$this->assertInstanceOf(\MongoDB\Driver\WriteConcern::class, $debug['writeConcern']);
$this->assertSame(WriteConcern::MAJORITY, $debug['writeConcern']->getW());
Expand All @@ -284,7 +284,7 @@ public function testWithOptionsPassesOptions()
$this->assertSame(ReadConcern::LOCAL, $debug['readConcern']->getLevel());
$this->assertInstanceOf(\MongoDB\Driver\ReadPreference::class, $debug['readPreference']);
$this->assertSame(ReadPreference::RP_SECONDARY_PREFERRED, $debug['readPreference']->getMode());
$this->assertInternalType('array', $debug['typeMap']);
$this->assertIsArray($debug['typeMap']);
$this->assertSame(['root' => 'array'], $debug['typeMap']);
$this->assertInstanceOf(\MongoDB\Driver\WriteConcern::class, $debug['writeConcern']);
$this->assertSame(WriteConcern::MAJORITY, $debug['writeConcern']->getW());
Expand Down
15 changes: 9 additions & 6 deletions tests/Collection/CrudSpecFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use IteratorIterator;
use LogicException;
use MultipleIterator;
use Symfony\Bridge\PhpUnit\SetUpTearDownTrait;

/**
* CRUD spec functional tests.
Expand All @@ -19,9 +20,11 @@
*/
class CrudSpecFunctionalTest extends FunctionalTestCase
{
use SetUpTearDownTrait;

private $expectedCollection;

public function setUp()
private function doSetUp()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, can we still chain to parent::setUp() below despite this method being renamed to doSetUp()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is still possible. The parent::setUp() call in doSetUp will always go to the parent class. In this case, the parent (FunctionalTestCase) also includes the trait, so the call will again land in the shim and call the doSetUp method that is written in FunctionalTestCase. Chaining the setup/teardown methods is still possible as before, we just have to remember to use the polyfill trait and write doSetUp methods until we can get rid of PHPUnit < 8.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm: the trait has been designed so that only the signature of the method is changed, and not its body.

{
parent::setUp();

Expand Down Expand Up @@ -297,7 +300,7 @@ private function executeAssertResult(array $operation, $expectedResult, $actualR
break;

case 'bulkWrite':
$this->assertInternalType('array', $expectedResult);
$this->assertIsArray($expectedResult);
$this->assertInstanceOf(\MongoDB\BulkWriteResult::class, $actualResult);

if (isset($expectedResult['deletedCount'])) {
Expand Down Expand Up @@ -354,7 +357,7 @@ private function executeAssertResult(array $operation, $expectedResult, $actualR

case 'deleteMany':
case 'deleteOne':
$this->assertInternalType('array', $expectedResult);
$this->assertIsArray($expectedResult);
$this->assertInstanceOf(\MongoDB\DeleteResult::class, $actualResult);

if (isset($expectedResult['deletedCount'])) {
Expand All @@ -372,7 +375,7 @@ private function executeAssertResult(array $operation, $expectedResult, $actualR
break;

case 'insertMany':
$this->assertInternalType('array', $expectedResult);
$this->assertIsArray($expectedResult);
$this->assertInstanceOf(\MongoDB\InsertManyResult::class, $actualResult);

if (isset($expectedResult['insertedCount'])) {
Expand All @@ -388,7 +391,7 @@ private function executeAssertResult(array $operation, $expectedResult, $actualR
break;

case 'insertOne':
$this->assertInternalType('array', $expectedResult);
$this->assertIsArray($expectedResult);
$this->assertInstanceOf(\MongoDB\InsertOneResult::class, $actualResult);

if (isset($expectedResult['insertedCount'])) {
Expand All @@ -406,7 +409,7 @@ private function executeAssertResult(array $operation, $expectedResult, $actualR
case 'replaceOne':
case 'updateMany':
case 'updateOne':
$this->assertInternalType('array', $expectedResult);
$this->assertIsArray($expectedResult);
$this->assertInstanceOf(\MongoDB\UpdateResult::class, $actualResult);

if (isset($expectedResult['matchedCount'])) {
Expand Down
7 changes: 5 additions & 2 deletions tests/Collection/FunctionalTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,18 @@
use MongoDB\Collection;
use MongoDB\Driver\WriteConcern;
use MongoDB\Tests\FunctionalTestCase as BaseFunctionalTestCase;
use Symfony\Bridge\PhpUnit\SetUpTearDownTrait;

/**
* Base class for Collection functional tests.
*/
abstract class FunctionalTestCase extends BaseFunctionalTestCase
{
use SetUpTearDownTrait;

protected $collection;

public function setUp()
private function doSetUp()
{
parent::setUp();

Expand All @@ -22,7 +25,7 @@ public function setUp()
$this->dropCollection();
}

public function tearDown()
private function doTearDown()
{
if ($this->hasFailed()) {
return;
Expand Down
25 changes: 25 additions & 0 deletions tests/Compat/PolyfillAssertTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php

namespace MongoDB\Tests\Compat;

use PHPUnit\Framework\Assert;
use ReflectionClass;
use Symfony\Bridge\PhpUnit\Legacy\PolyfillAssertTrait as SymfonyPolyfillAssertTrait;

$r = new ReflectionClass(Assert::class);
if (! $r->hasMethod('assertEqualsWithDelta')) {
/**
* @internal
*/
trait PolyfillAssertTrait
{
use SymfonyPolyfillAssertTrait;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason that PolyfillAssertTrait doesn't have a compat shim, like SetUpTearDownTrait?

Is that because it really is only relevant to older PHPUnit versions, so Symfony is careful not to use it in newer branches?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked to @nicolas-grekas about this. The assertion polypill is marked as @internal because they only use it to patch PHPUnit classes in their simple-phpunit binary: https://github.com/symfony/symfony/blob/a9ace363899373f4fed96f32cd38fc94187666d2/src/Symfony/Bridge/PhpUnit/bin/simple-phpunit.php#L154..L157. With the myriad of PHPUnit versions Symfony needs to support, it's too risky for them to have a single trait and let people use that in classes extending from PHPUnit\Framework\Assert, as it would then override the methods already shipped by PHPUnit. Patching the original class works around this issue as the code in PHPUnit\Framework\Assert then overrides the trait code.

In our case, I didn't want to patch PHPUnit code and with the PHPUnit versions we currently support, the single trait is good enough for us as all new assertion methods were added in or after PHPUnit 7. That's why we declare our own trait, which is empty if the PHPUnit version is new enough, as it allows us to always include our polyfill trait in the test classes.

There is still a certain risk using internal code from Symfony, but given that they need to keep the trait around as long as they support PHPUnit < 7 I feel rather confident there: dropping older PHPUnit versions for them won't be possible until Symfony 4.4 is EOL, which will be in November 2023. I hope we'll be able to get rid of support for PHP 5 before then. However, if you'd rather not use an internal class we can always duplicate the polyfill to our library, of course keeping the original copyright intact.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or you could use simple-phpunit :)
But I agree, the risk is low.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the deep explanation! I'm content with this.

@nicolas-grekas: Thanks for chiming in. simple-phpunit looks interesting (first I've seen of it) and is probably something (along with the bridge) that I could have benefited from a good while back 😄. I'll defer to @alcaeus about whether it's worth migrating over to that at some later point.

}
} else {
/**
* @internal
*/
trait PolyfillAssertTrait
{
}
}
10 changes: 5 additions & 5 deletions tests/Database/DatabaseFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public function testCommandAppliesTypeMapToCursor()
$commandResult = current($cursor->toArray());

$this->assertCommandSucceeded($commandResult);
$this->assertInternalType('array', $commandResult);
$this->assertIsArray($commandResult);
$this->assertArrayHasKey('ismaster', $commandResult);
$this->assertTrue($commandResult['ismaster']);
}
Expand Down Expand Up @@ -207,7 +207,7 @@ public function testSelectCollectionInheritsOptions()
$this->assertSame(ReadConcern::LOCAL, $debug['readConcern']->getLevel());
$this->assertInstanceOf(\MongoDB\Driver\ReadPreference::class, $debug['readPreference']);
$this->assertSame(ReadPreference::RP_SECONDARY_PREFERRED, $debug['readPreference']->getMode());
$this->assertInternalType('array', $debug['typeMap']);
$this->assertIsArray($debug['typeMap']);
$this->assertSame(['root' => 'array'], $debug['typeMap']);
$this->assertInstanceOf(\MongoDB\Driver\WriteConcern::class, $debug['writeConcern']);
$this->assertSame(WriteConcern::MAJORITY, $debug['writeConcern']->getW());
Expand All @@ -229,7 +229,7 @@ public function testSelectCollectionPassesOptions()
$this->assertSame(ReadConcern::LOCAL, $debug['readConcern']->getLevel());
$this->assertInstanceOf(\MongoDB\Driver\ReadPreference::class, $debug['readPreference']);
$this->assertSame(ReadPreference::RP_SECONDARY_PREFERRED, $debug['readPreference']->getMode());
$this->assertInternalType('array', $debug['typeMap']);
$this->assertIsArray($debug['typeMap']);
$this->assertSame(['root' => 'array'], $debug['typeMap']);
$this->assertInstanceOf(\MongoDB\Driver\WriteConcern::class, $debug['writeConcern']);
$this->assertSame(WriteConcern::MAJORITY, $debug['writeConcern']->getW());
Expand Down Expand Up @@ -303,7 +303,7 @@ public function testWithOptionsInheritsOptions()
$this->assertSame(ReadConcern::LOCAL, $debug['readConcern']->getLevel());
$this->assertInstanceOf(\MongoDB\Driver\ReadPreference::class, $debug['readPreference']);
$this->assertSame(ReadPreference::RP_SECONDARY_PREFERRED, $debug['readPreference']->getMode());
$this->assertInternalType('array', $debug['typeMap']);
$this->assertIsArray($debug['typeMap']);
$this->assertSame(['root' => 'array'], $debug['typeMap']);
$this->assertInstanceOf(\MongoDB\Driver\WriteConcern::class, $debug['writeConcern']);
$this->assertSame(WriteConcern::MAJORITY, $debug['writeConcern']->getW());
Expand All @@ -325,7 +325,7 @@ public function testWithOptionsPassesOptions()
$this->assertSame(ReadConcern::LOCAL, $debug['readConcern']->getLevel());
$this->assertInstanceOf(\MongoDB\Driver\ReadPreference::class, $debug['readPreference']);
$this->assertSame(ReadPreference::RP_SECONDARY_PREFERRED, $debug['readPreference']->getMode());
$this->assertInternalType('array', $debug['typeMap']);
$this->assertIsArray($debug['typeMap']);
$this->assertSame(['root' => 'array'], $debug['typeMap']);
$this->assertInstanceOf(\MongoDB\Driver\WriteConcern::class, $debug['writeConcern']);
$this->assertSame(WriteConcern::MAJORITY, $debug['writeConcern']->getW());
Expand Down
5 changes: 4 additions & 1 deletion tests/Database/FunctionalTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,18 @@

use MongoDB\Database;
use MongoDB\Tests\FunctionalTestCase as BaseFunctionalTestCase;
use Symfony\Bridge\PhpUnit\SetUpTearDownTrait;

/**
* Base class for Database functional tests.
*/
abstract class FunctionalTestCase extends BaseFunctionalTestCase
{
use SetUpTearDownTrait;

protected $database;

public function setUp()
private function doSetUp()
{
parent::setUp();

Expand Down
9 changes: 6 additions & 3 deletions tests/DocumentationExamplesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use MongoDB\Driver\WriteConcern;
use MongoDB\Driver\Exception\ConnectionTimeoutException;
use MongoDB\Operation\DropCollection;
use Symfony\Bridge\PhpUnit\SetUpTearDownTrait;

/**
* Documentation examples to be parsed for inclusion in the MongoDB manual.
Expand All @@ -21,14 +22,16 @@
*/
class DocumentationExamplesTest extends FunctionalTestCase
{
public function setUp()
use SetUpTearDownTrait;

private function doSetUp()
{
parent::setUp();

$this->dropCollection();
}

public function tearDown()
private function doTearDown()
{
if ($this->hasFailed()) {
return;
Expand Down Expand Up @@ -495,7 +498,7 @@ public function testExample_38_41()

$this->assertSame(2, $insertManyResult->getInsertedCount());
foreach ($insertManyResult->getInsertedIds() as $id) {
$this->assertInternalType('int', $id);
$this->assertIsInt($id);
}
$this->assertInventoryCount(2);

Expand Down
7 changes: 5 additions & 2 deletions tests/FunctionalTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,26 @@
use MongoDB\Operation\DropCollection;
use InvalidArgumentException;
use stdClass;
use Symfony\Bridge\PhpUnit\SetUpTearDownTrait;
use UnexpectedValueException;

abstract class FunctionalTestCase extends TestCase
{
use SetUpTearDownTrait;

protected $manager;

private $configuredFailPoints = [];

public function setUp()
private function doSetUp()
{
parent::setUp();

$this->manager = new Manager(static::getUri());
$this->configuredFailPoints = [];
}

public function tearDown()
private function doTearDown()
{
$this->disableFailPoints();

Expand Down
2 changes: 1 addition & 1 deletion tests/GridFS/BucketFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,7 @@ public function testOpenDownloadStreamAndMultipleReadOperations($input)
$expectedReadLength = min(4096, strlen($input) - strlen($buffer));
$buffer .= $read = fread($stream, 4096);

$this->assertInternalType('string', $read);
$this->assertIsString($read);
$this->assertEquals($expectedReadLength, strlen($read));
}

Expand Down
Loading