-
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
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!
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.
Co-authored-by: teddddd <[email protected]>
…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
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.
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.
LGTM, just a few little cleanup comments. Thanks!
Co-authored-by: teddddd <[email protected]>
…ode into runtime-rules-engine

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