Switch to longs for hash values for percent condition evaluation #2507
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
to prevent some flaky tests.
Co-authored-by: jen_h <harveyjen@google.com>
|
Thanks @amanda-xia ! For the CI error, could you try removing the package-lock file and node_modules locally and running |
|
Hey @lahirumaramba! I gave your suggestion a try, but the error is still persisting. Additionally, there is actually one unit test that is failing when I run |
|
With @erikeldridge's guidance, I locally switched to using Node v14, and then repeated @lahirumaramba's steps, and this resolved the CI check :) |
erikeldridge
requested changes
Mar 27, 2024
erikeldridge
approved these changes
Apr 3, 2024
lahirumaramba
approved these changes
Apr 4, 2024
trekforever
added a commit
that referenced
this pull request
Apr 4, 2024
* Get And condition passing * Support and.or condition * Support true condition * Support false condition, and fix tests * Use or.and, not the other way around * Integrate conditional values into evaluate method * Test handling for multiple conditions * Clean up logs * Extract condition evaluation to class for testing * Namespace condition names * Iterate over ordered condition list * Test condition ordering * Differentiate named conditions * Document condition types * Generalize condition eval test and fix styling * Replace log statement with todo * Implement evaluate percent condition for RC server-side * Apply lint fixes * Add context param to evaluate method * Add tests for percent condition eval * Update evaluator tests to use context * Increase threshold to +/- 500 for percent condition eval tests to prevent some flaky tests. * Clean up percentCondition tests a bit and add note on the tolerance used * Apply suggestions from code review Co-authored-by: jen_h <harveyjen@google.com> * Update copyright date and remove stray log statement * Mock farmhash in tests * Add Math.abs for farmhash - to be consistent with the internal implementation * Regenerate package-lock to fix Node 14 CI error re busboy * Fix lint errors * Rename "id" to "randomizationId" per discussion * Extract API * Only return false in cases of uknown template evaluation * Remove product prefix from type names * Remove product prefix from exported types * Remove unused "expression" field from server condition * Extract API * Remove prefix from impl classes, for consistency * Remove prefix from new internal classes * Remove "server" prefix * Remove prefix from NamedCondition * Rename "or" and "and" fields to match API * Rename "operator" field to "percentOperator" to match API * Extract API after "and" and "or" rename * use longjs library for hash * re-run npm install * re-attempt * use node 14, re-attempt * remove file * Add comment, switch from lte to lt --------- Co-authored-by: Erik Eldridge <erikeldridge@google.com> Co-authored-by: Xin Wei <xinwei@google.com> Co-authored-by: jen_h <harveyjen@google.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Discussion
We discovered that for a given seed and ID, the hash outputted for percent condition evaluation is inconsistent with the hash outputted in the targeting service that handles percent condition evaluation for client side templates. This PR fixes the inconsistency by importing a Longjs library so we can represent hashes as a long, which is consistent with what the targeting service does.
Longjs is a new dependency and is under the Apache 2.0 license.
Testing
Ran npm test and all tests pass.
Manually tested using a local server.
API Changes
None