sebastianbergmann / phpunit

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

Doubling interfaces with property hooks #5945

Closed davidbyoung closed 1 week ago

davidbyoung commented 2 weeks ago

With the introduction of property hooks, we'll likely have interfaces with properties defined. Let's say I have the following interface in PHP 8.4:

interface Foo
{
    public string $bar { get; }
}

When creating a mock for Foo, I'll need to mock the get-value for $bar. AFAIK, there is no way (or issue created for it) to be able to mock it, eg something like:

use PHPUnit\Framework\TestCase;

class FooTest extends TestCase
{
    public function testBar(): void
    {
         $foo = $this->createMock(Foo::class);
         // This is a made up method, but gets what I'm trying to do across
         $foo->property('bar')
             ->willReturn('baz');

         // Do tests and assertions...
    }
}

Of course, I could just create an anonymous class that implements Foo and sets $bar, but I may need to mock other methods that exist in an interface in real test cases. What's the best way to do this? Or does additional functionality need to be added to PHPUnit to support this?

Thanks for all you do for the PHP community!!

sebastianbergmann commented 2 weeks ago

I have not thought about whether PHPUnit needs to be adapted with regards to property hooks. Maybe @iluuu1994 and @Crell can share insight and/or provide guidance what they think PHPUnit should do?

sebastianbergmann commented 2 weeks ago

Removed the type/change-in-php-requires-adaptation label as that is used when a change in PHP requires a change so that existing PHPUnit functionality continues to work. This is about supporting a new PHP language feature.

iluuu1994 commented 2 weeks ago

@sebastianbergmann I don't have a good understanding of PHPUnits mocking. Interface properties can be implemented as either plain properties, or properties with the corresponding hooks. If the property needs behavior, it seems like yes, the mock generator needs some way to allow declaring hooks on properties. Sorry if that wasn't a particularly helpful response. In that case, please clarify your question.

Crell commented 2 weeks ago

I almost never use mocks and encourage others to avoid them as well, so I don't have much stake here. In general, from an API perspective, I think something like @davidbyoung's proposal is probably the way to go. I also don't know enough about the PHPUnit internals to offer useful advice there.

davidbyoung commented 2 weeks ago

FWIW my current workaround is to change my mocks to use an abstract class that opens up getters and setters for properties defined in an interface, eg

abstract class MockableFoo implements Foo
{
    public string $bar;
}

Then, use that in my test cases:

use PHPUnit\Framework\TestCase;

class FooTest extends TestCase
{
    public function testBar(): void
    {
         $foo = $this->createMock(MockableFoo::class);
         $foo->bar = 'baz';

         // Do tests and assertions...
    }
}

As properties on interfaces become more and more adopted after PHP 8.4 is released, though, I think this workaround may prove to be cumbersome because it requires manual abstract class definitions for each interface being mocked.

sebastianbergmann commented 2 weeks ago

I think the following could work.

When we have an interface I

interface I
{
    public string $property { get; set; }
}

then PHPUnit could generate code for a test double for I that looks along the line of this

final class C implements I
{
    public string $property {
        get {
            // Either return the configured return value or
            // return the simplest value that satisfies the property's type declaration
        }

        set (string $value) {
            // Record that method named "set" was called with $value
            // (so that corresponding mock object expectation can be verified)
        }
    }
}

Here is an example of what configuring test stubs and mock objects could look like:

$stub = $this->createStub(I::class);
$stub->method('$property::get')->willReturn('value');

$mock = $this->createMock(I::class);
$mock->expects($this->once())->method('$property::set')->with('value');

I would like to avoid introducing a new method named property as proposed in https://github.com/sebastianbergmann/phpunit/issues/5945#issue-2512560110. Doing so would not work when the interface (or extendable class) to be doubled has a method named property of its own.

davidbyoung commented 2 weeks ago

Yep, that looks great to me!

davidbyoung commented 1 week ago

Since abstract properties in abstract classes can also exist, I imagine we should permit mocking of those as well.

Crell commented 1 week ago

The syntax of ->method("$propery::set") feels very clumsy to me, and error prone. Is it not feasible to give properties their own method(s)?

sebastianbergmann commented 1 week ago

Is it not feasible to give properties their own method(s)?

No, this is not feasible as I already wrote in https://github.com/sebastianbergmann/phpunit/issues/5945#issuecomment-2339804681.

sebastianbergmann commented 1 week ago

Here is an example of what configuring test stubs and mock objects could look like:

$stub = $this->createStub(I::class);
$stub->method('$property::get')->willReturn('value');

$mock = $this->createMock(I::class);
$mock->expects($this->once())->method('$property::set')->with('value');

In an effort to make the argument passed to method() less clumsy, the method now also accepts a PropertyHook value object that hides PHP's implementation detail of how property hooks are named from PHPUnit's users:

$stub = $this->createStub(I::class);
$stub->method(PropertyHook::get('property'))->willReturn('value');

$mock = $this->createMock(I::class);
$mock->expects($this->once())->method(PropertyHook::set('property'))->with('value');

The above already works in the current state of #5948.

sebastianbergmann commented 1 week ago

Superseded by #5948.