Sameeran/ffl 420 holdouts #66
Merged
Conversation
This file contains hidden or 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
- Add entityId field to Assignment class and constructor - Add entityId field to FlagEvaluation struct and all factory methods - Add entityId field to UFC_Flag struct - Update EppoClient to pass entityId from FlagEvaluation to Assignment - Update RuleEvaluator to pass entityId from flag config to FlagEvaluation - Update all test files to include entityId parameter in test configurations - Fix test assertions to check for empty extraLogging instead of nil - Add holdout logging tests with entityId scenarios This change enables tracking of entity IDs throughout the flag evaluation pipeline, from flag configuration through to assignment logging.
- Remove testAssignmentWithAllFields() - combination test covered by individual tests - Remove testAssignmentWithNilHoldoutFields() - basic initialization already tested - Add missing extraLogging.isEmpty assertion to testAssignmentWithEntityId() Simplifies test file while maintaining adequate coverage of entityId and extraLogging functionality through focused individual tests.
Remove trailing comma after entityId parameter in Assignment.init() to fix Swift compilation error. Trailing commas are not allowed after the last parameter in function signatures.
- Change testHoldoutLoggingWithEntityIdAndHoldoutInfo to use boolean-flag instead of flag-mid-holdout - Change testHoldoutLoggingWithoutEntityIdNorHoldoutInfo to use boolean-flag instead of flag-mid-holdout - Update both tests to use getBooleanAssignment() instead of getStringAssignment() - Remove entityId from second test to verify behavior without entityId - Update test assertions to match boolean flag variations (true/false instead of control/treatment) - Verify getBooleanAssignment() works without try-catch wrapper
- Add testHoldoutLoggingWithDoLogFalse() to verify that doLog: false prevents assignment logging
- Remove redundant comments ("Initialize EppoClient with test configuration", "Perform an assignment")
- Remove entityId from testHoldoutLoggingWithoutEntityIdNorHoldoutInfo to properly test without entityId scenario
- Remove extraLogging from doLog: false test since it's not needed for the test
- Change variationKey to "true" in doLog: false test for better test coverage
leoromanovsky
approved these changes
Jul 21, 2025
Update SDK version from 4.3.0 to 4.4.0 in Constants.swift
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
🎟️ Fixes issue
📜 Design Doc: link if applicable
Motivation and Context
Description
holdoutKeyandholdoutVariationinextraLoggingdoLog: falseprevents assignment loggingeppoClient.getBooleanAssignmentdoes not need to be wrapped with a do-try-catchdoLogis respected and prevents logging when set to falseentityIdsupport for parity with js/node SDKs (SetentityIdon flag evaluation js-sdk-common#234)How has this been documented?
Eppo-exp/eppo-docs#691
Or alternatively we make the SDKs consistent with the holdout fields at the top-level of the event object, but the challenge there is that the nested
extraLoggingmay already be in use in the SDKs where it was includedHow has this been tested?
Added tests in
AssignmentLoggerTests.swift:testHoldoutLoggingWithEntityIdAndHoldoutInfo: Verifies holdout logging with entityId and extraLoggingtestHoldoutLoggingWithoutEntityIdNorHoldoutInfo: Verifies logging without entityId nor extraLoggingtestHoldoutLoggingWithDoLogFalse: Verifies that doLog: false prevents assignment logging