Feature toggleability #168
Conversation
1) updated datafile 2) updated testCases
Feature toggleability Added. Updated testcases.
|
Can one of the admins verify this patch? |
wangjoshuah
left a comment
wangjoshuah
left a comment
There was a problem hiding this comment.
looks mostly good. Make sure to describe the test cases.
@thomaszurkan-optimizely any feedback from you?
| } | ||
|
|
||
| @Test | ||
| public void isFeatureEnabledTrueWhenFeatureEnabledOfVariationIsTrue() throws Exception{ |
There was a problem hiding this comment.
describe the test case in in comments above
| } | ||
|
|
||
| @Test | ||
| public void isFeatureEnabledFalseWhenFeatureEnabledOfVariationIsFalse() throws Exception{ |
There was a problem hiding this comment.
add test description here.
There was a problem hiding this comment.
Can we add one more explicit test: isFeatureEnabledFalseWhenFeatureEnabledOfVariationIsNotPresentInJson ?
Feel free to not use that name.
|
build |
1 similar comment
|
build |
thomaszurkan-optimizely
left a comment
thomaszurkan-optimizely
left a comment
There was a problem hiding this comment.
After re-reviewing the design documents, it is not a bug for the datafile variation to not have feature_enable. If it is not present it is assumed to be false. I see that the Java SDK json parsers handle this but I don’t see a test case. Did I just miss it?
| @JsonProperty("variables") List<LiveVariableUsageInstance> liveVariableUsageInstances) { | ||
| this.id = id; | ||
| this.key = key; | ||
| this.featureEnabled = featureEnabled; |
There was a problem hiding this comment.
I see in the JSON parsers you make sure that the featureEnabled is not null. Can't you do that here as well? if (featureEnbaled == null) set it to false. Just for safety sake.
| } | ||
|
|
||
| @Test | ||
| public void isFeatureEnabledFalseWhenFeatureEnabledOfVariationIsFalse() throws Exception{ |
There was a problem hiding this comment.
Can we add one more explicit test: isFeatureEnabledFalseWhenFeatureEnabledOfVariationIsNotPresentInJson ?
Feel free to not use that name.
* Added unit test of if featureenabled is not set thn
|
build |
1 similar comment
|
build |
|
build |
wangjoshuah
left a comment
wangjoshuah
left a comment
There was a problem hiding this comment.
lgtm. I think @thomaszurkan-optimizely wanted one more test case added though.
| * feature enabled will return false by default | ||
| */ | ||
| @Test | ||
| public void isFeatureEnabledWithExperimentKeyForcedWithNoFeatureEnabledSet() throws Exception { |
There was a problem hiding this comment.
@thomaszurkan-optimizely here is the test case I am checking in which we are not setting featureEnabled variable in datafile and in config so its returning false by default
|
build |
Pull Request Test Coverage Report for Build 471
💛 - Coveralls |
1 similar comment
Pull Request Test Coverage Report for Build 471
💛 - Coveralls |
No description provided.