Add logic for remaining custom signal operators #2633
Conversation
| @@ -246,3 +271,23 @@ function compareStrings( | |||
| const actual = String(actualValue); | |||
| return targetValues.some((target) => predicateFn(target, actual)); | |||
| } | |||
|
|
|||
| // Compares numeric version strings against each other. | |||
| function compareNumericVersions(version1String: string|number, version2String: string): number { | |||
There was a problem hiding this comment.
Given this is used for comparing semantic version strings, and we have other logic for comparing numbers, naming this compareSemanticVersions would be more clear.
There was a problem hiding this comment.
Done. This was a holdover from how things are named elsewhere :)
| // TODO: add comparison logic for additional operators here. | ||
|
|
||
| case CustomSignalOperator.NUMERIC_LESS_THAN: | ||
| return Number(actualCustomSignalValue) < Number(targetCustomSignalValues[0]); |
There was a problem hiding this comment.
Given we accept an array of target values, what's the plan for comparing the other values?
There was a problem hiding this comment.
For numeric and version operators, only one value is allowed. This is true for the client implementation as well. Added some comments here to document that.
| customSignal: { | ||
| customSignalOperator: CustomSignalOperator.NUMERIC_EQUAL, | ||
| customSignalKey: 'user_prop', | ||
| targetCustomSignalValues: ['5.11.9-beta'] |
There was a problem hiding this comment.
Thinking: this could be any non-numeric string.
| }); | ||
|
|
||
| it('should evaluate to false', () => { | ||
| const condition = createNamedCondition('is_enabled', { |
There was a problem hiding this comment.
Here and below, we should also test non-numeric string handling, as we do above for NUMERIC_EQUAL.
| customSignal: { | ||
| customSignalOperator: CustomSignalOperator.SEMANTIC_VERSION_EQUAL, | ||
| customSignalKey: 'user_prop', | ||
| targetCustomSignalValues: [' 5.12.3 '] |
There was a problem hiding this comment.
Since this whitespace-ignoring behavior applies to all semver comparisons, it might be more efficient to test it explicitly.
| customSignal: { | ||
| customSignalOperator: CustomSignalOperator.SEMANTIC_VERSION_LESS_EQUAL, | ||
| customSignalKey: 'user_prop', | ||
| targetCustomSignalValues: ['5.13'] |
There was a problem hiding this comment.
Seems we're missing the case of comparing a single digit actual version with a multi-digit target version. For example, 1 <= 1.2.3. Since we wrote the semver comparison logic and it has a few conditional branches, I think it'd be worth testing that logic directly. Then these condition-level tests could just cover an example case.
There was a problem hiding this comment.
Updated a bunch of these tests. We've got pretty extensive coverage now
| @@ -960,10 +960,17 @@ describe('ConditionEvaluator', () => { | |||
| targetCustomSignalValues: ['foo', 'bar'] | |||
| } | |||
| }); | |||
| const condition2 = createNamedCondition('is_active', { | |||
There was a problem hiding this comment.
Nit: naming the different test cases something more self-descriptive than "is_enabled" and "is_active" would be helpful.
There was a problem hiding this comment.
sure sure. was just following convention in this file but I agree
* initial skeleton for custom signal evaluation logic * adjust some formatting * remove extra curly brace * run lint * Run apidocs * Split EvaluationContext into UserProvidedSignals and PredefinedSignals * rerun apidocs * add logic for remaining custom signal operators * more test cases * test cases for numeric operators * update tests to be way more robust and implement handling for a few edge cases * update some comments --------- Co-authored-by: Kevin Elko <kjelko@google.com>
* Initial set up and evaluation logic for server side custom signals (#2628) initial skeleton for custom signal evaluation logic --------- Co-authored-by: Kevin Elko <kjelko@google.com> * Add logic for remaining custom signal operators (#2633) * initial skeleton for custom signal evaluation logic * adjust some formatting * remove extra curly brace * run lint * Run apidocs * Split EvaluationContext into UserProvidedSignals and PredefinedSignals * rerun apidocs * add logic for remaining custom signal operators * more test cases * test cases for numeric operators * update tests to be way more robust and implement handling for a few edge cases * update some comments --------- Co-authored-by: Kevin Elko <kjelko@google.com> * Ssrc targeting numeric version fixes (#2656) * refactor numeric version parsing logic and add more test cases * run lint * test cases for max num segments * run lint on tests --------- Co-authored-by: Kevin Elko <kjelko@google.com> * fix test ordering * Export signal types and rerun apidocs * update docstrings * uber minor comment fix --------- Co-authored-by: Kevin Elko <kjelko@google.com>
No description provided.