phpunit icon indicating copy to clipboard operation
phpunit copied to clipboard

Methods that return `never` cannot be doubled

Open othercorey opened this issue 3 years ago • 6 comments

Q A
PHPUnit version 9.5.24
PHP version 8.1
Installation Method Composer

Summary

Mocking a class with a method that returns never throws 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.

othercorey avatar Sep 18 '22 08:09 othercorey

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 avatar Sep 18 '22 15:09 sebastianbergmann

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

sebastianbergmann avatar Sep 18 '22 15:09 sebastianbergmann

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...

ondrejmirtes avatar Sep 18 '22 16:09 ondrejmirtes

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().

sebastianbergmann avatar Sep 18 '22 19:09 sebastianbergmann

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

othercorey avatar Sep 18 '22 22:09 othercorey

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()?

willemstuursma avatar May 03 '23 13:05 willemstuursma