method stubbing & parameter matching

[literal]

First of all: Phake ist great, thanks a lot for the work Mike!

I started using Phake today. Coming from PHPUnit mock objects, I was
used to parameter matching for stubbed methods being optional. I. e.
the "->with(...)" can be omitted and then the call arguments are
ignored.

So when converting this PHPUnit mock code (and we have a lot of this
kind of stubbing in our test suite):

$myMock->expects($this->any())->method('foo')->will($this-
>returnValue(true));

I quite naturally expected the Phake equivalent to be:

Phake::when($myMock)->foo()->returnValue(true);

But this will only work if foo() has no parameters. If foo has e. g.
two parameters, I must write:

Phake::when($myMock)->foo($this->anything(), $this->anything())-
>returnValue(true);

Not only does this make the statement even longer than the PHPUnit
version, it also took me quite some time to figure this out.

So I suggest that at least the docs should make this difference clear
on the "Basic Method Stubbing" page.

I understand where this behaviour comes from: Unlike PHPUnit, Phake
allows to stub a single method multiple times with different return
values depending on the call arguments (which is a great
improvement).

But couldn't Phake assume as many "$this->anything()" matchers as the
method has parameters, when no matchers or values are given in the
"Phake::when" statement?

Ok, you might wonder how one could then specify the "foo() called with
no args" case. But if "foo()" actually has (mandatory) parameters,
calling it without args would be an error anyway.

The only relevant case would be optional parameters. If "foo()" was
defined as "public function foo ($bar = null, $boom = null)", it might
make sense to explicitly stub calls without args or with only one arg.
But I guess this is seldomly needed and could probably be specified in
a different, more verbose way - e. g. "Phake::whenExactly()".

[Mike Lively]

Thanks for the suggestions. There are definitely some documentation
changes I need to make.

This is actually currently supported via an "undocumented" feature.

$myMock->expects($this->any())->method('foo')->will($this-
>returnValue(true));

can be re-expressed as

Phake::when($myMock)->foo(Phake::anyParameters())->returnValue(true);

Phake::anyParameters() will match any number of arguments containing
any value. I couldn't find this anywhere in my documentation so I will
open up a ticket to add this info.

Unfortunately if foo() doesn't have parameters and someone calls it
with parameters it doesn't necessarily yield any errors in runtime.
There is also the scenario during refactoring where someone adds new
parameters to a method, the tests could potentially continue to run
happily or error out in an unobvious way. I am kind of curious as to
whether or not Phake::anyParameters() satisfies your concerns. If not
I am definitely open to other alternatives. I would like to keep an
explicit different between no arg calls and calls where no arguments
matter.

Sorry again about the lack of documentation and thank you for the
feedback.

[literal]

Hi Mike,

Thanks for the quick and comprehensive reply!

I see your point, but I hope you won't be offended if I question the
concept behind it.

One of the wonderful features of Phake as compared to PHPUnit mocks is
that it brings back the "given/when/then" separation.

"Phake::when" is the "given" part. It sets up a specific environment
to make the SUT behave in a particular way. But it should not set up
any expectations. It's the job of "Phake::verify" to assert the SUT
calls its dependencies in the correct way (i. e. with the expected
arguments).

Moreover, setting up expectations in the "given" part will often
violate the common "one concept per test" rule. If I stub a method to
make my SUT do something particular and my test is about verifying the
correct result of what the SUT did, it should be out of the scope of
that test to check the stub was called in the expected way. I should
rather write another test for that (one that does not care about the
result of the operation).

From this point of view, the only proper use for the argument matchers
in "Phake::when" statements is to allow specifying different returns
(or exceptions) for different calls to a stubbed method.

And then it would be correct for "Phake::when($myMock)->foo()->... "
to set-up a stub for *any call* to "foo()", and for
"Phake::when($myMock)->foo('bar')->... " to stub any calls to "foo()"
where the first arg is 'bar', regardless of any further args.

Cheers
Stan

[Mike Lively]

I may have a good compromise. I would prefer to stay away from
modifying the functionality of a stub with no args to mean more than
just stubbing methods with no args. My hesitation is that coming from
the Mockito world, there isn't a similar behavior. I do aknowledge
that Java and PHP are two very different languages and as a result the
developers that use them have very different approaches and my Java
bias may be peaking through a little bit with this functionality.

So, the compromise that I see is as follows:

Phake::when($mock)->foo->thenReturn(true);

Basically if you stub a method "without the parenthesis" it stubs
every invocation. This could be implemented pretty easily utilizing
the __get() method on the stubber proxy and I think it would be fairly
easy to document. What are your thoughts?

[literal]

Hi Mike,

Your suggested solution would certainly improve the situation in my
view. But I see two drawbacks:

1. The syntax is nicely simple but somewhat counter-intuitive. The
docs can explain it, of course.
It's just not the sort of obvious feature which everyone would expect
to be there and work they way it does.

2. Stubbing all calls to "foo()" with a first arg of 'bar' regardless
of any further args will still require
a completely different and rather lengthy syntax, althogh in fact it
is just a variation of the "any call to foo"
case.


I didn't know Mockito, let alone that Phake was based on it (my
exposure to Java mostly dates back to the last millennium, and I've
been more of a C++-guy anyway... :-).

I respect your wish to mimic Mockito as closely as practicable in PHP.
But I also believe that modelling something new
after a good existing project provides the unique chance of making it
even better in ways that cannot be applied to the
original project because of backwards compatibility requirements. A
good example for this might be Twig, which is based on an early
version of the Django template engine.

So I would raise the question, whether the behaviour criticized in my
last post is actually a flaw in Mockito
and whether Phake could do better...

Cheers,
Stan

[Rick Wong]

I think Phake is a winner when it comes to readability of the syntax. The compromise that is mentioned seems to be in line with that.

Having this shorthand notation means:

* Lesser code = lesser bugs

* Readable difference between "mock every call to foo" and "mock calls to foo with param1"

* New users of Phake will learn about the important difference between foo() and foo(Phake::anyParameters())/shorthand sooner, *because* of the shorthand notation. This will save them some debugging time, I know it would have saved mine.

[Mike Lively]

I haven't forgotten about this feature request. I honestly haven't had the time to come up with a solution in code yet. If anyone wants to take a stab at it and submit a pull request for review I am certainly willing to take a look at it.

[Rick Wong]

Hey Mike, I would appreciated feedback on this pull request: https://github.com/mlively/Phake/pull/77

First time committing code via Github, so do tell me if there's anything wrong with it.

Rick

[Rick Wong]

P.S. __get() could probably just invoke __call(). That would reduce copypasta...