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

RFC: Callable spies #712

Merged
merged 3 commits into from
Sep 13, 2018
Merged

Conversation

davedevelopment
Copy link
Collaborator

Here's a take on the request in #711

 $spy = spy(function($n) { return $n + 1;});

 array_map($spy, [1, 2]); // [2, 3]

 $spy->shouldHaveBeenCalled();
 $spy->shouldHaveBeenCalled()->twice();
 $spy->shouldHaveBeenCalled()->with(1)->once();
 $spy->shouldHaveBeenCalled()->with(2)->once();

 $spy->shouldHaveBeenCalled()->with(3); // throws...

Todos/considerations

  • Better error messages? Ideally we could identify that these are a special case and avoid complicating things showing the internals of ClosureWrapper::__invoke etc
  • Make it available for mocking as well? It would mean coming up with some syntax or probably overloading expects and allows somehow. Might get too complicated.
  • shouldNotHaveBeenCalledWith(123, 456, 789) ? I think I prefer this for specifying arguments with a not

@davedevelopment
Copy link
Collaborator Author

Also probably worth considering if it's even worth putting this in to mockery. It's simple enough to maintain I think, but the functionality isn't too difficult to roll on your own as you don't really need the code generation etc.

@davedevelopment davedevelopment changed the title WIP: Callable spies RFC: Callable spies Mar 9, 2017
@@ -92,6 +93,12 @@ public static function mock(...$args)
*/
public static function spy(...$args)
{
$args = func_get_args();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should not be necessary. ...$args is an array of all function arguments already.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@robertbasic
Copy link
Collaborator

@davedevelopment what's the status with this? Does it require any more work?

@davedevelopment
Copy link
Collaborator Author

@robertbasic no further progress. Error messages are not very friendly at the moment. Happy to merge as an experimental feature.

@tflori
Copy link
Contributor

tflori commented Sep 6, 2018

sorry, I somehow missed this MR.

In the mean time I found a problem we would have with this implementation in laravel: they have a lot of functions where they only allow \Closure. But I guess in this case the user has to write his own closure around the wrapper.

So for me this is good

@davedevelopment davedevelopment merged commit 9fbde0f into mockery:master Sep 13, 2018
@davedevelopment
Copy link
Collaborator Author

Merged, will leave undocumented as it's still considered experimental

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants