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

Doubling interfaces with property hooks #5945

Closed
davidbyoung opened this issue Sep 8, 2024 · 12 comments
Closed

Doubling interfaces with property hooks #5945

davidbyoung opened this issue Sep 8, 2024 · 12 comments
Assignees
Labels
feature/test-doubles Test Stubs and Mock Objects type/enhancement A new idea that should be implemented

Comments

@davidbyoung
Copy link
Sponsor

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

@davidbyoung davidbyoung added the type/enhancement A new idea that should be implemented label Sep 8, 2024
@sebastianbergmann sebastianbergmann added feature/test-doubles Test Stubs and Mock Objects type/change-in-php-requires-adaptation A change in PHP requires a change so that existing PHPUnit functionality continues to work labels Sep 9, 2024
@sebastianbergmann
Copy link
Owner

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 sebastianbergmann removed the type/change-in-php-requires-adaptation A change in PHP requires a change so that existing PHPUnit functionality continues to work label Sep 9, 2024
@sebastianbergmann
Copy link
Owner

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

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

Crell commented Sep 9, 2024

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

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

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 #5945 (comment). Doing so would not work when the interface (or extendable class) to be doubled has a method named property of its own.

@sebastianbergmann sebastianbergmann changed the title Mock interfaces with properties that have get-hooks in PHP 8.4 Doubling interfaces with property hooks Sep 10, 2024
@sebastianbergmann sebastianbergmann self-assigned this Sep 10, 2024
@davidbyoung
Copy link
Sponsor Author

Yep, that looks great to me!

@davidbyoung
Copy link
Sponsor Author

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

@Crell
Copy link

Crell commented Sep 12, 2024

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

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

No, this is not feasible as I already wrote in #5945 (comment).

@sebastianbergmann
Copy link
Owner

sebastianbergmann commented Sep 15, 2024

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

Superseded by #5948.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/test-doubles Test Stubs and Mock Objects type/enhancement A new idea that should be implemented
Projects
None yet
Development

No branches or pull requests

4 participants