Skip to content

PHPLIB-530: Test against MongoDB 4.3 #723

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

Merged
merged 5 commits into from
Feb 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,12 @@ jobs:
env:
- SERVER_VERSION=4.0.12

# Test upcoming server version
- stage: Test
php: "7.3"
env:
- SERVER_VERSION=4.3.3

# Test other server configurations
- stage: Test
php: "7.3"
Expand Down
4 changes: 2 additions & 2 deletions src/MapReduceResult.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ class MapReduceResult implements IteratorAggregate
public function __construct(callable $getIterator, stdClass $result)
{
$this->getIterator = $getIterator;
$this->executionTimeMS = (integer) $result->timeMillis;
$this->counts = (array) $result->counts;
$this->executionTimeMS = isset($result->timeMillis) ? (integer) $result->timeMillis : 0;
$this->counts = isset($result->counts) ? (array) $result->counts : [];
$this->timing = isset($result->timing) ? (array) $result->timing : [];
}

Expand Down
22 changes: 21 additions & 1 deletion src/Model/CollectionInfoCommandIterator.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
namespace MongoDB\Model;

use IteratorIterator;
use Traversable;

/**
* CollectionInfoIterator for listCollections command results.
Expand All @@ -32,6 +33,19 @@
*/
class CollectionInfoCommandIterator extends IteratorIterator implements CollectionInfoIterator
{
/** @var string|null */
private $databaseName;

/**
* @param string|null $databaseName
Copy link
Member

Choose a reason for hiding this comment

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

This doc block is inconsistent with the params.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the first argument is typed, it does not need a doc block to further clarify the type of the argument. I can add the @param annotation, but it doesn't add any value to the method.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I didn't realize this was just intended to document the union type. SGTM.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep: we can't use nullable types yet as they were only introduced in PHP 7.1.

*/
public function __construct(Traversable $iterator, $databaseName = null)
{
parent::__construct($iterator);

$this->databaseName = $databaseName;
}

/**
* Return the current element as a CollectionInfo instance.
*
Expand All @@ -41,6 +55,12 @@ class CollectionInfoCommandIterator extends IteratorIterator implements Collecti
*/
public function current()
{
return new CollectionInfo(parent::current());
$info = parent::current();

if ($this->databaseName !== null && isset($info['idIndex']) && ! isset($info['idIndex']['ns'])) {
$info['idIndex']['ns'] = $this->databaseName . '.' . $info['name'];
Copy link
Member

Choose a reason for hiding this comment

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

Is this related to PHPLIB-499 (#718)? If so, I'm curious if the spec change needs to be revised. mongodb/specifications@8ed5d56 for SPEC-1399 only addressed the listIndexes, but I gather this pertains to some behavioral change in listCollections.

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, which is why I missed this in the original PR. listCollections returns information about the _id index, which also omits the ns key in 4.4+. I've created SPEC-1584 to track this and will add this to the spec.

}

return new CollectionInfo($info);
}
}
2 changes: 1 addition & 1 deletion src/Operation/ListCollections.php
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,6 @@ private function executeCommand(Server $server)
$cursor = $server->executeReadCommand($this->databaseName, new Command($cmd), $this->createOptions());
$cursor->setTypeMap(['root' => 'array', 'document' => 'array']);

return new CollectionInfoCommandIterator(new CachingIterator($cursor));
return new CollectionInfoCommandIterator(new CachingIterator($cursor), $this->databaseName);
}
}
6 changes: 4 additions & 2 deletions tests/Collection/CollectionFunctionalTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -396,8 +396,10 @@ public function testMapReduce()

$this->assertSameDocuments($expected, $result);

$this->assertGreaterThanOrEqual(0, $result->getExecutionTimeMS());
$this->assertNotEmpty($result->getCounts());
if (version_compare($this->getServerVersion(), '4.3.0', '<')) {
$this->assertGreaterThanOrEqual(0, $result->getExecutionTimeMS());
$this->assertNotEmpty($result->getCounts());
}
}

public function collectionMethodClosures()
Expand Down
38 changes: 33 additions & 5 deletions tests/Operation/MapReduceFunctionalTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
use MongoDB\Operation\Find;
use MongoDB\Operation\MapReduce;
use MongoDB\Tests\CommandObserver;
use function is_object;
use function iterator_to_array;
use function usort;
use function version_compare;

class MapReduceFunctionalTest extends FunctionalTestCase
Expand Down Expand Up @@ -92,12 +94,19 @@ public function testResult()
$result = $operation->execute($this->getPrimaryServer());

$this->assertInstanceOf(MapReduceResult::class, $result);
$this->assertGreaterThanOrEqual(0, $result->getExecutionTimeMS());
$this->assertNotEmpty($result->getCounts());

if (version_compare($this->getServerVersion(), '4.3.0', '<')) {
$this->assertGreaterThanOrEqual(0, $result->getExecutionTimeMS());
$this->assertNotEmpty($result->getCounts());
}
}

public function testResultIncludesTimingWithVerboseOption()
{
if (version_compare($this->getServerVersion(), '4.3.0', '>=')) {
$this->markTestSkipped('mapReduce statistics are no longer exposed');
}

$this->createFixtures(3);

$map = new Javascript('function() { emit(this.x, this.y); }');
Expand All @@ -115,6 +124,10 @@ public function testResultIncludesTimingWithVerboseOption()

public function testResultDoesNotIncludeTimingWithoutVerboseOption()
{
if (version_compare($this->getServerVersion(), '4.3.0', '>=')) {
$this->markTestSkipped('mapReduce statistics are no longer exposed');
}

$this->createFixtures(3);

$map = new Javascript('function() { emit(this.x, this.y); }');
Expand Down Expand Up @@ -226,7 +239,7 @@ public function testTypeMapOptionWithInlineResults(array $typeMap = null, array
$operation = new MapReduce($this->getDatabaseName(), $this->getCollectionName(), $map, $reduce, $out, ['typeMap' => $typeMap]);
$results = iterator_to_array($operation->execute($this->getPrimaryServer()));

$this->assertEquals($expectedDocuments, $results);
$this->assertEquals($this->sortResults($expectedDocuments), $this->sortResults($results));
}

public function provideTypeMapOptionsAndExpectedDocuments()
Expand Down Expand Up @@ -273,12 +286,12 @@ public function testTypeMapOptionWithOutputCollection(array $typeMap = null, arr
$operation = new MapReduce($this->getDatabaseName(), $this->getCollectionName(), $map, $reduce, $out, ['typeMap' => $typeMap]);
$results = iterator_to_array($operation->execute($this->getPrimaryServer()));

$this->assertEquals($expectedDocuments, $results);
$this->assertEquals($this->sortResults($expectedDocuments), $this->sortResults($results));

$operation = new Find($this->getDatabaseName(), $out, [], ['typeMap' => $typeMap]);
$cursor = $operation->execute($this->getPrimaryServer());

$this->assertEquals($expectedDocuments, iterator_to_array($cursor));
$this->assertEquals($this->sortResults($expectedDocuments), $this->sortResults(iterator_to_array($cursor)));

$operation = new DropCollection($this->getDatabaseName(), $out);
$operation->execute($this->getPrimaryServer());
Expand All @@ -302,4 +315,19 @@ private function createFixtures($n)

$this->assertEquals($n * 2, $result->getInsertedCount());
}

private function sortResults(array $results) : array
{
$sortFunction = static function ($resultA, $resultB) : int {
$idA = is_object($resultA) ? $resultA->_id : $resultA['_id'];
$idB = is_object($resultB) ? $resultB->_id : $resultB['_id'];

return $idA <=> $idB;
};

$sortedResults = $results;
usort($sortedResults, $sortFunction);

return $sortedResults;
}
}
11 changes: 11 additions & 0 deletions tests/SpecTests/CrudSpecTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@
*/
class CrudSpecTest extends FunctionalTestCase
{
/** @var array */
private static $incompleteTests = [
'find-allowdiskuse: Find does not send allowDiskuse when value is not specified' => 'PHPLIB-500',
Copy link
Member

Choose a reason for hiding this comment

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

Noted that this depends on #721, which in turn is waiting for this PR to get merged.

'find-allowdiskuse: Find sends allowDiskuse false when false is specified' => 'PHPLIB-500',
'find-allowdiskuse: Find sends allowDiskUse true when true is specified' => 'PHPLIB-500',
];

/**
* Assert that the expected and actual command documents match.
*
Expand All @@ -37,6 +44,10 @@ public static function assertCommandMatches(stdClass $expected, stdClass $actual
*/
public function testCrud(stdClass $test, array $runOn = null, array $data, $databaseName = null, $collectionName = null)
{
if (isset(self::$incompleteTests[$this->dataDescription()])) {
$this->markTestIncomplete(self::$incompleteTests[$this->dataDescription()]);
}

if (isset($runOn)) {
$this->checkServerRequirements($runOn);
}
Expand Down