From 63d8d4fdc0d7edfbdf0e0992e29f9769d26aa10c Mon Sep 17 00:00:00 2001 From: Andreas Braun Date: Thu, 19 Sep 2024 14:27:47 +0200 Subject: [PATCH 1/3] Replace FailPointObserver with different logic --- tests/UnifiedSpecTests/Context.php | 18 ++++++ tests/UnifiedSpecTests/FailPointObserver.php | 63 -------------------- tests/UnifiedSpecTests/Operation.php | 13 +++- tests/UnifiedSpecTests/UnifiedTestRunner.php | 33 +++++----- 4 files changed, 47 insertions(+), 80 deletions(-) delete mode 100644 tests/UnifiedSpecTests/FailPointObserver.php diff --git a/tests/UnifiedSpecTests/Context.php b/tests/UnifiedSpecTests/Context.php index 18e2c3a39..f02550299 100644 --- a/tests/UnifiedSpecTests/Context.php +++ b/tests/UnifiedSpecTests/Context.php @@ -5,6 +5,7 @@ use LogicException; use MongoDB\Client; use MongoDB\Driver\ClientEncryption; +use MongoDB\Driver\Server; use MongoDB\Driver\ServerApi; use MongoDB\Model\BSONArray; use MongoDB\Tests\FunctionalTestCase; @@ -60,6 +61,9 @@ final class Context private ?object $advanceClusterTime = null; + /** @var list */ + private array $failPoints = []; + public function __construct(Client $internalClient, string $uri) { $this->entityMap = new EntityMap(); @@ -232,6 +236,20 @@ public function stopEventCollectors(): void } } + public function registerFailPoint(stdClass $failPoint, Server $server): void + { + $this->failPoints[] = [ + 'failPoint' => $failPoint, + 'server' => $server, + ]; + } + + /** @return list */ + public function getFailPoints(): array + { + return $this->failPoints; + } + /** @param string|array $readPreferenceTags */ private function convertReadPreferenceTags($readPreferenceTags): array { diff --git a/tests/UnifiedSpecTests/FailPointObserver.php b/tests/UnifiedSpecTests/FailPointObserver.php deleted file mode 100644 index f843d43fb..000000000 --- a/tests/UnifiedSpecTests/FailPointObserver.php +++ /dev/null @@ -1,63 +0,0 @@ -getCommand(); - - if (! isset($command->configureFailPoint)) { - return; - } - - if (isset($command->mode) && $command->mode === 'off') { - return; - } - - $this->failPointsAndServers[] = [$command->configureFailPoint, $event->getServer()]; - } - - /** @see https://php.net/manual/en/mongodb-driver-monitoring-commandsubscriber.commandsucceeded.php */ - public function commandSucceeded(CommandSucceededEvent $event): void - { - } - - public function disableFailPoints(): void - { - foreach ($this->failPointsAndServers as [$failPoint, $server]) { - $operation = new DatabaseCommand('admin', ['configureFailPoint' => $failPoint, 'mode' => 'off']); - $operation->execute($server); - } - - $this->failPointsAndServers = []; - } - - public function start(): void - { - addSubscriber($this); - } - - public function stop(): void - { - removeSubscriber($this); - } -} diff --git a/tests/UnifiedSpecTests/Operation.php b/tests/UnifiedSpecTests/Operation.php index 3fdf5ea70..58b921aab 100644 --- a/tests/UnifiedSpecTests/Operation.php +++ b/tests/UnifiedSpecTests/Operation.php @@ -909,7 +909,7 @@ private function executeForTestRunner() assertArrayHasKey('failPoint', $args); assertInstanceOf(Client::class, $args['client']); assertInstanceOf(stdClass::class, $args['failPoint']); - $args['client']->selectDatabase('admin')->command($args['failPoint']); + $this->createFailPoint($args['failPoint'], $args['client']->getManager()->selectServer()); break; case 'targetedFailPoint': assertArrayHasKey('session', $args); @@ -917,8 +917,7 @@ private function executeForTestRunner() assertInstanceOf(Session::class, $args['session']); assertInstanceOf(stdClass::class, $args['failPoint']); assertNotNull($args['session']->getServer(), 'Session is pinned'); - $operation = new DatabaseCommand('admin', $args['failPoint']); - $operation->execute($args['session']->getServer()); + $this->createFailPoint($args['failPoint'], $args['session']->getServer()); break; case 'loop': assertArrayHasKey('operations', $args); @@ -937,6 +936,14 @@ private function executeForTestRunner() } } + private function createFailPoint(stdClass $failPoint, Server $server): void + { + $operation = new DatabaseCommand('admin', $failPoint); + $operation->execute($server); + + $this->context->registerFailPoint($failPoint, $server); + } + private function getIndexNames(string $databaseName, string $collectionName): array { return array_map( diff --git a/tests/UnifiedSpecTests/UnifiedTestRunner.php b/tests/UnifiedSpecTests/UnifiedTestRunner.php index 644cbcaaa..c647cd4cd 100644 --- a/tests/UnifiedSpecTests/UnifiedTestRunner.php +++ b/tests/UnifiedSpecTests/UnifiedTestRunner.php @@ -77,8 +77,6 @@ final class UnifiedTestRunner /** @var callable(EntityMap):void */ private $entityMapObserver; - private ?FailPointObserver $failPointObserver = null; - private ServerParameterHelper $serverParameterHelper; public function __construct(string $internalClientUri) @@ -150,9 +148,6 @@ private function doSetUp(): void * after transient error within a transaction" pinning test causes the * subsequent transaction test to block. */ $this->killAllSessions(); - - $this->failPointObserver = new FailPointObserver(); - $this->failPointObserver->start(); } private function doTearDown(bool $hasFailed): void @@ -163,9 +158,6 @@ private function doTearDown(bool $hasFailed): void $this->killAllSessions(); } - $this->failPointObserver->stop(); - $this->failPointObserver->disableFailPoints(); - /* Manually invoking garbage collection since each test is prone to * create cycles (perhaps due to EntityMap), which can leak and prevent * sessions from being released back into the pool. */ @@ -216,13 +208,17 @@ private function doTestCase(stdClass $test, string $schemaVersion, ?array $runOn $context->startEventObservers(); $context->startEventCollectors(); - foreach ($test->operations as $o) { - $operation = new Operation($o, $context); - $operation->assert(); - } + try { + foreach ($test->operations as $o) { + $operation = new Operation($o, $context); + $operation->assert(); + } - $context->stopEventObservers(); - $context->stopEventCollectors(); + $context->stopEventObservers(); + $context->stopEventCollectors(); + } finally { + $this->disableFailPoints($context->getFailPoints()); + } if (isset($test->expectEvents)) { assertIsArray($test->expectEvents); @@ -235,6 +231,15 @@ private function doTestCase(stdClass $test, string $schemaVersion, ?array $runOn } } + /** @param list $failPoints */ + private function disableFailPoints(array $failPoints): void + { + foreach ($failPoints as ['failPoint' => $failPoint, 'server' => $server]) { + $operation = new DatabaseCommand('admin', ['configureFailPoint' => $failPoint, 'mode' => 'off']); + $operation->execute($server); + } + } + /** * Checks server version and topology requirements. * From b1422355e5a58cfa60804658ee637cf8986823a5 Mon Sep 17 00:00:00 2001 From: Jeremy Mikola Date: Thu, 19 Sep 2024 20:38:26 -0400 Subject: [PATCH 2/3] Extract fail point management to a trait used by Context --- tests/UnifiedSpecTests/Context.php | 20 +--------- .../ManagesFailPointsTrait.php | 40 +++++++++++++++++++ tests/UnifiedSpecTests/Operation.php | 15 ++----- tests/UnifiedSpecTests/UnifiedTestRunner.php | 11 +---- 4 files changed, 47 insertions(+), 39 deletions(-) create mode 100644 tests/UnifiedSpecTests/ManagesFailPointsTrait.php diff --git a/tests/UnifiedSpecTests/Context.php b/tests/UnifiedSpecTests/Context.php index f02550299..57e6e7bd2 100644 --- a/tests/UnifiedSpecTests/Context.php +++ b/tests/UnifiedSpecTests/Context.php @@ -5,7 +5,6 @@ use LogicException; use MongoDB\Client; use MongoDB\Driver\ClientEncryption; -use MongoDB\Driver\Server; use MongoDB\Driver\ServerApi; use MongoDB\Model\BSONArray; use MongoDB\Tests\FunctionalTestCase; @@ -39,6 +38,8 @@ */ final class Context { + use ManagesFailPointsTrait; + private ?string $activeClient = null; private EntityMap $entityMap; @@ -61,9 +62,6 @@ final class Context private ?object $advanceClusterTime = null; - /** @var list */ - private array $failPoints = []; - public function __construct(Client $internalClient, string $uri) { $this->entityMap = new EntityMap(); @@ -236,20 +234,6 @@ public function stopEventCollectors(): void } } - public function registerFailPoint(stdClass $failPoint, Server $server): void - { - $this->failPoints[] = [ - 'failPoint' => $failPoint, - 'server' => $server, - ]; - } - - /** @return list */ - public function getFailPoints(): array - { - return $this->failPoints; - } - /** @param string|array $readPreferenceTags */ private function convertReadPreferenceTags($readPreferenceTags): array { diff --git a/tests/UnifiedSpecTests/ManagesFailPointsTrait.php b/tests/UnifiedSpecTests/ManagesFailPointsTrait.php new file mode 100644 index 000000000..d88368fa9 --- /dev/null +++ b/tests/UnifiedSpecTests/ManagesFailPointsTrait.php @@ -0,0 +1,40 @@ + */ + private array $failPointsAndServers = []; + + public function configureFailPoint(stdClass $failPoint, Server $server): void + { + assertObjectHasAttribute('configureFailPoint', $failPoint); + assertIsString($failPoint->configureFailPoint); + assertObjectHasAttribute('mode', $failPoint); + + $operation = new DatabaseCommand('admin', $failPoint); + $operation->execute($server); + + if ($failPoint->mode !== 'off') { + $this->failPointsAndServers[] = [$failPoint->configureFailPoint, $server]; + } + } + + public function disableFailPoints(): void + { + foreach ($this->failPointsAndServers as [$failPoint, $server]) { + $operation = new DatabaseCommand('admin', ['configureFailPoint' => $failPoint, 'mode' => 'off']); + $operation->execute($server); + } + + $this->failPointsAndServers = []; + } +} diff --git a/tests/UnifiedSpecTests/Operation.php b/tests/UnifiedSpecTests/Operation.php index 58b921aab..11da19280 100644 --- a/tests/UnifiedSpecTests/Operation.php +++ b/tests/UnifiedSpecTests/Operation.php @@ -16,7 +16,6 @@ use MongoDB\Model\CollectionInfo; use MongoDB\Model\DatabaseInfo; use MongoDB\Model\IndexInfo; -use MongoDB\Operation\DatabaseCommand; use MongoDB\Operation\FindOneAndReplace; use MongoDB\Operation\FindOneAndUpdate; use PHPUnit\Framework\Assert; @@ -909,7 +908,8 @@ private function executeForTestRunner() assertArrayHasKey('failPoint', $args); assertInstanceOf(Client::class, $args['client']); assertInstanceOf(stdClass::class, $args['failPoint']); - $this->createFailPoint($args['failPoint'], $args['client']->getManager()->selectServer()); + // Configure the fail point via the Context so it can later be disabled + $this->context->configureFailPoint($args['failPoint'], $args['client']->getManager()->selectServer()); break; case 'targetedFailPoint': assertArrayHasKey('session', $args); @@ -917,7 +917,8 @@ private function executeForTestRunner() assertInstanceOf(Session::class, $args['session']); assertInstanceOf(stdClass::class, $args['failPoint']); assertNotNull($args['session']->getServer(), 'Session is pinned'); - $this->createFailPoint($args['failPoint'], $args['session']->getServer()); + // Configure the fail point via the Context so it can later be disabled + $this->context->configureFailPoint($args['failPoint'], $args['session']->getServer()); break; case 'loop': assertArrayHasKey('operations', $args); @@ -936,14 +937,6 @@ private function executeForTestRunner() } } - private function createFailPoint(stdClass $failPoint, Server $server): void - { - $operation = new DatabaseCommand('admin', $failPoint); - $operation->execute($server); - - $this->context->registerFailPoint($failPoint, $server); - } - private function getIndexNames(string $databaseName, string $collectionName): array { return array_map( diff --git a/tests/UnifiedSpecTests/UnifiedTestRunner.php b/tests/UnifiedSpecTests/UnifiedTestRunner.php index c647cd4cd..5c901078f 100644 --- a/tests/UnifiedSpecTests/UnifiedTestRunner.php +++ b/tests/UnifiedSpecTests/UnifiedTestRunner.php @@ -217,7 +217,7 @@ private function doTestCase(stdClass $test, string $schemaVersion, ?array $runOn $context->stopEventObservers(); $context->stopEventCollectors(); } finally { - $this->disableFailPoints($context->getFailPoints()); + $context->disableFailPoints(); } if (isset($test->expectEvents)) { @@ -231,15 +231,6 @@ private function doTestCase(stdClass $test, string $schemaVersion, ?array $runOn } } - /** @param list $failPoints */ - private function disableFailPoints(array $failPoints): void - { - foreach ($failPoints as ['failPoint' => $failPoint, 'server' => $server]) { - $operation = new DatabaseCommand('admin', ['configureFailPoint' => $failPoint, 'mode' => 'off']); - $operation->execute($server); - } - } - /** * Checks server version and topology requirements. * From 89211526a38fe3a14ff93c20a08c7c7f88dbcb51 Mon Sep 17 00:00:00 2001 From: Jeremy Mikola Date: Thu, 19 Sep 2024 20:41:42 -0400 Subject: [PATCH 3/3] Ensure event listeners are still unregistered after an operation fails --- tests/UnifiedSpecTests/UnifiedTestRunner.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/UnifiedSpecTests/UnifiedTestRunner.php b/tests/UnifiedSpecTests/UnifiedTestRunner.php index 5c901078f..f816d8456 100644 --- a/tests/UnifiedSpecTests/UnifiedTestRunner.php +++ b/tests/UnifiedSpecTests/UnifiedTestRunner.php @@ -213,10 +213,9 @@ private function doTestCase(stdClass $test, string $schemaVersion, ?array $runOn $operation = new Operation($o, $context); $operation->assert(); } - + } finally { $context->stopEventObservers(); $context->stopEventCollectors(); - } finally { $context->disableFailPoints(); }