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

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

Open
Jubeki opened this issue Sep 11, 2024 · 5 comments
Open

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

Jubeki opened this issue Sep 11, 2024 · 5 comments
Labels

Comments

@Jubeki
Copy link

Jubeki commented Sep 11, 2024

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 Jubeki added the bug label Sep 11, 2024
@danielh-official
Copy link

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

@Jubeki
Copy link
Author

Jubeki commented Sep 11, 2024

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.

@danielh-official
Copy link

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
Copy link

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.

@Jubeki
Copy link
Author

Jubeki commented Sep 13, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants