-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Added GetAllFeatureVariables API and NotificationListener support #217
Conversation
# Conflicts: # OptimizelySDK.Tests/OptimizelyJsonTest.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove irrelevant test cases. Also Use proper Optimizely constructor
, don't use reflection until it's really necessary.
@@ -1789,6 +1789,9 @@ public void TestUnsupportedVariableType() | |||
var featureVariableStringRandomType = Optimizely.GetFeatureVariableString("", "any_key", TestUserId); | |||
Assert.IsNull(featureVariableStringRandomType); | |||
|
|||
var featureVariableStringJsonType = Optimizely.GetFeatureVariableString("unsupported_variabletype", "string_json_key", TestUserId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the point to add test here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was an old test which was testing that getFeatureVariableString will return string value even when given json as subtype but this is no longer the case here. So now if the sub type is JSON and we will call getFeatureVariableString to get that variable, it will return null because type will not be matched.
|
||
var optly = Helper.CreatePrivateOptimizely(); | ||
|
||
optly.SetFieldOrProperty("ProjectConfigManager", ConfigManager); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why we are using reflection
to instantiate optimizely
instance. please explain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is to set decisionservice object to decisionserviceMock object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it.
var result = (OptimizelyJson)optly.Invoke("GetAllFeatureVariables", null, TestUserId, userAttributes); | ||
Assert.Null(result); | ||
|
||
LoggerMock.Verify(log => log.Log(LogLevel.WARN, "The featureKey parameter must be nonnull."), Times.Once); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nonnull
or not-null
@pawels-optimizely need suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cannot be null
|
||
var featureFlag = Config.GetFeatureFlagFromKey(featureKey); | ||
|
||
var decision = new FeatureDecision(experiment, null, FeatureDecision.DECISION_SOURCE_ROLLOUT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why variation is null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To verify info log that ""User "" + TestUserId + "" was not bucketed into any variation for feature flag "" + featureKey + "". The default values are being returned.""
|
||
DecisionServiceMock.Setup(ds => ds.GetVariationForFeature(featureFlag, TestUserId, Config, userAttributes)).Returns(decision); | ||
|
||
var optly = Helper.CreatePrivateOptimizely(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revise it.
} | ||
|
||
[Test] | ||
public void TestGetAllFeatureVariablesValid() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it mean valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -1788,7 +1788,7 @@ public void TestUnsupportedVariableType() | |||
{ | |||
var featureVariableStringRandomType = Optimizely.GetFeatureVariableString("", "any_key", TestUserId); | |||
Assert.IsNull(featureVariableStringRandomType); | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove spacing.
|
||
var optly = Helper.CreatePrivateOptimizely(); | ||
|
||
optly.SetFieldOrProperty("ProjectConfigManager", ConfigManager); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Summary
Test Plan