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.

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-1531: Replace FailPointObserver with different logic #1427

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions tests/UnifiedSpecTests/Context.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -60,6 +61,9 @@ final class Context

private ?object $advanceClusterTime = null;

/** @var list<array{failPoint: stdClass, server: Server}> */
private array $failPoints = [];

public function __construct(Client $internalClient, string $uri)
{
$this->entityMap = new EntityMap();
Expand Down Expand Up @@ -232,6 +236,20 @@ public function stopEventCollectors(): void
}
}

public function registerFailPoint(stdClass $failPoint, Server $server): void
{
$this->failPoints[] = [
'failPoint' => $failPoint,
'server' => $server,
];
}

/** @return list<array{failPoint: stdClass, server: Server}> */
public function getFailPoints(): array
{
return $this->failPoints;
}

/** @param string|array $readPreferenceTags */
private function convertReadPreferenceTags($readPreferenceTags): array
{
Expand Down
63 changes: 0 additions & 63 deletions tests/UnifiedSpecTests/FailPointObserver.php

This file was deleted.

13 changes: 10 additions & 3 deletions tests/UnifiedSpecTests/Operation.php
Original file line number Diff line number Diff line change
Expand Up @@ -909,16 +909,15 @@ 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);
assertArrayHasKey('failPoint', $args);
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);
Expand All @@ -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(
Expand Down
33 changes: 19 additions & 14 deletions tests/UnifiedSpecTests/UnifiedTestRunner.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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. */
Expand Down Expand Up @@ -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);
Expand All @@ -235,6 +231,15 @@ private function doTestCase(stdClass $test, string $schemaVersion, ?array $runOn
}
}

/** @param list<array{failPoint: stdClass, server: Server}> $failPoints */
private function disableFailPoints(array $failPoints): void
{
foreach ($failPoints as ['failPoint' => $failPoint, 'server' => $server]) {
$operation = new DatabaseCommand('admin', ['configureFailPoint' => $failPoint, 'mode' => 'off']);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$operation = new DatabaseCommand('admin', ['configureFailPoint' => $failPoint, 'mode' => 'off']);
$operation = new DatabaseCommand('admin', ['configureFailPoint' => $failPoint->configureFailPoint, 'mode' => 'off']);

@alcaeus: This should resolve the runtime error you encountered.

That said, I came up with another PR that consolidates this logic into a ManagesFailPointsTrait, which is closer to what we had with FailPointObserver. These files are already quite complex, so I wanted to minimize additional clutter. I'll defer to you, though.

See: alcaeus#1

$operation->execute($server);
}
}

/**
* Checks server version and topology requirements.
*
Expand Down
Loading