Add default callback functionality #42
Conversation
| public function testIfSetDefaultInvokesCallbackIfArgIsString() | ||
| { | ||
| $this->registrar = Mockery::mock(CallbackRegistrar::class)->makePartial(); | ||
| $this->registrar->loadCallbacks(['foo' => '\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo']); | ||
|
|
||
| $this->registrar->shouldReceive('callback')->with('foo')->once(); | ||
|
|
||
| $this->registrar->setDefault('foo'); | ||
| } |
There was a problem hiding this comment.
In my opinion this test is unnecessary. You should test the functionality, not the implementation.
There was a problem hiding this comment.
Omitting this test means that we'll have non-test covered code, also this test assures that the method does not interpret callables (e.g. FooClass::bar) as strings which would result in an exception.
There was a problem hiding this comment.
In my opinion you should test setDefault with getDefault. As I said test the functionality not the implementation.
There was a problem hiding this comment.
I have thought of that during the testing phase, but that would make the tests tightly coupled and would cause both to fail if the getDefault method is failing. In addition, in my own humble opinion, this is actually testing the functionality which is accepting both callables and strings and in return setting the default value.
There was a problem hiding this comment.
To be exact my problem is at line 126. With that you are testing the implementation. However, as far as I know you made setDefault to return something. in that case you can test the return of that function. Lastly, getters and setters are usually "tightly coupled" similarly to push and pop.
| public function setDefault($callback, ...$arguments) | ||
| { | ||
| if (is_callable($callback)) { | ||
| return $this->default = [$callback, $arguments]; |
There was a problem hiding this comment.
According to the doc block @return void but this clearly returns the default.
There was a problem hiding this comment.
Oh that is a typo, forgot to change the auto generated return type, i will also add the @throws exception
Resolves #11