From 5dc93aa12bac76ae60129057316fa258ce33e7c8 Mon Sep 17 00:00:00 2001 From: Tom Carrio Date: Mon, 2 Jan 2023 00:39:17 -0500 Subject: [PATCH] test: coverage improvements and chores (#65) ## This PR - test: add shared base hook classes cases - fix!: rename finallyAfter to finally in AbstractHook - refactor: default to set NoOpProvider - chore: cleanup commented code for $_SESSION - test: can get version from Client - feat: hook execution reversal - test: cases for NoOpClient - test: implement decapitalize unit tests ### Related Issues Fixes #54 Fixes #64 Fixes #55 Fixes #63 Fixes #57 Fixes #62 Fixes #59 Fixes #60 Signed-off-by: Tom Carrio --- src/OpenFeatureAPI.php | 22 +- src/OpenFeatureClient.php | 10 +- src/implementation/hooks/AbstractHook.php | 2 +- tests/unit/BaseHooksTest.php | 241 ++++++++++++++++++++++ tests/unit/NoOpClientTest.php | 56 +++++ tests/unit/OpenFeatureClientTest.php | 14 +- tests/unit/StringHelperTest.php | 26 +++ 7 files changed, 343 insertions(+), 28 deletions(-) create mode 100644 tests/unit/BaseHooksTest.php diff --git a/src/OpenFeatureAPI.php b/src/OpenFeatureAPI.php index 73b712f..8971509 100644 --- a/src/OpenFeatureAPI.php +++ b/src/OpenFeatureAPI.php @@ -19,16 +19,13 @@ use function array_merge; use function is_null; -class OpenFeatureAPI implements API, LoggerAwareInterface +final class OpenFeatureAPI implements API, LoggerAwareInterface { use LoggerAwareTrait; private static ?OpenFeatureAPI $instance = null; - //// TODO: Support global using $_SESSION? - // private const GLOBAL_OPEN_FEATURE_KEY = '__OPENFEATURE_INSTANCE_ID__'; - - private ?Provider $provider = null; + private Provider $provider; /** @var Hook[] $hooks */ private array $hooks = []; @@ -43,15 +40,6 @@ class OpenFeatureAPI implements API, LoggerAwareInterface */ public static function getInstance(): API { - //// TODO: Support global using $_SESSION? - // if (isset($_SESSION)) { - // if (is_null($_SESSION[self::GLOBAL_OPEN_FEATURE_KEY])) { - // $_SESSION[self::GLOBAL_OPEN_FEATURE_KEY] = new self(); - // } - - // return $_SESSION[self::GLOBAL_OPEN_FEATURE_KEY]; - // } - if (is_null(self::$instance)) { self::$instance = new self(); } @@ -70,15 +58,11 @@ public static function getInstance(): API */ private function __construct() { - // no-op + $this->provider = new NoOpProvider(); } public function getProvider(): Provider { - if (!$this->provider) { - return new NoOpProvider(); - } - return $this->provider; } diff --git a/src/OpenFeatureClient.php b/src/OpenFeatureClient.php index adbc6b6..cd140a9 100644 --- a/src/OpenFeatureClient.php +++ b/src/OpenFeatureClient.php @@ -37,6 +37,7 @@ use Throwable; use function array_merge; +use function array_reverse; use function sprintf; class OpenFeatureClient implements Client, LoggerAwareInterface @@ -331,13 +332,8 @@ private function evaluateFlag( $options->getHooks(), $provider->getHooks(), ); - // TODO: Should we do a complete reversal of $mergedBeforeHooks instead? - $mergedRemainingHooks = array_merge( - $provider->getHooks(), - $options->getHooks(), - $this->getHooks(), - $api->getHooks(), - ); + + $mergedRemainingHooks = array_reverse(array_merge([], $mergedBeforeHooks)); try { $contextFromBeforeHook = $hookExecutor->beforeHooks($flagValueType, $hookContext, $mergedBeforeHooks, $hookHints); diff --git a/src/implementation/hooks/AbstractHook.php b/src/implementation/hooks/AbstractHook.php index b0cc669..4303d9c 100644 --- a/src/implementation/hooks/AbstractHook.php +++ b/src/implementation/hooks/AbstractHook.php @@ -19,7 +19,7 @@ abstract public function after(HookContext $context, ResolutionDetails $details, abstract public function error(HookContext $context, Throwable $error, HookHints $hints): void; - abstract public function finallyAfter(HookContext $context, HookHints $hints): void; + abstract public function finally(HookContext $context, HookHints $hints): void; abstract public function supportsFlagValueType(string $flagValueType): bool; } diff --git a/tests/unit/BaseHooksTest.php b/tests/unit/BaseHooksTest.php new file mode 100644 index 0000000..83903e9 --- /dev/null +++ b/tests/unit/BaseHooksTest.php @@ -0,0 +1,241 @@ +assertEquals($supportsFlagValueType, $testBooleanHook->supportsFlagValueType($flagValueType)); + } + + /** + * @return Array> + */ + public function dataBooleanHook(): array + { + return [ + [FlagValueType::BOOLEAN, true], + [FlagValueType::FLOAT, false], + [FlagValueType::INTEGER, false], + [FlagValueType::OBJECT, false], + [FlagValueType::STRING, false], + ]; + } + + /** + * @dataProvider dataFloatHook + */ + public function testFloatHook(string $flagValueType, bool $supportsFlagValueType): void + { + $testFloatHook = new class extends FloatHook { + public function before(HookContext $context, HookHints $hints): ?EvaluationContext + { + return null; + } + + public function after(HookContext $context, ResolutionDetails $details, HookHints $hints): void + { + // no-op + } + + public function error(HookContext $context, Throwable $error, HookHints $hints): void + { + // no-op + } + + public function finally(HookContext $context, HookHints $hints): void + { + // no-op + } + }; + + $this->assertEquals($supportsFlagValueType, $testFloatHook->supportsFlagValueType($flagValueType)); + } + + /** + * @return Array> + */ + public function dataFloatHook(): array + { + return [ + [FlagValueType::BOOLEAN, false], + [FlagValueType::FLOAT, true], + [FlagValueType::INTEGER, false], + [FlagValueType::OBJECT, false], + [FlagValueType::STRING, false], + ]; + } + + /** + * @dataProvider dataIntegerHook + */ + public function testIntegerHook(string $flagValueType, bool $supportsFlagValueType): void + { + $testIntegerHook = new class extends IntegerHook { + public function before(HookContext $context, HookHints $hints): ?EvaluationContext + { + return null; + } + + public function after(HookContext $context, ResolutionDetails $details, HookHints $hints): void + { + // no-op + } + + public function error(HookContext $context, Throwable $error, HookHints $hints): void + { + // no-op + } + + public function finally(HookContext $context, HookHints $hints): void + { + // no-op + } + }; + + $this->assertEquals($supportsFlagValueType, $testIntegerHook->supportsFlagValueType($flagValueType)); + } + + /** + * @return Array> + */ + public function dataIntegerHook(): array + { + return [ + [FlagValueType::BOOLEAN, false], + [FlagValueType::FLOAT, false], + [FlagValueType::INTEGER, true], + [FlagValueType::OBJECT, false], + [FlagValueType::STRING, false], + ]; + } + + /** + * @dataProvider dataObjectHook + */ + public function testObjectHook(string $flagValueType, bool $supportsFlagValueType): void + { + $testObjectHook = new class extends ObjectHook { + public function before(HookContext $context, HookHints $hints): ?EvaluationContext + { + return null; + } + + public function after(HookContext $context, ResolutionDetails $details, HookHints $hints): void + { + // no-op + } + + public function error(HookContext $context, Throwable $error, HookHints $hints): void + { + // no-op + } + + public function finally(HookContext $context, HookHints $hints): void + { + // no-op + } + }; + + $this->assertEquals($supportsFlagValueType, $testObjectHook->supportsFlagValueType($flagValueType)); + } + + /** + * @return Array> + */ + public function dataObjectHook(): array + { + return [ + [FlagValueType::BOOLEAN, false], + [FlagValueType::FLOAT, false], + [FlagValueType::INTEGER, false], + [FlagValueType::OBJECT, true], + [FlagValueType::STRING, false], + ]; + } + + /** + * @dataProvider dataStringHook + */ + public function testStringHook(string $flagValueType, bool $supportsFlagValueType): void + { + $testStringHook = new class extends StringHook { + public function before(HookContext $context, HookHints $hints): ?EvaluationContext + { + return null; + } + + public function after(HookContext $context, ResolutionDetails $details, HookHints $hints): void + { + // no-op + } + + public function error(HookContext $context, Throwable $error, HookHints $hints): void + { + // no-op + } + + public function finally(HookContext $context, HookHints $hints): void + { + // no-op + } + }; + + $this->assertEquals($supportsFlagValueType, $testStringHook->supportsFlagValueType($flagValueType)); + } + + /** + * @return Array> + */ + public function dataStringHook(): array + { + return [ + [FlagValueType::BOOLEAN, false], + [FlagValueType::FLOAT, false], + [FlagValueType::INTEGER, false], + [FlagValueType::OBJECT, false], + [FlagValueType::STRING, true], + ]; + } +} diff --git a/tests/unit/NoOpClientTest.php b/tests/unit/NoOpClientTest.php index c2d304e..093bb77 100644 --- a/tests/unit/NoOpClientTest.php +++ b/tests/unit/NoOpClientTest.php @@ -9,6 +9,11 @@ use OpenFeature\implementation\flags\EvaluationContext; use OpenFeature\implementation\flags\EvaluationDetailsBuilder; use OpenFeature\implementation\flags\NoOpClient; +use OpenFeature\interfaces\hooks\Hook; +use OpenFeature\interfaces\hooks\HookContext; +use OpenFeature\interfaces\hooks\HookHints; +use OpenFeature\interfaces\provider\ResolutionDetails; +use Throwable; class NoOpClientTest extends TestCase { @@ -156,4 +161,55 @@ public function testGetHooks(): void $this->assertEquals($expectedValue, $actualValue); } + + public function testSetEvaluationContext(): void + { + $evaluationContext = new EvaluationContext('override-targeting-key'); + + $this->client->setEvaluationContext($evaluationContext); + + $actualValue = $this->client->getEvaluationContext(); + + $expectedValue = new EvaluationContext('no-op-targeting-key'); + + $this->assertEquals($expectedValue, $actualValue); + } + + public function testSetHooks(): void + { + $fakeHook = new class implements Hook { + public function before(HookContext $context, HookHints $hints): ?EvaluationContext + { + return null; + } + + public function after(HookContext $context, ResolutionDetails $details, HookHints $hints): void + { + // no-op + } + + public function error(HookContext $context, Throwable $error, HookHints $hints): void + { + // no-op + } + + public function finally(HookContext $context, HookHints $hints): void + { + // no-op + } + + public function supportsFlagValueType(string $flagValueType): bool + { + return true; + } + }; + + $this->client->setHooks([$fakeHook]); + + $actualValue = $this->client->getHooks(); + + $expectedValue = []; + + $this->assertEquals($expectedValue, $actualValue); + } } diff --git a/tests/unit/OpenFeatureClientTest.php b/tests/unit/OpenFeatureClientTest.php index e6231f6..1b5cd46 100644 --- a/tests/unit/OpenFeatureClientTest.php +++ b/tests/unit/OpenFeatureClientTest.php @@ -1474,7 +1474,8 @@ public function testErrorsInAfterHooksStopsFurtherExecutionOfRemainingSaidHooks( $api = APITestHelper::new(); $api->setProvider($mockProvider); - $api->addHooks($erroringHook, $subsequentHook); + // error hooks run in reverse order + $api->addHooks($subsequentHook, $erroringHook); $client = new OpenFeatureClient($api, 'test-name', 'test-version'); @@ -1510,6 +1511,17 @@ public function testErrorInBeforeHooksMustReturnDefaultValue(): void $this->assertEquals($actualValue, false); } + public function testCanGetVersion(): void + { + $expectedVersion = 'a.b.c'; + + $client = new OpenFeatureClient(APITestHelper::new(), 'name', $expectedVersion); + + $actualVersion = $client->getVersion(); + + $this->assertEquals($expectedVersion, $actualVersion); + } + /** * @return Provider|MockInterface */ diff --git a/tests/unit/StringHelperTest.php b/tests/unit/StringHelperTest.php index 5883a09..52e4954 100644 --- a/tests/unit/StringHelperTest.php +++ b/tests/unit/StringHelperTest.php @@ -34,4 +34,30 @@ public function capitalizeData(): array ['Abc', 'Abc'], ]; } + + /** + * @dataProvider decapitalizeData + */ + public function testDecapitalize(string $input, string $expectedValue): void + { + $actualValue = StringHelper::decapitalize($input); + + $this->assertEquals($expectedValue, $actualValue); + } + + /** + * @return Array> + */ + public function decapitalizeData(): array + { + return [ + ['', ''], + ['a', 'a'], + ['A', 'a'], + ['ab', 'ab'], + ['Ab', 'ab'], + ['abc', 'abc'], + ['Abc', 'abc'], + ]; + } }