CR feedback

Author: jmikola

tests/UnifiedSpecTests/CollectionData.php

@@ -41,12 +41,7 @@ public function __construct(stdClass $o)
         $this->documents = $o->documents;
     }
 
-    /**
-     * Prepare collection state for "initialData".
-     *
-     * @param Client $client
-     */
-    public function prepare(Client $client)
+    public function prepareInitialData(Client $client)
     {
         $database = $client->selectDatabase(
             $this->databaseName,
@@ -61,15 +56,9 @@ public function prepare(Client $client)
             return;
         }
 
-        $collection = $database->selectCollection($this->collectionName);
-        $collection->insertMany($this->documents);
+        $database->selectCollection($this->collectionName)->insertMany($this->documents);
     }
 
-    /**
-     * Assert collection contents for "outcome".
-     *
-     * @param Client $client
-     */
     public function assertOutcome(Client $client)
     {
         $collection = $client->selectCollection(
@@ -92,8 +81,8 @@ public function assertOutcome(Client $client)
             assertNotNull($expectedDocument);
             assertNotNull($actualDocument);
 
-            /* Disable extra root keys and operators when matching, which is
-             * effectively an exact match that allows key order variation. */
+            /* Prohibit extra root keys and disable operators to enforce exact
+             * matching of documents. Key order variation is still allowed. */
             $constraint = new Matches($expectedDocument, null, false, false);
             assertThat($actualDocument, $constraint, sprintf('documents[%d] match', $i));
         }

tests/UnifiedSpecTests/Constraint/Matches.php

@@ -43,9 +43,11 @@
 use const PHP_INT_SIZE;
 
 /**
- * Constraint that checks if one document matches another.
+ * Constraint that checks if one value matches another.
  *
- * The expected value is passed in the constructor.
+ * The expected value is passed in the constructor. An EntityMap may be supplied
+ * for resolving operators (e.g. $$matchesEntity). Behavior for allowing extra
+ * keys in root documents and processing operators is also configurable.
  */
 class Matches extends Constraint
 {
@@ -242,7 +244,7 @@ private function assertMatchesOperator(BSONDocument $operator, $actual, string $
                 '$$type requires string or string[]'
             );
 
-            $types = is_string($operator['$$type']) ? [$operator['$$type']] : $operator['$$type'];
+            $types = (array) $operator['$$type'];
             $constraints = [];
 
             foreach ($types as $type) {
@@ -286,9 +288,9 @@ private function assertMatchesOperator(BSONDocument $operator, $actual, string $
         }
 
         if ($name === '$$unsetOrMatches') {
-            /* If the operator is used at the top level, consider null
-             * values for $actual to be unset. If the operator is nested
-             * this check is done later document iteration. */
+            /* If the operator is used at the top level, consider null values
+             * for $actual to be unset. If the operator is nested, this check is
+             * done later during document iteration. */
             if ($keyPath === '' && $actual === null) {
                 return;
             }

tests/UnifiedSpecTests/Context.php

@@ -15,8 +15,6 @@
 use function array_fill_keys;
 use function array_key_exists;
 use function array_keys;
-use function assertArrayHasKey;
-use function assertArrayNotHasKey;
 use function assertContains;
 use function assertCount;
 use function assertEmpty;
@@ -83,7 +81,6 @@ public function createEntities(array $entities)
 
             $id = $def->id ?? null;
             assertInternalType('string', $id);
-            assertArrayNotHasKey($id, $this->entityMap);
 
             switch ($type) {
                 case 'client':
@@ -128,7 +125,6 @@ public function prepareOperationArguments(array $args) : array
 
         if (array_key_exists('session', $args)) {
             assertInternalType('string', $args['session']);
-            assertArrayHasKey($args['session'], $this->entityMap);
             $session = $this->entityMap[$args['session']];
             assertInstanceOf(Session::class, $session);
             $args['session'] = $session;
@@ -212,7 +208,6 @@ private function createCollection(stdClass $o) : Collection
 
         assertInternalType('string', $collectionName);
         assertInternalType('string', $database);
-        assertArrayHasKey($database, $this->entityMap);
 
         $database = $this->entityMap[$database];
         assertInstanceOf(Database::class, $database);
@@ -236,7 +231,6 @@ private function createDatabase(stdClass $o) : Database
 
         assertInternalType('string', $databaseName);
         assertInternalType('string', $client);
-        assertArrayHasKey($client, $this->entityMap);
 
         $client = $this->entityMap[$client];
         assertInstanceOf(Client::class, $client);

tests/UnifiedSpecTests/RunOnRequirement.php

@@ -48,14 +48,7 @@ public function __construct(stdClass $o)
         }
     }
 
-    /**
-     * Checks if the requirements are satisfied.
-     *
-     * @param string $serverVersion
-     * @param string $topology
-     * @return boolean
-     */
-    public function isSatisfied(string $serverVersion, string $topology)
+    public function isSatisfied(string $serverVersion, string $topology) : bool
     {
         if (isset($this->minServerVersion) && version_compare($serverVersion, $this->minServerVersion, '<')) {
             return false;

tests/UnifiedSpecTests/UnifiedSpecTest.php

@@ -225,11 +225,7 @@ private function getCachedTopology()
      */
     private function isSchemaVersionSupported($schemaVersion)
     {
-        if (version_compare($schemaVersion, '1.0', '>=') && version_compare($schemaVersion, '1.1', '<')) {
-            return true;
-        }
-
-        return false;
+        return version_compare($schemaVersion, '1.0', '>=') && version_compare($schemaVersion, '1.1', '<');
     }
 
     /**
@@ -277,7 +273,7 @@ private function prepareInitialData(array $initialData)
 
         foreach ($initialData as $data) {
             $collectionData = new CollectionData($data);
-            $collectionData->prepare(self::$internalClient);
+            $collectionData->prepareInitialData(self::$internalClient);
         }
     }
 }