From 25b730bed2fafe40fd202054c90b9d7984170a90 Mon Sep 17 00:00:00 2001 From: Patrick Kusebauch Date: Mon, 11 Mar 2024 09:53:50 +0100 Subject: [PATCH] fix: Throw an error when undefined layer is referenced in the layer collector (#1382) Fixes a case where wi silently skip over collectors that we deem "unresolvable". This is a vestigial case from time when we first implemented "Layer" collector. With the current state of the code, it is not necessary to check if a collector is "resolvable" and it can just fail when it throws an exception while we try to actually run it. These exceptions are already properly covered and rethrown to the end-user. With this change the result of running the deptrac.yaml from the issue will cause: ```console [ERROR] Analysis finished with an Exception. Invalid collector definition. Unknown layer "Domain_typo" specified in collector. ``` --- phpunit.xml.dist | 2 +- src/Core/Layer/Collector/BoolCollector.php | 34 +--- .../ConditionalCollectorInterface.php | 17 -- src/Core/Layer/Collector/LayerCollector.php | 9 +- src/Core/Layer/LayerResolver.php | 16 +- tests/Contract/Config/DeptracConfigTest.php | 2 +- .../Layer/Collector/BoolCollectorTest.php | 167 +----------------- .../Layer/Collector/LayerCollectorTest.php | 26 --- tests/Core/Layer/LayerResolverTest.php | 35 +--- 9 files changed, 11 insertions(+), 297 deletions(-) delete mode 100644 src/Core/Layer/Collector/ConditionalCollectorInterface.php diff --git a/phpunit.xml.dist b/phpunit.xml.dist index bb0c56643..3f0718d11 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -1,5 +1,5 @@ - + ./tests/ diff --git a/src/Core/Layer/Collector/BoolCollector.php b/src/Core/Layer/Collector/BoolCollector.php index cc72838f3..332613ece 100644 --- a/src/Core/Layer/Collector/BoolCollector.php +++ b/src/Core/Layer/Collector/BoolCollector.php @@ -5,9 +5,10 @@ namespace Qossmic\Deptrac\Core\Layer\Collector; use Qossmic\Deptrac\Contract\Ast\TokenReferenceInterface; +use Qossmic\Deptrac\Contract\Layer\CollectorInterface; use Qossmic\Deptrac\Contract\Layer\InvalidCollectorDefinitionException; -final class BoolCollector implements ConditionalCollectorInterface +final class BoolCollector implements CollectorInterface { public function __construct(private readonly CollectorResolverInterface $collectorResolver) {} @@ -38,37 +39,6 @@ public function satisfy(array $config, TokenReferenceInterface $reference): bool return true; } - public function resolvable(array $config): bool - { - $configuration = $this->normalizeConfiguration($config); - - /** @var array{type: string, args: array} $v */ - foreach ((array) $configuration['must'] as $v) { - $collectable = $this->collectorResolver->resolve($v); - $collector = $collectable->collector; - - if ($collector instanceof ConditionalCollectorInterface - && !$collector->resolvable($collectable->attributes) - ) { - return false; - } - } - - /** @var array{type: string, args: array} $v */ - foreach ((array) $configuration['must_not'] as $v) { - $collectable = $this->collectorResolver->resolve($v); - $collector = $collectable->collector; - - if ($collector instanceof ConditionalCollectorInterface - && !$collector->resolvable($collectable->attributes) - ) { - return false; - } - } - - return true; - } - /** * @param array> $configuration * diff --git a/src/Core/Layer/Collector/ConditionalCollectorInterface.php b/src/Core/Layer/Collector/ConditionalCollectorInterface.php deleted file mode 100644 index bf6c24285..000000000 --- a/src/Core/Layer/Collector/ConditionalCollectorInterface.php +++ /dev/null @@ -1,17 +0,0 @@ -> $config - * - * @throws \Qossmic\Deptrac\Contract\Layer\InvalidCollectorDefinitionException - */ - public function resolvable(array $config): bool; -} diff --git a/src/Core/Layer/Collector/LayerCollector.php b/src/Core/Layer/Collector/LayerCollector.php index 84c30fb44..c13b7b327 100644 --- a/src/Core/Layer/Collector/LayerCollector.php +++ b/src/Core/Layer/Collector/LayerCollector.php @@ -5,6 +5,7 @@ namespace Qossmic\Deptrac\Core\Layer\Collector; use Qossmic\Deptrac\Contract\Ast\TokenReferenceInterface; +use Qossmic\Deptrac\Contract\Layer\CollectorInterface; use Qossmic\Deptrac\Contract\Layer\InvalidCollectorDefinitionException; use Qossmic\Deptrac\Contract\Layer\InvalidLayerDefinitionException; use Qossmic\Deptrac\Core\Layer\LayerResolverInterface; @@ -13,7 +14,7 @@ use function is_string; use function sprintf; -final class LayerCollector implements ConditionalCollectorInterface +final class LayerCollector implements CollectorInterface { /** * @var array> @@ -47,10 +48,4 @@ public function satisfy(array $config, TokenReferenceInterface $reference): bool return $this->resolved[$token][$layer] = $this->resolver->isReferenceInLayer($config['value'], $reference); } - - public function resolvable(array $config): bool - { - /** @var array{layer?: string, value: string} $config */ - return $this->resolver->has($config['value']); - } } diff --git a/src/Core/Layer/LayerResolver.php b/src/Core/Layer/LayerResolver.php index a01654d6b..ee75fce1c 100644 --- a/src/Core/Layer/LayerResolver.php +++ b/src/Core/Layer/LayerResolver.php @@ -8,7 +8,6 @@ use Qossmic\Deptrac\Contract\Layer\InvalidLayerDefinitionException; use Qossmic\Deptrac\Core\Layer\Collector\Collectable; use Qossmic\Deptrac\Core\Layer\Collector\CollectorResolverInterface; -use Qossmic\Deptrac\Core\Layer\Collector\ConditionalCollectorInterface; use function array_key_exists; @@ -45,13 +44,7 @@ public function getLayersForReference(TokenReferenceInterface $reference): array foreach ($this->layers as $layer => $collectables) { foreach ($collectables as $collectable) { - $collector = $collectable->collector; $attributes = $collectable->attributes; - if ($collector instanceof ConditionalCollectorInterface - && !$collector->resolvable($attributes) - ) { - continue; - } if ($collectable->collector->satisfy($attributes, $reference)) { if (array_key_exists($layer, $this->resolved[$tokenName]) && $this->resolved[$tokenName][$layer]) { @@ -83,14 +76,7 @@ public function isReferenceInLayer(string $layer, TokenReferenceInterface $refer $collectables = $this->layers[$layer]; foreach ($collectables as $collectable) { - $collector = $collectable->collector; - if ($collector instanceof ConditionalCollectorInterface - && !$collector->resolvable($collectable->attributes) - ) { - continue; - } - - if ($collector->satisfy($collectable->attributes, $reference)) { + if ($collectable->collector->satisfy($collectable->attributes, $reference)) { return true; } } diff --git a/tests/Contract/Config/DeptracConfigTest.php b/tests/Contract/Config/DeptracConfigTest.php index 876d3489b..d76a70698 100644 --- a/tests/Contract/Config/DeptracConfigTest.php +++ b/tests/Contract/Config/DeptracConfigTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace Tests\Qossmic\Deptrac\Contract\Analyser; +namespace Tests\Qossmic\Deptrac\Contract\Config; use PHPUnit\Framework\TestCase; use Qossmic\Deptrac\Contract\Config\AnalyserConfig; diff --git a/tests/Core/Layer/Collector/BoolCollectorTest.php b/tests/Core/Layer/Collector/BoolCollectorTest.php index 52ee75596..f3ca7582c 100644 --- a/tests/Core/Layer/Collector/BoolCollectorTest.php +++ b/tests/Core/Layer/Collector/BoolCollectorTest.php @@ -5,13 +5,13 @@ namespace Tests\Qossmic\Deptrac\Core\Layer\Collector; use PHPUnit\Framework\TestCase; +use Qossmic\Deptrac\Contract\Layer\CollectorInterface; use Qossmic\Deptrac\Contract\Layer\InvalidCollectorDefinitionException; use Qossmic\Deptrac\Core\Ast\AstMap\ClassLike\ClassLikeReference; use Qossmic\Deptrac\Core\Ast\AstMap\ClassLike\ClassLikeToken; use Qossmic\Deptrac\Core\Layer\Collector\BoolCollector; use Qossmic\Deptrac\Core\Layer\Collector\Collectable; use Qossmic\Deptrac\Core\Layer\Collector\CollectorResolverInterface; -use Qossmic\Deptrac\Core\Layer\Collector\ConditionalCollectorInterface; final class BoolCollectorTest extends TestCase { @@ -21,11 +21,7 @@ protected function setUp(): void { parent::setUp(); - $collector = $this->createMock(ConditionalCollectorInterface::class); - $collector - ->method('resolvable') - ->with($this->callback(static fn (array $config): bool => 'custom' === $config['type'])) - ->willReturnCallback(static fn (array $config): bool => (bool) $config['resolvable'] ?? false); + $collector = $this->createMock(CollectorInterface::class); $collector ->method('satisfy') ->with($this->callback(static fn (array $config): bool => 'custom' === $config['type'])) @@ -40,165 +36,6 @@ protected function setUp(): void $this->collector = new BoolCollector($resolver); } - public static function provideResolvableConfiguration(): iterable - { - yield 'must with resolvable collector' => [ - [ - 'must' => [ - [ - 'type' => 'custom', - 'resolvable' => true, - ], - ], - ], - true, - ]; - - yield 'must with unresolvable collector' => [ - [ - 'must' => [ - [ - 'type' => 'custom', - 'resolvable' => false, - ], - ], - ], - false, - ]; - - yield 'must_not with resolvable collector' => [ - [ - 'must_not' => [ - [ - 'type' => 'custom', - 'resolvable' => true, - ], - ], - ], - true, - ]; - - yield 'must_not with unresolvable collector' => [ - [ - 'must_not' => [ - [ - 'type' => 'custom', - 'resolvable' => false, - ], - ], - ], - false, - ]; - - yield 'must with multiple collectors, unresolvable' => [ - [ - 'must' => [ - [ - 'type' => 'custom', - 'resolvable' => true, - ], - [ - 'type' => 'custom', - 'resolvable' => false, - ], - [ - 'type' => 'custom', - 'resolvable' => true, - ], - ], - ], - false, - ]; - - yield 'must_not with multiple collectors, unresolvable' => [ - [ - 'must_not' => [ - [ - 'type' => 'custom', - 'resolvable' => true, - ], - [ - 'type' => 'custom', - 'resolvable' => true, - ], - [ - 'type' => 'custom', - 'resolvable' => false, - ], - ], - ], - false, - ]; - - yield 'mixed with must_not unresolvable' => [ - [ - 'must' => [ - [ - 'type' => 'custom', - 'resolvable' => true, - ], - ], - 'must_not' => [ - [ - 'type' => 'custom', - 'resolvable' => false, - ], - ], - ], - false, - ]; - - yield 'mixed with must unresolvable' => [ - [ - 'must' => [ - [ - 'type' => 'custom', - 'resolvable' => false, - ], - ], - 'must_not' => [ - [ - 'type' => 'custom', - 'resolvable' => true, - ], - ], - ], - false, - ]; - - yield 'mixed with all resolvable' => [ - [ - 'must' => [ - [ - 'type' => 'custom', - 'resolvable' => true, - ], - ], - 'must_not' => [ - [ - 'type' => 'custom', - 'resolvable' => true, - ], - [ - 'type' => 'custom', - 'resolvable' => true, - ], - ], - ], - true, - ]; - } - - /** - * @dataProvider provideResolvableConfiguration - */ - public function testResolvable(array $config, bool $expectedOutcome): void - { - $actualOutcome = $this->collector->resolvable($config); - - self::assertSame($expectedOutcome, $actualOutcome); - } - public static function providesatisfiableConfiguration(): iterable { yield 'must with satisfiable collector' => [ diff --git a/tests/Core/Layer/Collector/LayerCollectorTest.php b/tests/Core/Layer/Collector/LayerCollectorTest.php index 78ea0db13..b17eeca54 100644 --- a/tests/Core/Layer/Collector/LayerCollectorTest.php +++ b/tests/Core/Layer/Collector/LayerCollectorTest.php @@ -37,32 +37,6 @@ public function testConfig(): void ); } - public function testResolvable(): void - { - $this->resolver - ->expects($this->once()) - ->method('has') - ->with('test') - ->willReturn(true); - - $actual = $this->collector->resolvable(['value' => 'test']); - - self::assertSame(true, $actual); - } - - public function testUnrsolvable(): void - { - $this->resolver - ->expects($this->once()) - ->method('has') - ->with('test') - ->willReturn(false); - - $actual = $this->collector->resolvable(['value' => 'test']); - - self::assertSame(false, $actual); - } - public function testSatisfyWithUnknownLayer(): void { $this->resolver diff --git a/tests/Core/Layer/LayerResolverTest.php b/tests/Core/Layer/LayerResolverTest.php index 2fc0e279d..9e263c9bf 100644 --- a/tests/Core/Layer/LayerResolverTest.php +++ b/tests/Core/Layer/LayerResolverTest.php @@ -5,12 +5,12 @@ namespace Tests\Qossmic\Deptrac\Core\Layer; use PHPUnit\Framework\TestCase; +use Qossmic\Deptrac\Contract\Layer\CollectorInterface; use Qossmic\Deptrac\Contract\Layer\InvalidLayerDefinitionException; use Qossmic\Deptrac\Core\Ast\AstMap\ClassLike\ClassLikeReference; use Qossmic\Deptrac\Core\Ast\AstMap\ClassLike\ClassLikeToken; use Qossmic\Deptrac\Core\Layer\Collector\Collectable; use Qossmic\Deptrac\Core\Layer\Collector\CollectorResolverInterface; -use Qossmic\Deptrac\Core\Layer\Collector\ConditionalCollectorInterface; use Qossmic\Deptrac\Core\Layer\LayerResolver; final class LayerResolverTest extends TestCase @@ -115,7 +115,6 @@ public function testIsReferenceInLayer(): void 'collectors' => [ [ 'type' => 'custom', - 'resolvable' => true, 'satisfy' => true, ], ], @@ -145,7 +144,6 @@ public function testGetLayersForReference(): void 'collectors' => [ [ 'type' => 'custom', - 'resolvable' => true, 'satisfy' => true, ], ], @@ -170,7 +168,6 @@ public function testGetLayersForReferenceWhenCollectorDoesNotSatisfy(): void 'collectors' => [ [ 'type' => 'custom', - 'resolvable' => true, 'satisfy' => false, ], ], @@ -184,37 +181,9 @@ public function testGetLayersForReferenceWhenCollectorDoesNotSatisfy(): void ); } - public function testGetLayersForReferenceWhenCollectorIsNotResolvable(): void - { - $reference = new ClassLikeReference(ClassLikeToken::fromFQCN('foo')); - $resolver = new LayerResolver( - $this->buildCollectorResolverWithFakeCollector(), - [ - [ - 'name' => 'test', - 'collectors' => [ - [ - 'type' => 'custom', - 'resolvable' => false, - ], - ], - ], - ] - ); - - self::assertSame( - [], - $resolver->getLayersForReference($reference) - ); - } - private function buildCollectorResolverWithFakeCollector(): CollectorResolverInterface { - $collector = $this->createMock(ConditionalCollectorInterface::class); - $collector - ->method('resolvable') - ->with($this->callback(static fn (array $config): bool => 'custom' === $config['type'])) - ->willReturnCallback(static fn (array $config): bool => (bool) $config['resolvable'] ?? false); + $collector = $this->createMock(CollectorInterface::class); $collector ->method('satisfy') ->with($this->callback(static fn (array $config): bool => 'custom' === $config['type']))