Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Feature/mock features #788

Merged
merged 3 commits into from Jan 18, 2013

Conversation

Projects
None yet
3 participants
Member

blainesch commented Jan 16, 2013

Possible fix for #784

Contributor

jails commented Jan 16, 2013

I'm not sure you can really test the ordering of calls with this implementation.

What I mean here is:

assertCalled($mock, 'method1', 'eq', 1);
assertCalled($mock, 'method2', 'eq', 1);

//is different from:

assertCalled($mock, 'method2', 'eq', 1);
assertCalled($mock, 'method1', 'eq', 1);

Imo you need to keep a trace of your "run through" to not restart each time on your $mock::results (this of course imply an additionnal method, i.e a kind of $mock->flush() which allow restarting on $mock::results)

So regarding the syntax something like this looks like more "fluent" for managing the ordering of calls.

assertTrue($mock->call('method1')->with('a', 'b')->eq(1)
                ->call('method2')->with('c', 'd')->eq(1)->success());
Contributor

jails commented Jan 16, 2013

Just wonder if we also need to differentiate calls from static calls.

Member

blainesch commented Jan 16, 2013

I'm nervous about adding a lot of methods to Mock since every method I manually put it will not only be in string form but also can overwrite a method on the mocked class.

The $results currently store the arguments and return values so it's possible to change/add assertion methods to compensate for with, however testing the order of calls isn't something that's been implemented so would require a refactor.

@jails I'm not sure what you mean by your second comment "differentiate calls from static calls". Can you point me to the code you're looking at?

Contributor

jails commented Jan 16, 2013

Imo you don't need to add all the methods in Mocker i.e.

assertTrue(Mocker::chain($mock)->called('method1')
                ->with('a', 'b')
                ->eq(1)
                ->called('method2')
                ->with('c', 'd')
                ->eq(1)
                ->success());

Mocker::chain($mock) returns what you called $results. So you only need here is to change $results to be an object instance which contain all the logic about the "chaining".

Mocker::chain($mock) will return $mock->results if $mock is an instance or $mock::results if $mock is a string. The second advantage here is you can change $results to $resultsMinimiumChanceToConflict if you consider $results may conflict.

For the calls/static calls, I wonder if we need to differentiate '::' from '->' call or maybe I missed something in your implementation ?

Member

blainesch commented Jan 16, 2013

So the only functionality that isn't inside Mocker is logging the results in the order they were called. After that all the other functionality could be implemented in addition to the current pull request, unless you think that Mocker::chain should be implemented instead of assertCalled and assertNotCalled?

Honestly, all the chaining doesn't look very elegant to me. Maybe we can extend assertCalled to also check arguments similar to your with, update Mocker to add order to the method calls and then possible create another assertion for testing "before" assertCalledBefore($stub, 'method1', 'method2')?

Contributor

jails commented Jan 16, 2013

It was just my thought on this subject, but I don't know if it's the best solution. I was not necessarily looking for something elegant first but a possible way to describe a flow of calls. May I miss something but I don't really see how you can describe a ->called('method1')->called('method2')->called('method1') with assertCalledBefore.

Regarding the elegance there will always be a way to hide the Mocker::chain($mock) with an assertCalled or something like that. But maybe @davidpersson @nateabele or @gwoo see what is the best way to go here.

Blaine Schmeisser added some commits Jan 15, 2013

Blaine Schmeisser Add logging ability to Mocker for later assertions. c87910b
Blaine Schmeisser Update pass by reference logic.
Most methods cannot be passed by reference, however __get should mirror it's parent.
77a0f46
Blaine Schmeisser Add MockerChain to make assertions easier and extendible. 8c138f6
Member

blainesch commented Jan 17, 2013

Alright @jails I'm convinced. I'm also not sure I like the class name MockerChain.

@nateabele nateabele commented on the diff Jan 17, 2013

analysis/Debugger.php
@@ -16,7 +16,7 @@
* The `Debugger` class provides basic facilities for generating and rendering meta-data about the
* state of an application in its current context.
*/
-class Debugger extends \lithium\core\Object {
+class Debugger extends \lithium\core\StaticObject {
@nateabele

nateabele Jan 17, 2013

Owner

A quick scan of Debugger seems to show it doesn't really need to extend anything. Thoughts?

@blainesch

blainesch Jan 17, 2013

Member

I assumed it was an unwritten lithium convention to always extend Object or StaticObject? It'll probably make it faster not to extend anything, but without benchmarks I'd probably say very minimal performance improvement. I like that most objects extend core functionality.

The only side effect I can think of is if it doesn't extend one of them it cannot be used by Mocker since it will no longer have access to applyFilter.

@nateabele

nateabele Jan 17, 2013

Owner

I assumed it was an unwritten lithium convention to always extend Object or StaticObject?

Really only when necessary.

The only side effect I can think of is if it doesn't extend one of them it cannot be used by Mocker since it will no longer have access to applyFilter.

I'm not sure I follow, since Debugger doesn't have any filterable methods.

@blainesch

blainesch Jan 17, 2013

Member

Right, no filterable methods exist. However if I try to autoload lithium\analysis\debugger\Mock now all (well most) methods are filterable. However if the class doesn't extend Object or StaticObject the Mocker autoloader won't even try to create it.

@nateabele

nateabele Jan 17, 2013

Owner

Okay, gotcha. Never mind then, carry on. :-)

@jails

jails Jan 17, 2013

Contributor

Pure hypothesis, since we need to override all methods in the Mock to make them filterable, can't we add to Mock it's own Object + StaticObject logic ?
i.e.

  • Mock will simply be a kind of "proxy" to the mocked class/instance.
  • an simple if (isset($this)) allow to differenciate Mock::applyFilter from $mock->applyFilter call.
  • According the ReflectionClass we know if a method is static or not so we can build the correct filter.

This way the Mocker can Mock _any kind of class_ with a small "footprints" (i.e adding applyFilter, _filter and $_methodFilters to the attributes/methods). And this way Debugger doesn't need to extends anything to be mocked.

Does that make sense ?

@blainesch

blainesch Jan 18, 2013

Member

That's an interesting idea. It sounds like it would work, I'll try and tackle that for my next pr. It would be nice to be able to 'mock' any class.

@nateabele If there are no other side effects of not extending Object or StaticObject, to increase performance would a pr be accepted that looked through and manually pulled out unnecessary extensions? Or do you think it's too few to be worth the time?

@jails

jails Jan 18, 2013

Contributor

As an afterthought, looks like mixing Object and StaticObject will break the strict standards (i.e. Non-static method should not be called statically). Moreover Models override the default StaticObject filter system so here the Mock must extends Model to make it work.

However I'm pretty sure we can find an alternative by dynamically extending the Mock from the mocked class if the mocked extends StaticObject or Object. And for other kind of class, the Mock will extends StaticObject or Object depending on "something" (something can be replaced here by a big question mark). Or maybe using two keywords Mock|StaticMock which indicate the type of the class ? Need to think about it.

@nateabele

nateabele Jan 18, 2013

Owner

@blainesch The thing with Debugger is that there's really no reason anyone would ever extend it. The thing with going through every class in the codebase is that there are very few other classes for which that is definitively, absolutely true. So yeah, not worth it, IMO.

Member

blainesch commented Jan 18, 2013

Any other concerns with this pull request, or can it be merged in?

Owner

nateabele commented Jan 18, 2013

Yeah, sorry, no other concerns.

@nateabele nateabele added a commit that referenced this pull request Jan 18, 2013

@nateabele nateabele Merge pull request #788 from BlaineSch/feature/mockFeatures
Feature/mock features
ddf711f

@nateabele nateabele merged commit ddf711f into UnionOfRAD:dev Jan 18, 2013

1 check passed

default The Travis build passed
Details

@blainesch blainesch deleted the unknown repository branch Jan 18, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment