Skip to content
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

PHPLIB-1541: Include specs repository as a submodule #1429

Merged
merged 18 commits into from
Oct 16, 2024
Merged

PHPLIB-1541: Include specs repository as a submodule #1429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
73a17d1
Add submodule for specifications
alcaeus Sep 20, 2024
b603979
Run spec tests from submodule
alcaeus Sep 20, 2024
d1313e9
Add dependabot configuration to update spec submodule
alcaeus Sep 20, 2024
135582a
Add submodule update to contributing docs
alcaeus Sep 20, 2024
99c7d24
Initialise spec submodule through composer
alcaeus Sep 24, 2024
6244176
Add guidance for fixing spec test failures in dependabot PRs
alcaeus Sep 24, 2024
7eb4d70
PHPLIB-1458: Update aggregate-write-readPreference tests
alcaeus Oct 1, 2024
c5c0631
Update spec test paths
alcaeus Oct 1, 2024
70a38d9
Allow skipping duplicate tests in data providers
alcaeus Oct 1, 2024
fb7b3ec
Skip all clientBulkWrite tests
alcaeus Oct 1, 2024
b0269f6
PHPLIB-1450: Consolidate and improve skipping of tests
alcaeus Oct 2, 2024
b1b1260
Update to latest specs commit
alcaeus Oct 14, 2024
5615fc5
Use str_starts_with instead of regex logic
alcaeus Oct 14, 2024
1ed6e95
Add annotations to type-qualify arrays
alcaeus Oct 14, 2024
65357f4
Always require passing test group in provideTests
alcaeus Oct 14, 2024
d9fc698
Consistent wrapping
alcaeus Oct 14, 2024
af04a9c
PHPLIB-1492: Skip spec tests for sort in update operations
alcaeus Oct 15, 2024
afbcd0b
Remove obsolete imports
alcaeus Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
4 changes: 4 additions & 0 deletions .github/dependabot.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,7 @@ updates:
directory: "/"
schedule:
interval: "weekly"
- package-ecosystem: "gitsubmodule"
directory: "/"
schedule:
interval: "weekly"
1 change: 1 addition & 0 deletions .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ jobs:
uses: "actions/checkout@v4"
with:
fetch-depth: 2
submodules: true

- id: setup-mongodb
uses: mongodb-labs/drivers-evergreen-tools@master
Expand Down
3 changes: 3 additions & 0 deletions .gitmodules
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[submodule "specifications"]
path = tests/specifications
url = https://github.com/mongodb/specifications
30 changes: 30 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ $ composer update
In addition to installing project dependencies, Composer will check that the
required extension version is installed. Directions for installing the extension
may be found [here](https://php.net/manual/en/mongodb.installation.php).
Composer will also install the submodule required for running spec tests.

Installation directions for Composer may be found in its
[Getting Started](https://getcomposer.org/doc/00-intro.md) guide.
Expand Down Expand Up @@ -100,6 +101,35 @@ The following environment variables are used for [CSFLE testing](https://github.
* `KMS_TLS_CA_FILE`
* `KMS_TLS_CERTIFICATE_KEY_FILE`

### Updating spec tests

Tests from the MongoDB Specifications repository are included through a
submodule and updated automatically through Dependabot. To update tests
manually, switch to the `tests/specifications` directory and update the
repository to the appropriate commit. Remember to commit this change to the
library repository.

#### Handling test failures on updates

Failures on updates can occur for multiple reasons, and the remedy to this will
depend on the type of failure. Note that only tests for implemented
specifications are run in the test runner.

* If a specification is not fully implemented (e.g. a recent change to the spec
has not been applied yet), skip the test in question with a reference to the
ticket that covers the change
* If a test fails because it uses features not yet implemented in the unified
test runner, skip the corresponding test with a reference to the ticket that
covers implementing the new features
* If the test failure points to a bug in the spec, consider the effort required
to fix the failure. If it's a small change, commit and push the fix directly
to the pull request. Otherwise, skip the test with a reference to a ticket to
fix the failing test.

The goal is that the library passes tests with the latest spec version at all
times, either by implementing small changes quickly, or by skipping tests as
necessary.
Comment on lines +112 to +131
Copy link
Member Author

Choose a reason for hiding this comment

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

@jmikola this is a first shot at covering potential failures in a dependabot PR. Let me know if I missed something or if we should change the guidance. Either way, this would be a learning experience from us and might require some care when implementing spec changes (e.g. don't modify existing test cases when adding functionality).

Copy link
Member

Choose a reason for hiding this comment

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

The three bullets above seem sufficient.

I'm not sure what you mean by "don't modify existing test cases when adding functionality". I assume you're not talking about making custom modifications to the JSON files, as I've never done that (although I recall libmongoc has).

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'm referring to when changes are made in the specs repository. For example, new functionality should rely on new tests as much as possible, without changing existing tests. This allows skipping new tests without sacrificing code coverage because we'd have to skip a test that we were previously able to run.


## Code quality

Before submitting a pull request, please ensure that your code adheres to the
Expand Down
2 changes: 2 additions & 0 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@
}
},
"scripts": {
"pre-install-cmd": "git submodule update --init",
"pre-update-cmd": "git submodule update --init",
"bench": "cd benchmark && composer update && vendor/bin/phpbench run --report=aggregate",
"checks": [
"@check:cs",
Expand Down
6 changes: 4 additions & 2 deletions tests/SpecTests/ClientSideEncryption/Prose22_RangeExplicitEncryptionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ class Prose22_RangeExplicitEncryptionTest extends FunctionalTestCase
private ?Client $encryptedClient = null;
private $key1Id;

private static string $specDir = __DIR__ . '/../../specifications/source/client-side-encryption';
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to use a const or readonly here? Doesn't make much difference -- just curious. The important thing is that it's concise to reference later.

Copy link
Member Author

Choose a reason for hiding this comment

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

No real reason other than using private static in other places. Note that readonly wouldn't work, as readonly properties cannot have a default value but have to be initialised in the constructor.

I suppose we could change this property, as well as a number of other properties in UnifiedSpecTest private constants.


public function setUp(): void
{
parent::setUp();
Expand All @@ -50,7 +52,7 @@ public function setUp(): void

$client = static::createTestClient();

$key1Document = $this->decodeJson(file_get_contents(__DIR__ . '/../client-side-encryption/etc/data/keys/key1-document.json'));
$key1Document = $this->decodeJson(file_get_contents(self::$specDir . '/etc/data/keys/key1-document.json'));
$this->key1Id = $key1Document->_id;

// Drop the key vault collection and insert key1Document with a majority write concern
Expand Down Expand Up @@ -85,7 +87,7 @@ public function setUpWithTypeAndRangeOpts(string $type, array $rangeOpts): void
* for 64-bit integers. This means that DropEncryptedCollection and
* CreateEncryptedCollection will be unable to inspect the option for
* metadata collection names, but that's not necessary for the test. */
$encryptedFields = Document::fromJSON(file_get_contents(__DIR__ . '/../client-side-encryption/etc/data/range-encryptedFields-' . $type . '.json'));
$encryptedFields = Document::fromJSON(file_get_contents(self::$specDir . '/etc/data/range-encryptedFields-' . $type . '.json'));

$database = $this->encryptedClient->selectDatabase($this->getDatabaseName());
$database->dropCollection('explicit_encryption', ['encryptedFields' => $encryptedFields]);
Expand Down
38 changes: 20 additions & 18 deletions tests/SpecTests/ClientSideEncryptionSpecTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ class ClientSideEncryptionSpecTest extends FunctionalTestCase
'fle2v2-Compact: Compact works' => 'Failing due to bug in libmongocrypt (LIBMONGOCRYPT-699)',
];

private static string $specDir = __DIR__ . '/../specifications/source/client-side-encryption';

public function setUp(): void
{
parent::setUp();
Expand Down Expand Up @@ -222,7 +224,7 @@ public static function provideTests()
{
$testArgs = [];

foreach (glob(__DIR__ . '/client-side-encryption/tests/*.json') as $filename) {
foreach (glob(self::$specDir . '/tests/legacy/*.json') as $filename) {
$group = basename($filename, '.json');

/* Some tests need to differentiate int32 and int64 BSON types.
Expand Down Expand Up @@ -421,7 +423,7 @@ public function testExternalKeyVault($withExternalKeyVault): void
$client->selectCollection('db', 'coll')->drop();

self::insertKeyVaultData($client, [
$this->decodeJson(file_get_contents(__DIR__ . '/client-side-encryption/external/external-key.json')),
$this->decodeJson(file_get_contents(self::$specDir . '/external/external-key.json')),
]);

$encryptionOpts = [
Expand All @@ -437,7 +439,7 @@ public function testExternalKeyVault($withExternalKeyVault): void

$autoEncryptionOpts = $encryptionOpts + [
'schemaMap' => [
'db.coll' => $this->decodeJson(file_get_contents(__DIR__ . '/client-side-encryption/external/external-schema.json')),
'db.coll' => $this->decodeJson(file_get_contents(self::$specDir . '/external/external-schema.json')),
],
];

Expand Down Expand Up @@ -567,10 +569,10 @@ public function testBSONSizeLimitsAndBatchSplitting(Closure $test): void
$client = static::createTestClient();

$client->selectCollection('db', 'coll')->drop();
$client->selectDatabase('db')->createCollection('coll', ['validator' => ['$jsonSchema' => $this->decodeJson(file_get_contents(__DIR__ . '/client-side-encryption/limits/limits-schema.json'))]]);
$client->selectDatabase('db')->createCollection('coll', ['validator' => ['$jsonSchema' => $this->decodeJson(file_get_contents(self::$specDir . '/limits/limits-schema.json'))]]);

self::insertKeyVaultData($client, [
$this->decodeJson(file_get_contents(__DIR__ . '/client-side-encryption/limits/limits-key.json')),
$this->decodeJson(file_get_contents(self::$specDir . '/limits/limits-key.json')),
]);

$autoEncryptionOpts = [
Expand All @@ -585,7 +587,7 @@ public function testBSONSizeLimitsAndBatchSplitting(Closure $test): void

$collection = $clientEncrypted->selectCollection('db', 'coll');

$document = json_decode(file_get_contents(__DIR__ . '/client-side-encryption/limits/limits-doc.json'), true, 512, JSON_THROW_ON_ERROR);
$document = json_decode(file_get_contents(self::$specDir . '/limits/limits-doc.json'), true, 512, JSON_THROW_ON_ERROR);

$test($this, $collection, $document);
}
Expand Down Expand Up @@ -634,18 +636,18 @@ public function testCorpus($schemaMap = true): void
$client = static::createTestClient();
$client->selectDatabase('db')->dropCollection('coll');

$schema = $this->decodeJson(file_get_contents(__DIR__ . '/client-side-encryption/corpus/corpus-schema.json'));
$schema = $this->decodeJson(file_get_contents(self::$specDir . '/corpus/corpus-schema.json'));

if (! $schemaMap) {
$client->selectDatabase('db')->createCollection('coll', ['validator' => ['$jsonSchema' => $schema]]);
}

self::insertKeyVaultData($client, [
$this->decodeJson(file_get_contents(__DIR__ . '/client-side-encryption/corpus/corpus-key-local.json')),
$this->decodeJson(file_get_contents(__DIR__ . '/client-side-encryption/corpus/corpus-key-aws.json')),
$this->decodeJson(file_get_contents(__DIR__ . '/client-side-encryption/corpus/corpus-key-azure.json')),
$this->decodeJson(file_get_contents(__DIR__ . '/client-side-encryption/corpus/corpus-key-gcp.json')),
$this->decodeJson(file_get_contents(__DIR__ . '/client-side-encryption/corpus/corpus-key-kmip.json')),
$this->decodeJson(file_get_contents(self::$specDir . '/corpus/corpus-key-local.json')),
$this->decodeJson(file_get_contents(self::$specDir . '/corpus/corpus-key-aws.json')),
$this->decodeJson(file_get_contents(self::$specDir . '/corpus/corpus-key-azure.json')),
$this->decodeJson(file_get_contents(self::$specDir . '/corpus/corpus-key-gcp.json')),
$this->decodeJson(file_get_contents(self::$specDir . '/corpus/corpus-key-kmip.json')),
]);

$encryptionOpts = [
Expand All @@ -670,7 +672,7 @@ public function testCorpus($schemaMap = true): void
];
}

$corpus = (array) $this->decodeJson(file_get_contents(__DIR__ . '/client-side-encryption/corpus/corpus.json'));
$corpus = (array) $this->decodeJson(file_get_contents(self::$specDir . '/corpus/corpus.json'));
$corpusCopied = [];

$clientEncrypted = static::createTestClient(null, [], ['autoEncryption' => $autoEncryptionOpts]);
Expand Down Expand Up @@ -701,7 +703,7 @@ public function testCorpus($schemaMap = true): void

$this->assertDocumentsMatch($corpus, $corpusDecrypted);

$corpusEncryptedExpected = (array) $this->decodeJson(file_get_contents(__DIR__ . '/client-side-encryption/corpus/corpus-encrypted.json'));
$corpusEncryptedExpected = (array) $this->decodeJson(file_get_contents(self::$specDir . '/corpus/corpus-encrypted.json'));
$corpusEncryptedActual = $client->selectCollection('db', 'coll')->findOne(['_id' => 'client_side_encryption_corpus'], ['typeMap' => ['root' => 'array', 'document' => stdClass::class, 'array' => 'array']]);

foreach ($corpusEncryptedExpected as $fieldName => $expectedData) {
Expand Down Expand Up @@ -913,7 +915,7 @@ public function testBypassSpawningMongocryptdViaLoadingSharedLibrary(): void
'local' => ['key' => new Binary(base64_decode(self::LOCAL_MASTERKEY))],
],
'schemaMap' => [
'db.coll' => $this->decodeJson(file_get_contents(__DIR__ . '/client-side-encryption/external/external-schema.json')),
'db.coll' => $this->decodeJson(file_get_contents(self::$specDir . '/external/external-schema.json')),
],
'extraOptions' => [
'mongocryptdBypassSpawn' => true,
Expand Down Expand Up @@ -955,7 +957,7 @@ public function testBypassSpawningMongocryptdViaBypassSpawn(): void
'local' => ['key' => new Binary(base64_decode(self::LOCAL_MASTERKEY))],
],
'schemaMap' => [
'db.coll' => $this->decodeJson(file_get_contents(__DIR__ . '/client-side-encryption/external/external-schema.json')),
'db.coll' => $this->decodeJson(file_get_contents(self::$specDir . '/external/external-schema.json')),
],
'extraOptions' => [
'mongocryptdBypassSpawn' => true,
Expand Down Expand Up @@ -1335,8 +1337,8 @@ public function testExplicitEncryption(Closure $test): void
$this->skipIfServerVersion('<', '7.0.0', 'Explicit encryption tests require MongoDB 7.0 or later');

// Test setup
$encryptedFields = $this->decodeJson(file_get_contents(__DIR__ . '/client-side-encryption/etc/data/encryptedFields.json'));
$key1Document = $this->decodeJson(file_get_contents(__DIR__ . '/client-side-encryption/etc/data/keys/key1-document.json'));
$encryptedFields = $this->decodeJson(file_get_contents(self::$specDir . '/etc/data/encryptedFields.json'));
$key1Document = $this->decodeJson(file_get_contents(self::$specDir . '/etc/data/keys/key1-document.json'));
$key1Id = $key1Document->_id;

$client = static::createTestClient();
Expand Down
Loading