-
Notifications
You must be signed in to change notification settings - Fork 163
Runtime rules engine ⚙️ #249
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
base: master
Are you sure you want to change the base?
Conversation
Still need caseinsensitive on rule
msiebert
left a comment
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.
tdumitrescu
left a comment
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.
Thanks for the thorough test cases. Mostly just some nitpicks on my end!
| }); | ||
|
|
||
| it("should return fallback when runtime rule is invalid", async () => { | ||
| const runtimeEvaluationRule = { "=oops=": [{ var: "plan" }, "Premium"] }; |
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.
Might be worth adding debug logging for this case? (at least when the rule is loaded over the network, probably too noisy to do it on every check)
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.
Good call out, looks like I've already covered that here
this.logger?.error(`Error evaluating runtime rule: ${error.message}`);
test/flags/local_flags.js
Outdated
| }); | ||
|
|
||
| const result = provider.getVariant(FLAG_KEY, FALLBACK, context); | ||
| expect(result.variant_value).not.toBe(FALLBACK_NAME); |
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.
Can we look for a specific value here rather than just "not the fallback"? Since that could pass the test for weird failure cases like if variant_value is unexpectedly 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.
Good thinking, just refactored to use a helper assertion that asserts both. Note that because there's a 50/50 split on the variant, I just check that the result is one of the two. The hashing logic is tested elsewhere and is an orthogonal concern to these test cases.
| it("should return fallback when runtime evaluation not satisfied", async () => { | ||
| const runtimeEval = { plan: "premium", region: "US" }; | ||
| const flag = createTestFlag({ runtimeEvaluation: runtimeEval }); | ||
| it("should return variant when runtime evaluation rule case-insensitively satisfied", async () => { |
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.
Logically this test case isn't any different than the one on L318 is 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.
They're actually different. The one on L318 is asserting case-insensitivity of rules, this one here is asserting case-insensitivity of runtime parameters. Both have to be lowercased. If only one is lowercased, then the corresponding test fails.
| await createFlagAndLoadItIntoSDK({ runtimeEvaluationRule }, provider); | ||
|
|
||
| const context = userContextWithRuntimeParameters({ | ||
| name: "d", |
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.
might be worth adding a test case where this value is a substring of one of the array values e.g. name: "all"
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.
Good idea, just added substring and superstring cases.
| mockFlagDefinitionsResponse([flag]); | ||
| await provider.startPollingForDefinitions(); | ||
| const context = userContextWithRuntimeParameters({ | ||
| plan: "premium", |
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.
Apologies if I missed it, but let's make sure to have a test case that specifies the behavior when the case of the key is mismatched e.g. {PLAN: "premium"}
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.
Good catch, this isn't under test, just added a test.
Co-authored-by: teddddd <ted@mixpanel.com>
…ode into runtime-rules-engine
joshua-koehler
left a comment
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.
Thanks for the thorough review, ready for a second look @tdumitrescu
| }); | ||
|
|
||
| it("should return fallback when runtime rule is invalid", async () => { | ||
| const runtimeEvaluationRule = { "=oops=": [{ var: "plan" }, "Premium"] }; |
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.
Good call out, looks like I've already covered that here
this.logger?.error(`Error evaluating runtime rule: ${error.message}`);
test/flags/local_flags.js
Outdated
| }); | ||
|
|
||
| const result = provider.getVariant(FLAG_KEY, FALLBACK, context); | ||
| expect(result.variant_value).not.toBe(FALLBACK_NAME); |
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.
Good thinking, just refactored to use a helper assertion that asserts both. Note that because there's a 50/50 split on the variant, I just check that the result is one of the two. The hashing logic is tested elsewhere and is an orthogonal concern to these test cases.
| it("should return fallback when runtime evaluation not satisfied", async () => { | ||
| const runtimeEval = { plan: "premium", region: "US" }; | ||
| const flag = createTestFlag({ runtimeEvaluation: runtimeEval }); | ||
| it("should return variant when runtime evaluation rule case-insensitively satisfied", async () => { |
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.
They're actually different. The one on L318 is asserting case-insensitivity of rules, this one here is asserting case-insensitivity of runtime parameters. Both have to be lowercased. If only one is lowercased, then the corresponding test fails.
| mockFlagDefinitionsResponse([flag]); | ||
| await provider.startPollingForDefinitions(); | ||
| const context = userContextWithRuntimeParameters({ | ||
| plan: "premium", |
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.
Good catch, this isn't under test, just added a test.
| await createFlagAndLoadItIntoSDK({ runtimeEvaluationRule }, provider); | ||
|
|
||
| const context = userContextWithRuntimeParameters({ | ||
| name: "d", |
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.
Good idea, just added substring and superstring cases.

Background
Local evaluation already supports evaluating the legacy runtime rule, which was simple exact key-value matching.
This pr
Support complex runtime rules in local-evaluation using jsonLogic.
QA'd local and remote eval ✅
https://github.com/mixpanel/sdk-integration-testing/commit/fe6d0968da0d4f30ce482abf46906d82bf7cc373
Example of what they look like in the UI