Skip to content

Conversation

@mnoman09
Copy link
Contributor

@mnoman09 mnoman09 commented Feb 1, 2021

Summary

  • Added unit tests to verify when source type is rollout and SendFlagDecisions is false then event should not dispatch and decisionEventDispatched should be false.
  • Added unit tests to verify when source type is rollout and SendFlagDecisions is true then event should dispatch and decisionEventDispatched should be true.

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look good.
Suggested to add the same tests for featureTest as well.

var featureKey = "boolean_single_variable_feature";
var featureFlag = Config.GetFeatureFlagFromKey(featureKey);
var variables = Optimizely.GetAllFeatureVariables(featureKey, TestUserId);
var reasons = new OptimizelyDecideOption[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line

var featureKey = "boolean_single_variable_feature";
var featureFlag = Config.GetFeatureFlagFromKey(featureKey);
var variables = Optimizely.GetAllFeatureVariables(featureKey, TestUserId);
var reasons = new OptimizelyDecideOption[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

{ "ruleKey", ruleKey },
{ "reasons", reasons },
{ "decisionEventDispatched", false }
}))), Times.Once);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you validation that event is actually not sent out?

{ "reasons", reasons },
{ "decisionEventDispatched", true }
}))), Times.Once);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you validation that event is actually sent out and event contents as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can we add the same couple of tests for "featureTest" (not rollout) as well?

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jaeopt jaeopt merged commit 1f14ca5 into master Feb 12, 2021
@jaeopt jaeopt deleted the mnoman/DispatchedEventTests branch February 12, 2021 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants