sebastianbergmann / phpunit

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

Doubled methods cannot be called from destructor on test double created by the return value generator #5874

Open frenchcomp opened 2 months ago

frenchcomp commented 2 months ago
Q A
PHPUnit version 11.2+
PHP version 8.2+
Installation Method Composer

Summary

Hi, Since PHPUnit 11.2, I have issue when I want mock Symfony Process. I have no issue with PHPUnit 11.1 and priors versions.

Current behavior

When a mock the Symfony Process class, when I call a method on an instance of this mock, I have the error : _Typed property MockObject_Process_XXXX::$__phpunitstate must not be accessed before initialization I had test with define an expects() on the mock, but I had always the error.

There was 1 error:

1) Test\IssueTest::testBug Error: Typed property MockObject_Process_62f16cad::$__phpunit_state must not be accessed before initialization

How to reproduce

I have reproducted the issue in this repository https://github.com/frenchcomp/test_phpunit_11_2_process You can run the test case in the src folder : vendor/bin/phpunit src/IssueTest.php

sebastianbergmann commented 2 months ago

Are you using the latest version of PHPUnit 11.2?

sebastianbergmann commented 2 months ago

I am sorry to say, but https://github.com/frenchcomp/test_phpunit_11_2_process is not a minimal, self-contained, reproducing test case.

Without such a minimal, self-contained, reproducing test case I will not be able to investigate this issue.

frenchcomp commented 2 months ago

Yes :

$ composer show | grep phpunit
phpunit/php-code-coverage          11.0.3 Library that provides collection, processing, and rendering functionality for PHP code coverage information.
phpunit/php-file-iterator          5.0.0  FilterIterator implementation that filters files based on a list of suffixes.
phpunit/php-invoker                5.0.0  Invoke callables with a timeout
phpunit/php-text-template          4.0.0  Simple template engine.
phpunit/php-timer                  7.0.0  Utility class for timing
phpunit/phpunit                    11.2.3 The PHP Unit Testing framework.
sebastianbergmann commented 2 months ago

Are you using the latest version of PHPUnit 11.2?

Yes, you are (according to https://github.com/frenchcomp/test_phpunit_11_2_process/blob/main/composer.lock#L570).

frenchcomp commented 2 months ago

I am sorry to say, but https://github.com/frenchcomp/test_phpunit_11_2_process is not a minimal, self-contained, reproducing test case.

Without such a minimal, self-contained, reproducing test case I will not be able to investigate this issue.

What is a "minimal, self-contained, reproducing test case" ?

sebastianbergmann commented 2 months ago

What is a "minimal, self-contained, reproducing test case" ?

Something that does not depend on a third-party library such as Symfony Process.

sebastianbergmann commented 2 months ago

This is the full stack trace:

Error: Typed property MockObject_Process_be57136f::$__phpunit_state must not be accessed before initialization

/home/sb/test_phpunit_11_2_process/vendor/symfony/process/Process.php:215
/home/sb/test_phpunit_11_2_process/src/IssueTest.php:52

Process.php:215 is inside the destructor of Process.

frenchcomp commented 2 months ago

I updated my repo with a selfcontained class and removed process. The issue is called when a method is called in the destructor of the doubled instance.

sebastianbergmann commented 2 months ago

In https://github.com/frenchcomp/test_phpunit_11_2_process/blob/18a41ac0d23c0d4f16483d9476ec0a7c77f3dbc5/src/IssueTest.php you have:

class IssueTest extends TestCase
{
    /**
     * @var Process
     */
    private $process;

    public function getProcessMock(): MockObject&Process
    {
        if (!$this->process instanceof Process) {
            $this->process = $this->createMock(Process::class);
        }

        return $this->process;
    }

    public function testBug()
    {
        $p = $this->getProcessMock();

        self::assertInstanceOf(
            Process::class,
            $p->setWorkingDirectory("foo"),
        );
    }
}

Here is what I understand to be happening:

I was not able to reproduce in a minimal test case (without Process) whether running the destructor of a test double object created by the return value generator and where the destructor calls a doubled method is actually the problem, but at least some evidence points there. This needs more debugging, for which I currently do not have the time, sorry.

If this really is the case, then yes: you have a found a bug. However, I suspect that you assume in your test that $p->setWorkingDirectory("foo") returns a reference to $p, along the lines of the fluent API of this class. This was never the case. If you are doubling an object that provides a fluent API you need to configure the return value of the methods you use in your test accordingly using willReturnSelf():

class IssueTest extends TestCase
{
    public function testBug()
    {
        $p = $this->createMock(Issue::class);

        $p
            ->method('setWorkingDirectory')
            ->willReturnSelf();

        self::assertInstanceOf(
            Issue::class,
            $p->setWorkingDirectory("foo"),
        );
    }
}

(https://github.com/sebastianbergmann/phpunit/issues/5874#issuecomment-2178775859 happened in the middle of me debugging the issue and writing this comment)

frenchcomp commented 2 months ago

My repo, you have an example without Process, only with PHPUnit and a sample code reproducing the error, and I have always the issue.

1) Test\IssueTest::testBug Error: Typed property MockObject_Issue_cfe8d93d::$__phpunit_state must not be accessed before initialization

/home/richard/Bureau/test_phpunit_11_2_process/src/Issue.php:11 /home/richard/Bureau/test_phpunit_11_2_process/src/IssueTest.php:20

frenchcomp commented 2 months ago

I got it.


    public function callWithStaticAsReturnType(string $cwd): static
    {
        return $this;
    }

    public function callWithSelfAsReturnType(string $cwd): self
    {
        return $this;
    }

With "self" as return type, i have no error, but with static i have an error

sebastianbergmann commented 2 months ago

Interestingly, the return value generator has a special case for static but not for self:

https://github.com/sebastianbergmann/phpunit/blob/11.2.3/src/Framework/MockObject/Runtime/ReturnValueGenerator.php#L95-L97

sebastianbergmann commented 2 months ago

Thank you, @frenchcomp, for bringing this to my attention. I hope to be able to look into this more closely soon.

frenchcomp commented 2 months ago

You' re welcome. Thank you for you works. :)

VincentLanglet commented 1 week ago

I got the same issue while mocking the interface https://github.com/symfony/symfony/blob/7.2/src/Symfony/Component/Validator/Violation/ConstraintViolationBuilderInterface.php#L46 and I can confirm than changing static into self solve the issue.