Skip to content

Conversation

yasirfolio3
Copy link
Contributor

Summary

  • fixes target-rollout error continuing other rules when a rule hits but bucketing fails.

Test Plan

  • FSC should pass.
  • Updated unit tests accordingly.

Issues

@coveralls
Copy link

coveralls commented May 12, 2020

Coverage Status

Coverage decreased (-0.05%) to 98.771% when pulling 1038c2c on yasir/OASIS-6334 into 2ecd899 on master.

@yasirfolio3 yasirfolio3 removed their assignment May 12, 2020
@msohailhussain msohailhussain requested a review from jaeopt May 12, 2020 16:59
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.

Suggested some test changes

[
"variables": [],
"id": "10389700000",
"key": kVariationKeyA,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use different key names (like "kRolloutVariationKeyA"). It's already used in experiment variations. Can be confused. Same for other rollout variations as well.

let variation = self.decisionService.getVariationForFeatureRollout(config: config,
featureFlag: featureFlag,
userId: kUserId,
attributes: kAttributesCountryMatch)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, we need different audience for 2nd rollout rule. CountryMatch is used for feature-experiment. So no guarantee if it's passed down to rollout.

let variation = self.decisionService.getVariationForFeatureRollout(config: config,
featureFlag: featureFlag,
userId: kUserId,
attributes: kAttributesAgeMatch)
Copy link
Contributor

Choose a reason for hiding this comment

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

With this setting, we cannot tell if it skips the rest of them or goes thru all rules. What about this setting:

Rule1: age < 30 (traffic allocation = 0)
Rule2: age < 40 (traffic allocation = 10000)
Rule3: everybody

When attribute age = 20, we should expect variation for Rule 3 if it skips. Otherwise, variation for Rule 2 will hit.

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 226741f into master May 13, 2020
@jaeopt jaeopt deleted the yasir/OASIS-6334 branch May 13, 2020 16:27
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.

3 participants