pest icon indicating copy to clipboard operation
pest copied to clipboard

[Bug]: Arch Test fails silently on incorrect interface implementations

Open Jubeki opened this issue 1 year ago • 5 comments

What Happened

I tried to run the Pest Arch Laravel Preset on one of my projects and it simply failed silently without outputting anything.

With a lot of try and error I detected, that on of the classes used an interface, which implemented an incorrect method signature, which resulted in the failure. (I already solved the problem with the correcting the implementation, but I would have expected some kind of output to find the issue)

This one class was not covered with a test yet, because the project is WIP.

How to Reproduce

  1. Setup Laravel Arch Preset Test Case.
  2. Create an Interface with a method
  3. Create a Class which implements the Interface, but defines the signature incorrectly.

Here an example interface:

<?php

namespace App;

interface ExampleContract
{
    public function getClaims(array $scopes): array;
}

And the incorrect class implementation

<?php

namespace App;

class ExampleImplementation implements ExampleContract
{
    public function getClaims(): array
    {
        return [];
    }
}

Sample Repository

https://github.com/Jubeki/pestphp-arch-issue

Pest Version

3.0.4

PHP Version

8.3.11

Operation System

macOS

Notes

Running with the debug flag outputs the following (Nothing more):

PHPUnit Started (PHPUnit 11.3.4 using PHP 8.3.10 (cli) on Darwin)
Test Runner Configured
Bootstrap Finished (vendor/autoload.php)
Test Suite Loaded (1 test)
Event Facade Sealed
Test Runner Started
Test Suite Sorted
Test Runner Execution Started (1 test)
Test Suite Started (phpunit.xml, 1 test)
Test Suite Started (Arch, 1 test)
Test Suite Started (P\Tests\Arch\ArchTest, 1 test)
Before First Test Method Called (P\Tests\Arch\ArchTest::setUpBeforeClass)
Before First Test Method Finished:
- P\Tests\Arch\ArchTest::setUpBeforeClass
Test Preparation Started (P\Tests\Arch\ArchTest::__pest_evaluable_preset__→_laravel_)
Before Test Method Called (P\Tests\Arch\ArchTest::setUp)
Before Test Method Finished:
- P\Tests\Arch\ArchTest::setUp
Test Prepared (P\Tests\Arch\ArchTest::__pest_evaluable_preset__→_laravel_)

Jubeki avatar Sep 11 '24 09:09 Jubeki

Wouldn't your Laravel application throw an error anyway if an interface wasn't applied correctly?

danielh-official avatar Sep 11 '24 20:09 danielh-official

Yeah it would. In this case the class was not tested yet and due to an update to an external component, the implementation didn't match anymore. Which would have resulted in a problem in production. The update was not pushed at this point. Tests did not detect it and the Arch Tests failed silently.

Jubeki avatar Sep 11 '24 20:09 Jubeki

Looking into it. As a side-note, this seems like something that should error our in tests regardless of whether or not you use architecture testing.

danielh-official avatar Sep 12 '24 18:09 danielh-official

After looking at the code, I believe one way to resolve this is to add a new src/Expectation method in this repository.

Potential Implementation

    /**
     * Asserts that the given expectation target implements all of its interfaces correctly.
     */
    public function toImplementAllInterfacesCorrectly(): ArchExpectation
    {
        return Targeted::make(
            $this,
            function (ObjectDescription $object): bool {
                $interfaceNames = $object->reflectionClass->getInterfaceNames();
                $className = $object->name;

                foreach ($interfaceNames as $interfaceName) {
                    if (! interface_exists($interfaceName)) {
                        return false;
                    }

                    $interfaceMethods = get_class_methods($interfaceName);
                    $classMethods = $object->methods->getIterator();

                    foreach ($interfaceMethods as $method) {
                        if (! in_array($method, $classMethods)) {
                            return false;
                        }

                        $interfaceMethod = new ReflectionMethod($interfaceName, $method);
                        $classMethod = new ReflectionMethod($className, $method);

                        // Check if the number of parameters matches
                        if ($interfaceMethod->getNumberOfParameters() !== $classMethod->getNumberOfParameters()) {
                            return false;
                        }

                        // Check if parameters are correctly typed
                        $interfaceParams = $interfaceMethod->getParameters();
                        $classParams = $classMethod->getParameters();

                        foreach ($interfaceParams as $index => $interfaceParam) {
                            if (! isset($classParams[$index])) {
                                return false;
                            }

                            $classParam = $classParams[$index];
                            if ($interfaceParam->getType() !== $classParam->getType()) {
                                return false;
                            }
                        }
                    }
                }

                return true;
            },
            "to implement all interfaces correctly '",
            FileLineFinder::where(fn (string $line): bool => str_contains($line, 'class')),
        );
    }

Steps

Apparently, it seems that testing for these functions is being handled in pest-plugin-arch repository.

So what I would need to do then is:

  1. Fork both this and that repository
  2. Replace the "pestphp/pest": "^3.0.0" in my local pest-plugin-arch's composer.json with my local pestphp/pest
  3. Implement the method in my local pestphp/pest
  4. Then create a test in my local pest-plugin-arch to ensure that it passes on all the cases an example class implements an example interface correctly and fails on all the cases it doesn't.

danielh-official avatar Sep 12 '24 19:09 danielh-official

Looking into it. As a side-note, this seems like something that should error our in tests regardless of whether or not you use architecture testing.

Yeah, I would have expected that too, but that doesn't seem to be the case. The Feature Testsuite passes without problems, only the Arch Testsuite fails silently without any output.

Thanks for looking into it.

Jubeki avatar Sep 13 '24 08:09 Jubeki