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

Support for omitting parameter default values for willReturnMap() #5551

Closed
Schrank opened this issue Oct 27, 2023 · 2 comments
Closed

Support for omitting parameter default values for willReturnMap() #5551

Schrank opened this issue Oct 27, 2023 · 2 comments
Assignees
Labels
feature/test-doubles Test Stubs and Mock Objects type/enhancement A new idea that should be implemented
Milestone

Comments

@Schrank
Copy link
Contributor

Schrank commented Oct 27, 2023

There are at least two ways to tell a test double to return a value: with()->willReturn() and willReturnMap().

While with()->willReturn() kind of works as I expect it, willReturnMap() does not.

with()->willReturn() seems to "ignore" the default values and returns the willReturn() although the default values are not part of the test double configuration.

Example:

// We have a method like this:
public function methodWithDefaultValue(int $return, int $subtract = 0): int
{
        return $return - $subtract;
}

The following code will work as expected and be a valid test and $this->methodWithDefaultValue($a) will return 6.

 $a = 6;

$this->exampleMock
    ->method('methodWithDefaultValue')
    ->with($a)
    ->willReturn($a);

While this does not work:

        $a = 6;

        $this->exampleMock
            ->method('methodWithDefaultValue')
            ->willReturnMap([[$a, $a]]);

It returns an error TypeError: MockObject_Example_5ee46b65::methodWithDefaultValue(): Return value must be of type int, null returned

And this is already more than expected, because in the horrible past, the error would be: Failed asserting that null is identical to 6. if we don't define return types on the methods.

My situation is: I'm writing not enough unit tests, therefore I forget the problem often. And in combination with PHPStorm not autocompleting a method with its optional parameters, I often forget, that the method I double even has more than the required parameter.

To make willReturnMap() work, we need to pass all parameters to the mapping array, looking like this:

$this->exampleMock
            ->method('methodWithDefaultValue')
            ->willReturnMap([[$a, 0, $a]]);

then the code will return our expected 6.

After a nice chat with Sebastian, we thought about how to fix it and there are two location, where it might make sense to do it:

  1. When creating the ReturnValueMap or
  2. when matching against it.

Sebastian already said, 1. makes more sense, because then we know we have to handle default values, because the others are passed to us. While if we do it while matching, we first need to determine wether the values we have are default or not and over all, Sebastian said it is more complex.

@sebastianbergmann I hope I didn't forget anything. The good news: with()->willReturn() works as expected <3

Thanks for all your work!

@Schrank Schrank added the type/enhancement A new idea that should be implemented label Oct 27, 2023
@Schrank Schrank changed the title Improve DX for return value mapping and parameter default values Improve DX for willReturnMap and parameter default values Oct 27, 2023
@sebastianbergmann sebastianbergmann added the feature/test-doubles Test Stubs and Mock Objects label Oct 27, 2023
@sebastianbergmann sebastianbergmann self-assigned this Oct 27, 2023
@sebastianbergmann sebastianbergmann added this to the PHPUnit 10.5 milestone Oct 27, 2023
@sebastianbergmann sebastianbergmann changed the title Improve DX for willReturnMap and parameter default values Improve DX for willReturnMap() and parameter default values Oct 27, 2023
@sebastianbergmann sebastianbergmann changed the title Improve DX for willReturnMap() and parameter default values Support for omitting parameter default values for willReturnMap() Oct 28, 2023
@sebastianbergmann
Copy link
Owner

@Schrank Can you please test whether the changes I just pushed also work for your real-world use cases? Thanks!

@Schrank
Copy link
Contributor Author

Schrank commented Oct 30, 2023

Works like a charm! <3

Thanks for the quick implementation

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

2 participants