[PHPLIB-1608] Implement prose tests for $lookup support
#1752
Conversation
paulinevos
force-pushed
the
feature/1608_lookup-csfe-qe
branch
from
2b3a5a9 to
ed00780
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v2.x #1752 +/- ##
=========================================
Coverage 87.72% 87.72%
Complexity 3190 3190
=========================================
Files 424 424
Lines 6348 6348
=========================================
Hits 5569 5569
Misses 779 779
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
paulinevos
force-pushed
the
feature/1608_lookup-csfe-qe
branch
from
ed00780 to
270b4ad
Compare
This reverts commit e76d9f5.
As laid out in the spec: mongodb/specifications@527e22d
paulinevos
force-pushed
the
feature/1608_lookup-csfe-qe
branch
from
270b4ad to
955121d
Compare
| $collections = [ | ||
| self::COLL_CSFLE, | ||
| self::COLL_CSFLE2, | ||
| self::COLL_QE, | ||
| self::COLL_QE2, | ||
| self::COLL_NO_SCHEMA, | ||
| self::COLL_NO_SCHEMA2, | ||
| ]; |
There was a problem hiding this comment.
Do you need this array? If you add self::COLL_NO_SCHEMA => [], self::COLL_NO_SCHEMA2 => [] to $optionsMap you can look directly on it. That would be more directly comparable to the spec.
|
|
||
| private function refreshCollections(): void | ||
| { | ||
| $db = $this->encryptedClient->selectDatabase(self::getDatabaseName()); |
There was a problem hiding this comment.
selectDatabase and selectCollection are deprecated.
You can also initialize the unencrypted Database.
| $db = $this->encryptedClient->selectDatabase(self::getDatabaseName()); | |
| $encryptedDatabase = $this->encryptedClient->getDatabase(self::getDatabaseName()); | |
| $unencryptedDatabase = $this->unencryptedClient->getDatabase(self::getDatabaseName()); |
| $collection = $this->unencryptedClient | ||
| ->selectDatabase(self::getDatabaseName()) |
There was a problem hiding this comment.
Maybe keep this Database object in a variable before the loop (see comment at the beginning of the test function).
| if ($options) { | ||
| $document = $collection->findOne(['_id' => $result->getInsertedId()]); | ||
| $this->assertInstanceOf(Binary::class, $document->{$collectionName}); | ||
| } |
There was a problem hiding this comment.
Good factorisation of the test pattern 👍🏻
| $this->encryptedClient = self::createTestClient(null, [], [ | ||
| 'autoEncryption' => $autoEncryptionOpts, | ||
| /* libmongocrypt caches results from listCollections. Use a new | ||
| * client in each test to ensure its encryptedFields is applied. */ | ||
| 'disableClientPersistence' => true, | ||
| ]); |
There was a problem hiding this comment.
Recreate
encryptedClientwith the sameAutoEncryptionOptsas the setup. (Recreating prevents schema caching from impacting the test).
I understand that one instance of Client would be needed for setup and that a new instance would need to be created for each test.
Here you use the same instance in setup and test code, even if you create a new in the setup of each test.
| $this->skipIfServerVersion('<', '8.1.0', 'Lookup test case requires server version 8.1.0 or later'); | ||
| $this->skipIfClientSideEncryptionIsNotSupported(); |
There was a problem hiding this comment.
I see that only the test 9 has a distinct constraint; that's why you did not put this check in the setUp.
| $cursor = $this->encryptedClient->selectDatabase(self::getDatabaseName()) | ||
| ->getCollection($collection) |
There was a problem hiding this comment.
You can get the collection directly from the client.
| $cursor = $this->encryptedClient->selectDatabase(self::getDatabaseName()) | |
| ->getCollection($collection) | |
| $cursor = $this->encryptedClient->getCollection(self::getDatabaseName(), $collection); |
| $this->skipIfServerVersion('<', '8.1.0', 'Lookup test case requires server version 8.1.0 or later'); | ||
| $this->skipIfClientSideEncryptionIsNotSupported(); | ||
|
|
||
| $this->expectExceptionMessageMatches('#not supported#'); |
There was a problem hiding this comment.
I think expectExceptionMessage already matches substrings. expectExceptionMessageMatches is only if you have a regex.
| $this->expectExceptionMessageMatches('#not supported#'); | |
| $this->expectExceptionMessage('not supported'); |
| $this->encryptedClient->selectDatabase(self::getDatabaseName()) | ||
| ->getCollection(self::COLL_CSFLE) |
There was a problem hiding this comment.
| $this->encryptedClient->selectDatabase(self::getDatabaseName()) | |
| ->getCollection(self::COLL_CSFLE) | |
| $this->encryptedClient->getCollection(self::getDatabaseName(), self::COLL_CSFLE) |
As laid out in the spec: mongodb/specifications@527e22d
Closes PHPLIB-1608