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

alcaeus · 2024-09-20T11:42:49Z

This is a proof of concept that I'd like to try out for a few weeks. The idea is to do away with manual spec test updates and automate the process in a simple manner. To achieve this, we include the specifications as a git submodule and run the tests directly from there. Using dependabot, we get a weekly PR to update the submodule if any changes have been merged, and as long as the build is green we can merge the PR and close any PHP tickets associated to the linked drivers tickets.

The downside to this is that all our specs will be at the same versions. It should be possible to leverage test skips to work around any issues, but only time will tell. Note that undoing this will be relatively straightforward: we can copy the last set of tests that passed to the original test locations, then revert all of the commits from this PR.

GromNaN

Could you add a pre-install/pre-update hook in composer.json to update the submodule? So that we don't have to think about it.

.evergreen/config/functions.yml

alcaeus · 2024-09-24T07:30:25Z

Could you add a pre-install/pre-update hook in composer.json to update the submodule? So that we don't have to think about it.

Added this command. I've used pre-install and pre-update for hooks, which relies on git information to be present. If this is not the case, composer install and composer update will fail.

alcaeus · 2024-09-24T07:31:37Z

CONTRIBUTING.md

+#### 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.


@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).

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).

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.

alcaeus · 2024-10-01T07:20:13Z

Updated to include changes for PHPLIB-1458.

CONTRIBUTING.md

jmikola · 2024-10-01T13:16:27Z

tests/SpecTests/ClientSideEncryption/Prose22_RangeExplicitEncryptionTest.php

@@ -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';


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.

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.

jmikola · 2024-10-01T13:29:31Z

tests/UnifiedSpecTests/Operation.php

@@ -235,6 +235,10 @@ private function executeForChangeStream(ChangeStream $changeStream)

    private function executeForClient(Client $client)
    {
+        if ($this->name === 'clientBulkWrite') {
+            Assert::markTestSkipped('clientBulkWrite operation is not implemented');
+        }


I was going to ask why you implemented the check here, but then realized that Util::assertArgumentsBySchema() asserts that that the operation is supported. That actually makes the switch statement's default case unreachable; although I'm not sure if tooling is smart enough to not complain about that.

I do think we should revisit how we track skipped tests, as splitting logic between the Operation class and test runner will ultimately make things harder to track. I've long wanted to support regex patterns to avoid enumerating individual test files and cases. This kind of feature detection also makes sense, although it would ideally be consolidated in a single place. Opened PHPLIB-1540 to track that idea.

jmikola · 2024-10-01T13:30:36Z

tests/UnifiedSpecTests/UnifiedSpecTest.php

    ];

+    private static array $duplicateTests = ['crud/client bulkWrite partial results: partialResult is set when first operation fails during an unordered bulk write (summary)'];


I assume this is related to a non-unique test name. Consider adding a comment here, as this should ultimately be temporary and something we can revisit after enforcing uniqueness in the specs repo.

Asked about the offending test in https://github.com/mongodb/specifications/pull/1665/files#r1790278528

Thank you! This was fixed in the meantime, but I've kept the duplicateTests variable as a quick way to skip tests in future.

jmikola

Some suggestions but LGTM. Please create a ticket for this (and amend the commit message) before merging.

jmikola

Some more suggestions/questions.

jmikola · 2024-10-07T13:41:31Z

tests/UnifiedSpecTests/Operation.php

@@ -166,6 +184,7 @@ private function execute()
        $object = $this->entityMap[$this->object];
        assertIsObject($object);

+        $this->skipIfOperationIsNotSupported($object::class);


I realize Util::assertArgumentsBySchema() accepts either a class name or Operation::OBJECT_TEST_RUNNER; however, an internal skip method could solicit the object and operation names as strings (i.e. $o->object and $o->name). If so, it could be a static method that's called immediately after the type assertions in the constructor:

assertIsString($o->name); $this->name = $o->name; assertIsString($o->object); $this->isTestRunnerOperation = $o->object === self::OBJECT_TEST_RUNNER; $this->object = $this->isTestRunnerOperation ? null : $o->object; self::skipIfOperationIsNotSupported($o->object, $o->name);

Just a suggestion. Feel free to disregard if you prefer the current approach.

I started implementing this approach, unfortunately the object we have here is not an actual object, but a name of an object in the entity map. Since this object may not even exist at the time the operation instance is created (e.g. if it's only created later using createEntities), I also can't get a class name for the object being used. I'm afraid we'll have to skip when we get to execute.

tests/UnifiedSpecTests/UnifiedSpecTest.php

jmikola · 2024-10-07T13:57:40Z

tests/UnifiedSpecTests/UnifiedSpecTest.php

-                yield $group . '/' . $name => [$test];
+                $testKey = $testGroup . '/' . $name;
+
+                if (isset($duplicateTests[$testKey])) {


Do I understand correctly that the duplicate test name is only a problem because PHPUnit asserts that the data provider yields unique keys?

That is correct. PHPUnit asserts that each key is unique. While this was always true for arrays as data providers, using generators would allow one to yield the same key multiple times. I'm not sure why PHPUnit enforces this, but here we are 🤷

tests/UnifiedSpecTests/UnifiedSpecTest.php

jmikola · 2024-10-07T14:04:07Z

tests/UnifiedSpecTests/UnifiedSpecTest.php

    ];

+    private static array $duplicateTests = ['crud/client bulkWrite partial results: partialResult is set when first operation fails during an unordered bulk write (summary)'];


Asked about the offending test in https://github.com/mongodb/specifications/pull/1665/files#r1790278528

tests/UnifiedSpecTests/UnifiedSpecTest.php

jmikola

LGTM with phpcs warnings addressed.

jmikola · 2024-10-16T00:45:00Z

tests/UnifiedSpecTests/UnifiedSpecTest.php

@@ -14,8 +14,7 @@
 use function basename;
 use function dirname;


Note the phpcs warnings for basename and dirname.

* v1.x: (101 commits) PHPLIB-1541: Include specs repository as a submodule (#1429) PHPLIB-1548 Inherit `typeMap` option in `Collection::listSearchIndexes()` (#1482) Fix array shape for `Collection::listSearchIndex($options)` (#1480) PHPLIB-1545: Deprecate CreateCollection flags option and related constants (#1477) Fix junit logging (#1475) Deprecate typeMap on operations without meaningful result (#1473) PHPLIB-1369 Upgrade to PHPUnit 10 (#1412) Higher phpunit version required (#1463) Fix deprecations in tests (#1458) Deprecate functionality to be removed (#1441) Expect BulkWriteException (#1455) Merge v1.20 into v1.x (#1447) PHPLIB-1525 Removes dependency to Symfony PHPUnit bridge (#1413) Change deprecated assertObjectHasAttribute to assertObjectHasProperty (#1432) Performance: Keep collections and indexes between GridFS tests (#1421) Add final annotations to non-internal Operation classes (#1410) Fix types accepted by $round (#1401) Replace arrayHasKey with assertArrayHasKey in tests (#1403) PHPLIB-1514 Make data providers static (#1404) PHPLIB-1515 Replace assertObjectHasAttribute with assertObjectHasProperty (#1405) ...

alcaeus requested review from jmikola and GromNaN September 20, 2024 11:42

alcaeus requested a review from a team as a code owner September 20, 2024 11:42

GromNaN reviewed Sep 23, 2024

View reviewed changes

.evergreen/config/functions.yml Outdated Show resolved Hide resolved

alcaeus commented Sep 24, 2024

View reviewed changes

alcaeus force-pushed the spec-submodule branch 2 times, most recently from e50458e to bdb64a7 Compare October 1, 2024 07:19

alcaeus force-pushed the spec-submodule branch from 6f1cac1 to f40f9a6 Compare October 1, 2024 09:05

GromNaN approved these changes Oct 1, 2024

View reviewed changes

jmikola reviewed Oct 1, 2024

View reviewed changes

jmikola self-requested a review October 1, 2024 13:32

alcaeus changed the title ~~Include specs repository as a submodule~~ PHPLIB-1541: Include specs repository as a submodule Oct 1, 2024

jmikola approved these changes Oct 1, 2024

View reviewed changes

alcaeus force-pushed the spec-submodule branch from 9604272 to cb5f43f Compare October 2, 2024 08:34

alcaeus added 11 commits October 2, 2024 13:43

Add submodule for specifications

73a17d1

Run spec tests from submodule

b603979

Add dependabot configuration to update spec submodule

d1313e9

Add submodule update to contributing docs

135582a

Initialise spec submodule through composer

99c7d24

Add guidance for fixing spec test failures in dependabot PRs

6244176

PHPLIB-1458: Update aggregate-write-readPreference tests

7eb4d70

Update spec test paths

c5c0631

Allow skipping duplicate tests in data providers

70a38d9

Skip all clientBulkWrite tests

fb7b3ec

PHPLIB-1450: Consolidate and improve skipping of tests

b0269f6

alcaeus force-pushed the spec-submodule branch from cb5f43f to b0269f6 Compare October 2, 2024 11:43

alcaeus requested a review from jmikola October 2, 2024 15:02

jmikola reviewed Oct 7, 2024

View reviewed changes

alcaeus added 6 commits October 14, 2024 15:32

Update to latest specs commit

b1b1260

Use str_starts_with instead of regex logic

5615fc5

Add annotations to type-qualify arrays

1ed6e95

Always require passing test group in provideTests

65357f4

Consistent wrapping

d9fc698

PHPLIB-1492: Skip spec tests for sort in update operations

af04a9c

jmikola approved these changes Oct 16, 2024

View reviewed changes

Remove obsolete imports

afbcd0b

alcaeus enabled auto-merge (squash) October 16, 2024 06:41

alcaeus merged commit 48a78c7 into mongodb:v1.x Oct 16, 2024
29 checks passed

mongodb-php-bot mentioned this pull request Oct 16, 2024

Merge v1.x into v2.x #1485

Merged

alcaeus deleted the spec-submodule branch October 16, 2024 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alcaeus commented Sep 20, 2024

GromNaN left a comment

alcaeus commented Sep 24, 2024

alcaeus Sep 24, 2024

jmikola Oct 1, 2024

alcaeus Oct 1, 2024

alcaeus commented Oct 1, 2024

jmikola Oct 1, 2024

alcaeus Oct 14, 2024

jmikola Oct 1, 2024

jmikola Oct 1, 2024

jmikola Oct 7, 2024

alcaeus Oct 14, 2024

jmikola left a comment

jmikola left a comment

jmikola Oct 7, 2024

alcaeus Oct 14, 2024

jmikola Oct 7, 2024

alcaeus Oct 14, 2024

jmikola Oct 7, 2024

jmikola left a comment

jmikola Oct 16, 2024

		];

		private static array $duplicateTests = ['crud/client bulkWrite partial results: partialResult is set when first operation fails during an unordered bulk write (summary)'];

		@@ -14,8 +14,7 @@
		use function basename;
		use function dirname;

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

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

Conversation

alcaeus commented Sep 20, 2024

GromNaN left a comment

Choose a reason for hiding this comment

alcaeus commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment