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

Add withArgs() to validate via closure passed arguments at once #459

Closed
gmazzap opened this issue Mar 23, 2015 · 27 comments
Closed

Add withArgs() to validate via closure passed arguments at once #459

gmazzap opened this issue Mar 23, 2015 · 27 comments
Labels
Feature Request request a new feature

Comments

@gmazzap
Copy link
Contributor

gmazzap commented Mar 23, 2015

I think would be handy a method like

withArgs($closure)

that would work similar to

with( Mockery::on($closure) );

with the only difference that in first case the $closurereceives all the arguments passed to the target method.

@padraic padraic added the Feature Request request a new feature label Mar 23, 2015
@aik099
Copy link

aik099 commented Mar 23, 2015

With proposed withArgs($closure) syntax it's impossible to uniquely detect which expectation should fire because one with closure will be used in all cases.

Instead I recommend using ->andReturnUsing($closure). Then the $closure receives all the arguments passed.

@gmazzap
Copy link
Contributor Author

gmazzap commented Mar 23, 2015

Thanks @aik099 for your response.

To use andReturnUsing was my first thought, but to make test fail if arguments are not valid, the only way is to manually throw an exception, and it's quite verbose because an explicative message should be added too or is hard to understand why the test failed.

The only aim of my proposal is to have less verbose test code, so...

Well, yes, I assume that withArgs should be used as unique or last expectation for a method.

Thats IMO should not be a problem: for example, currently you can't mix andReturnUsing and andReturn in same test for same method, and in the same way you couldn't mix with and withArgs...

@beni0888
Copy link
Contributor

@padraic @aik099 @Giuseppe-Mazzapica So, what do you think about this feature? Should it be added to the API? Can I start working on it?

@davedevelopment
Copy link
Collaborator

Can for me!

@aik099
Copy link

aik099 commented Jan 28, 2016

How we can distinguish between 2 cases:

  • when withArgs($closure) means a I want this method to be called with 1 argument, which is closure
  • when withArgs($closure) means a I want the closure to handle argument matching for me

?

@aik099
Copy link

aik099 commented Jan 28, 2016

Ah, the withArgs get's called inside with using func_get_args(). In that case, yes it's possible to make withArgs to accept a closure.

@davedevelopment
Copy link
Collaborator

Yeah, withArgs as it stands is intended to take an array of all expected arguments, so we can just switch on type.

@beni0888
Copy link
Contributor

ok, I'll try it :)

beni0888 added a commit to beni0888/mockery that referenced this issue Feb 2, 2016
…functionality to support a closure as only argument to validate all passed arguments at once.
beni0888 added a commit to beni0888/mockery that referenced this issue Feb 2, 2016
beni0888 added a commit to beni0888/mockery that referenced this issue Feb 2, 2016
beni0888 added a commit to beni0888/mockery that referenced this issue Feb 2, 2016
beni0888 added a commit to beni0888/mockery that referenced this issue Feb 3, 2016
@peixinchen
Copy link

peixinchen commented Dec 19, 2016

In my opinion, withArgs($argsOr$cloure) should be separated as withArgs(array $args) and withArgsUsing(Closure $closure), and it could solve the problom.

BTW: The document has been updated but not the code, so it's a BUG now.

@Revisor
Copy link

Revisor commented Dec 20, 2016

Yeah, just came here to say the same. The documentation for 0.9 regarding withArgs() doesn't match the code.

@davedevelopment
Copy link
Collaborator

I'm not sure I understand, didn't this get merged in to the master branch? If someone could point me directly to the differences, I'll take a look.

@Revisor
Copy link

Revisor commented Dec 20, 2016

The documentation, whose title says "Mockery Docs 0.9 documentation" contains this:

withArgs(closure)

Instead of providing a built-in matcher for each argument, you can provide a closure that matches all passed arguments at once. The given closure receives all the arguments passed in the call to the expected method. In this way, this expectation only applies to method calls where passed arguments make the closure evaluates to true.

However that is not true for Mockery 0.9, where Expectation::withArgs() expects an array.

It is also not true for the master branch, where withArgs() expects either an array (like before), or a closure.

@davedevelopment
Copy link
Collaborator

@Revisor Thanks! I think there are a few problems in play here, will try and get them sorted.

@davedevelopment
Copy link
Collaborator

Fixed the documentation title in 0f183fa

So now the stable docs have "Mockery docs 0.9" in the title, the latest have "Mockery docs 1.0-alpha".

The documentation for the withArgs([$arg1, $arg2]) syntax is just above the withArgs($closure) documentation, but it's not all that clear, perhaps it should be a separate code block.

@Revisor
Copy link

Revisor commented Dec 20, 2016

Thank you.
Ah, I didn't notice withArgs in the block above, my eyes skimmed it.

May I suggest, instead of

with(arg1, arg2, ...) / withArgs(array(arg1, arg2, ...))

Format it like this

with(arg1, arg2, ...)
withArgs(array(arg1, arg2, ...))

What do you think?

@davedevelopment
Copy link
Collaborator

Sure, done in 89f7708

@halfer
Copy link

halfer commented Jan 15, 2017

@Revisor: if I want to use a closure to match method params, do I have to use the master branch at present? The change in master to support closures went in on 1 March 2016 (b4b978c) but it does not seem to be in 0.9.6 or 0.9.7.

@Revisor
Copy link

Revisor commented Jan 15, 2017

That's more a question for davedevelopment, but as far as I can see, there is currently no tagged version supporting closure as an argument for withArgs(). You would have to surf dangerously on the development branch.

But maybe Dave could tag the current version as 1.0 alpha?

@halfer
Copy link

halfer commented Jan 16, 2017

Thanks @Revisor, well surfing dangerously it is then :). I will revisit in a month or two to see if a new release has been made.

@rparsi-commer
Copy link

@davedevelopment @Revisor @halfer
What's the update on this?
I need to be able to match method params using a closure, to tell Mockery
if the params passed to the mocked method are for the specific expectation.

@halfer
Copy link

halfer commented Mar 27, 2017

@rparsi-commer: the latest release 0.9.9 does not have the closure facility, so that still requires dev-master. Unfortunately that also bumped up the minimum PHP version - I am still using Ubuntu 14.04 for development, so was still using 5.5. It gave me a good excuse to upgrade to 7.x, which is pretty easy to do.

It would be nice to see a release tag including this, but if you are stuck and don't want to build against HEAD for stability reasons, fork the code and create your own tag/branch. You can point your composer.json at that instead.

@rparsi-commer
Copy link

@halfer So closures don't work at all?
I'm using Mockery version 0.9.x-dev (branch alias), I think my actual installed version is older than 0.9.4.
I have also tried using Expectation::andReturnUsing($closure).

No matter what I do my closure doesn't get called.
I've also tried Expectation::ordered() when creating the expectations.

I'm mocking a method that has signature ($stringValue, BlahInterface $blah) where
the actual value of properties in $blah differs on each call.

How do I do this with current stable release?

@halfer
Copy link

halfer commented Mar 27, 2017

So closures don't work at all?

No @rparsi-commer, they work just fine. We're probably talking at cross-purposes; my point was that there is not a release for this, so you have to build against the master of Mockery.

Here is the line in my composer.json:

    "mockery/mockery": "dev-master",

How do I do this with current stable release?

You don't, it is not in a stable release. As I have explained, if you don't want to point at a moving target, fork the code yourself and point at that.

@rparsi-commer
Copy link

@halfer Ok, thanks for the clarification.

@davedevelopment
Copy link
Collaborator

There is a 1.0.0-alpha1 tag, I'd recommend moving to that. I find it stable enough for my tests.

@halfer
Copy link

halfer commented Apr 5, 2017

Lovely, will do @davedevelopment.

@robertbasic
Copy link
Collaborator

If I'm understanding this thread correctly, the feature requested is implemented, and available since 1.0.0-alpha1 tag, so I'm going to close this issue.

Please open a new issue if there's still something amiss with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request request a new feature
Projects
None yet
Development

No branches or pull requests

10 participants