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

Methods that return never cannot be doubled #5048

Closed
othercorey opened this issue Sep 18, 2022 · 6 comments
Closed

Methods that return never cannot be doubled #5048

othercorey opened this issue Sep 18, 2022 · 6 comments
Assignees
Labels
feature/test-doubles Stubs and Mock Objects type/bug Something is broken version/9 Something affects PHPUnit 9 version/10 Something affects PHPUnit 10

Comments

@othercorey
Copy link

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 othercorey added the type/bug Something is broken label Sep 18, 2022
@sebastianbergmann sebastianbergmann self-assigned this Sep 18, 2022
@sebastianbergmann sebastianbergmann added the feature/test-doubles Stubs and Mock Objects label Sep 18, 2022
@sebastianbergmann sebastianbergmann changed the title Mocking a class with a method that returns never throws a PHP Fatal Error Methods that return never cannot be doubled Sep 18, 2022
@sebastianbergmann
Copy link
Owner

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

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

@ondrejmirtes
Copy link

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

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

othercorey commented Sep 18, 2022

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

@willemstuursma
Copy link
Contributor

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

mhsdesign added a commit to neos/flow-development-collection that referenced this issue Nov 4, 2023
Instead, we use anonymous classes for now.

Eventually this will be released: sebastianbergmann/phpunit#5048
mhsdesign added a commit to neos/flow-development-collection that referenced this issue Nov 7, 2023
- fix types where neos depends on it (by correcting types and adding `never`)

- adjust unit test as `never` cannot be doubled
eventually this will be fixed via: sebastianbergmann/phpunit#5048

- fix ci and style as neos 9 followup for #3218
mhsdesign added a commit to neos/flow-development-collection that referenced this issue Nov 7, 2023
With #3218 PHPStan level 1 was added to the whole Flow code base and CI for Flow 8. This upmerged change needs some adjustments to pass the CI in Flow 9

- fix types and remove deprecated code
- fix types where neos depends on it (by correcting types and adding `never`)
- adjust unit test as `never` cannot be doubled (eventually this will be fixed via: sebastianbergmann/phpunit#5048)
- fix ci and style as neos 9 followup for #3218
neos-bot pushed a commit to neos/cache that referenced this issue Nov 8, 2023
With neos/flow-development-collection#3218 PHPStan level 1 was added to the whole Flow code base and CI for Flow 8. This upmerged change needs some adjustments to pass the CI in Flow 9

- fix types and remove deprecated code
- fix types where neos depends on it (by correcting types and adding `never`)
- adjust unit test as `never` cannot be doubled (eventually this will be fixed via: sebastianbergmann/phpunit#5048)
- fix ci and style as neos 9 followup for neos/flow-development-collection#3218
neos-bot pushed a commit to neos/eel that referenced this issue Nov 8, 2023
With neos/flow-development-collection#3218 PHPStan level 1 was added to the whole Flow code base and CI for Flow 8. This upmerged change needs some adjustments to pass the CI in Flow 9

- fix types and remove deprecated code
- fix types where neos depends on it (by correcting types and adding `never`)
- adjust unit test as `never` cannot be doubled (eventually this will be fixed via: sebastianbergmann/phpunit#5048)
- fix ci and style as neos 9 followup for neos/flow-development-collection#3218
neos-bot pushed a commit to neos/flow that referenced this issue Nov 8, 2023
With neos/flow-development-collection#3218 PHPStan level 1 was added to the whole Flow code base and CI for Flow 8. This upmerged change needs some adjustments to pass the CI in Flow 9

- fix types and remove deprecated code
- fix types where neos depends on it (by correcting types and adding `never`)
- adjust unit test as `never` cannot be doubled (eventually this will be fixed via: sebastianbergmann/phpunit#5048)
- fix ci and style as neos 9 followup for neos/flow-development-collection#3218
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/test-doubles Stubs and Mock Objects type/bug Something is broken version/9 Something affects PHPUnit 9 version/10 Something affects PHPUnit 10
Projects
None yet
Development

No branches or pull requests

4 participants