Add input validation to feature APIs #121
Conversation
src/Optimizely/Optimizely.php
Outdated
| } | ||
|
|
||
| $enabledFeatureKeys = []; |
There was a problem hiding this comment.
Can we leave this at line 561 (on left) and return this empty array on lines 569 and 574 on the left?
| $optimizelyMock->expects($this->once()) | ||
| ->method('validateInputs') | ||
| ->with($inputArray) | ||
| ->willReturn(false); |
There was a problem hiding this comment.
This testcase is a little confusing, you are passing in what seems to be valid inputs test_feature and test_user but mocking the validateInputs methods to return false.
Let's instead assign $userId to null and then ensure that validateInputs is called with it and then the assertFalse you already have will validate the isFeatureEnabled returns correctly. Also, ensure that the logger logs the appropriate message for invalid inputs
There was a problem hiding this comment.
Right, I have made userId null for clarity.
TestDoubles in PHPUnit provide a dummy implementation that simply returns null without calling the original method. See it here: https://phpunit.de/manual/6.5/en/test-doubles.html
That's why I add willReturn part for clarity to sort of show that when validateInputs returns false, the method is asserted false. The actual asserting part is the expectation of validateInputs being called with same params.
The testcase doesn't guarantee that the method ended right after valdiateInputs returned false or later. A better way could be asserting that code after validateInputs has returned isn't executed.
As for logging, I am already comprehensively testing logging in unit tests for validateInputs method. This would be repetitive here.
There was a problem hiding this comment.
Yeah that makes sense. Will approve now
| $optimizelyMock->expects($this->once()) | ||
| ->method('validateInputs') | ||
| ->with($inputArray) | ||
| ->willReturn(false); |
No description provided.