sebastianbergmann / phpunit

The PHP Unit Testing framework.
https://phpunit.de/
BSD 3-Clause "New" or "Revised" License
19.66k stars 2.2k forks source link

Methods that return `never` cannot be doubled #5048

Closed othercorey closed 11 months ago

othercorey commented 2 years ago
Q A
PHPUnit version 9.5.24
PHP version 8.1
Installation Method Composer

Summary

Mocking a class with a method that returns neverthrows a php fatal error in MockClass::generate().

Current behavior

PHP Fatal error:  Cannot use 'never' as class name as it is reserved in /home/runner/work/bake/bake/vendor/phpunit/phpunit/src/Framework/MockObject/MockClass.php(51) : eval()'d code on line 3

That error seems to be thrown from executing this code:

class never
{
}

class Mock_never_b52d52ab extends never implements PHPUnit\Framework\MockObject\MockObject
{
    use \PHPUnit\Framework\MockObject\Api;
    use \PHPUnit\Framework\MockObject\Method;
    use \PHPUnit\Framework\MockObject\MockedCloneMethodWithVoidReturnType;
}

How to reproduce

Create a mock of a class with a method that returns never:

    public function abort(string $message, int $code = CommandInterface::CODE_ERROR): never
    {
        $this->error($message);

        throw new StopException($message, $code);
    }
        $io = $this->createMock(ConsoleIo::class);
        $io->expects($this->never())->method('abort');

Expected behavior

The mock code doesn't generate a class named never.

sebastianbergmann commented 2 years ago

Thank you for bringing this to my attention.

Given this interface:

interface I
{
    public function m(): never;
}

when TestCase::createStub(I::class) is called in a test then the following test double code is generated:

class Mock_InterfaceWithMethodReturningNever_f206cac2 implements PHPUnit\Framework\MockObject\MockObject, PHPUnit\TestFixture\MockObject\InterfaceWithMethodReturningNever
{
    use \PHPUnit\Framework\MockObject\Api;
    use \PHPUnit\Framework\MockObject\Method;
    use \PHPUnit\Framework\MockObject\MockedCloneMethodWithVoidReturnType;

    public function neverReturns(): never
    {
        $__phpunit_arguments = [];
        $__phpunit_count     = func_num_args();

        if ($__phpunit_count > 0) {
            $__phpunit_arguments_tmp = func_get_args();

            for ($__phpunit_i = 0; $__phpunit_i < $__phpunit_count; $__phpunit_i++) {
                $__phpunit_arguments[] = $__phpunit_arguments_tmp[$__phpunit_i];
            }
        }

        $this->__phpunit_getInvocationHandler()->invoke(
            new \PHPUnit\Framework\MockObject\Invocation(
                'PHPUnit\TestFixture\MockObject\InterfaceWithMethodReturningNever', 'neverReturns', $__phpunit_arguments, 'never', $this, false
            )
        );
    }
}

When the neverReturns() method is called on the stub then the following additional code is generated:

class never
{
}

class Mock_never_5c984be4 extends never implements PHPUnit\Framework\MockObject\MockObject
{
    use \PHPUnit\Framework\MockObject\Api;
    use \PHPUnit\Framework\MockObject\Method;
    use \PHPUnit\Framework\MockObject\MockedCloneMethodWithVoidReturnType;
}

This happens because no return value has been configured for the stubbed method. The default return value generator is not aware of never and treats it as a type for which the test double code generator needs to be called recursively. This is obviously wrong.

However, even if the default return value generator were aware of never, the generated code for Mock_InterfaceWithMethodReturningNever_f206cac2::neverReturns() would not work as it has an implicit return statement which triggers the error shown below:

TypeError: Mock_InterfaceWithMethodReturningNever_f206cac2::neverReturns(): never-returning function must not implicitly return

Turns out, I only ever implemented support for the never return type in the test double code generator and totally forgot about the test double runtime.

To "solve" the never-returning function must not implicitly return issue the generated code would either have to exit() or raise an exception. However, I do not think that either of these are appropriate.

Right now, I have no idea what the right thing to do here is. I am open to suggestions.

sebastianbergmann commented 2 years ago

@muglug @ondrejmirtes Do you have an idea what the correct behaviour for PHPUnit would be here? Thanks!

ondrejmirtes commented 2 years ago

So never can mean three different things:

1) Infinite execution 2) Script exit() 3) Always-thrown exception

It's hard to infer what the mocked method does out of these three options. At the same time, 1) and 2) are impractical in testing environment so I'd always follow 3).

PHPUnit already must have some differentiating logic what to do between a type that returns a value (like : int) vs. : void.

A quickfix here would be to simply not return anything (to behave same way as : void) which would avoid the fatal error.

After that it's a matter of personal taste what should be done and how the behaviour should be configured in case the method is called:

a) Read @throws above the method, mock and throw that exception? b) Require the mocked method to always be configured via a new method on MockBuilder so that the user always specifies what should be done?

There might be other options too...

sebastianbergmann commented 2 years ago

Thank you, @ondrejmirtes. Unfortunately, "not return anything" does not work as it results in the never-returning function must not implicitly return error mentioned above.

I think I will change the runtime behaviour to throw new Exception by default and let that be configurable using throwsException().

othercorey commented 2 years ago

To confirm, that abort() function does throw an exception.

willemstuursma commented 1 year ago

We would like to assert that a method with the never return type is invoked.

During tests, infinite execution or exiting the process does not seem appropriate. The only thing that remains, is throwing an exception. Hence, during tests, a never method should always throw.

How about introducing a special exception, that could be overridden with willThrowException()?