Skip to content

Commit

Permalink
fix: Throw an error when undefined layer is referenced in the layer c…
Browse files Browse the repository at this point in the history 
…ollector (#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.
```
  • Loading branch information
@patrickkusebauch
patrickkusebauch authored Mar 11, 2024
1 parent f6964f5 commit 25b730b
Show file tree
Hide file tree
Showing 9 changed files with 11 additions and 297 deletions.
2 changes: 1 addition & 1 deletion phpunit.xml.dist
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/10.2/phpunit.xsd" bootstrap="vendor/autoload.php" colors="true" executionOrder="random" resolveDependencies="true" cacheResultFile=".cache/phpunit/phpunit.result.cache">
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/10.2/phpunit.xsd" bootstrap="vendor/autoload.php" colors="true" executionOrder="random" resolveDependencies="true" cacheResultFile=".cache/phpunit/phpunit.result.cache" displayDetailsOnTestsThatTriggerWarnings="true">
<testsuites>
<testsuite name="Tests">
<directory suffix="Test.php">./tests/</directory>
Expand Down
34 changes: 2 additions & 32 deletions src/Core/Layer/Collector/BoolCollector.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}

Expand Down Expand Up @@ -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<string, string>} $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<string, string>} $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<string, bool|string|array<string, string>> $configuration
*
Expand Down
17 changes: 0 additions & 17 deletions src/Core/Layer/Collector/ConditionalCollectorInterface.php
View file

This file was deleted.

9 changes: 2 additions & 7 deletions src/Core/Layer/Collector/LayerCollector.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -13,7 +14,7 @@
use function is_string;
use function sprintf;

final class LayerCollector implements ConditionalCollectorInterface
final class LayerCollector implements CollectorInterface
{
/**
* @var array<string, array<string, bool|null>>
Expand Down Expand Up @@ -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']);
}
}
16 changes: 1 addition & 15 deletions src/Core/Layer/LayerResolver.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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]) {
Expand Down Expand Up @@ -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;
}
}
Expand Down
2 changes: 1 addition & 1 deletion tests/Contract/Config/DeptracConfigTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
167 changes: 2 additions & 165 deletions tests/Core/Layer/Collector/BoolCollectorTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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']))
Expand All @@ -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' => [
Expand Down
26 changes: 0 additions & 26 deletions tests/Core/Layer/Collector/LayerCollectorTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit 25b730b

Please sign in to comment.