Added tests for Httplug factory #66
Conversation
| @@ -95,11 +93,14 @@ protected function evaluateCondition($condition) | |||
| $evaluatedCondition = true; | |||
|
|
|||
| // Immediately stop execution if the condition is false | |||
| for ($i = 0; $i < count($condition) && false !== $evaluatedCondition; ++$i) { | |||
| $evaluatedCondition &= $this->evaluateCondition($condition[$i]); | |||
There was a problem hiding this comment.
I can not see any reson why we need &=
There was a problem hiding this comment.
Because $evaluatedCondition is checked before the next loop and breaks it if false.
There was a problem hiding this comment.
I would not remove this as it is a wonderful solution for sparing that extra if you added.
There was a problem hiding this comment.
I will only "look nice". You will still do the same number of comparisons. The current solution does not support assoc. arrays.
Anyhow. I updated the PR so it gets the best of both worlds... What do you think?
There was a problem hiding this comment.
Same number of comparisons, but less branches IMO. LGTM.
| { | ||
| public function testEvaluateCondition() | ||
| { | ||
| $method = new \ReflectionMethod(HttplugFactory::class, 'evaluateCondition'); |
There was a problem hiding this comment.
The correct way would be mocking the Puli discovery, returning a mocked Class Binding and using it for testing here.
There was a problem hiding this comment.
And not using Reflection? Would that allow me to isolate the tests for evaluateCondition?
There was a problem hiding this comment.
Yes. You can mock the hasParameter and getParameter methods of the class binding.
Generally you should always test the public API. It is part of the public API that if a class binding has a depends parameter, it gets evaluated "somehow".
So what you really want to test in this case is what separate depends parameters return a valid class binding, which of them end up in an exception.
There was a problem hiding this comment.
Generally you should always test the public API.
I know that is "generally" the idea. Your suggestion would require a lot of mocking and is very likely to break if the class changes. That's why I chose to use reflection to just test the protected function.
So what you really want to test in this case is what separate depends parameters return a valid class binding, which of them end up in an exception.
Haha. No 😄 I really wanted to only test the evaluateCondition and leave the rest of the class untested.
There was a problem hiding this comment.
Your suggestion would require a lot of mocking and is very likely to break if the class changes.
What class?
I think it requires an acceptable amount of mocks. Not to mention you can reuse mock builders. (Even better: as of PHPUnit 4.5 you can use prophecy known from phpspec. You could even use phpspec in httplugbundle as well along with phpunit, that would probably be even better)
There was a problem hiding this comment.
What class?
This, HttplugFactory
Okey,
I'll give it a try and we take it from there. =)
|
Ive updated the PR using Prophecy. |
Nyholm
force-pushed
the
plug_factory
branch
2 times, most recently
from
8503184 to
8fa2c99
Compare
| /** | ||
| * @var \Prophecy\Prophet | ||
| */ | ||
| private $prophet; |
There was a problem hiding this comment.
You don't need to manually instantiate a prophet object. Try $this->prophesize()
Added tests for Httplug factory
I did also rewrite the
evaluateConditionfunction to support associative arrays.This will address one issue in #64