fix: non-bandit flags return string assignment #238
Conversation
greghuels
force-pushed
the
greg/FF-4105/no-bandit-flag-2
branch
from
3881f2d to
fe23022
Compare
greghuels
changed the title
greghuels
changed the title
greghuels
force-pushed
the
greg/FF-4105/no-bandit-flag-2
branch
2 times, most recently
from
e3c041a to
43078e9
Compare
greghuels
force-pushed
the
greg/FF-4105/no-bandit-flag-2
branch
from
43078e9 to
264202c
Compare
| expect(precomputedConfiguration).toEqual({ action: null, variation: 'nike' }); | ||
| }); | ||
|
|
||
| it('should return the assigned variation if a flag is not a bandit', () => { |
|
|
||
| it('should return the assigned variation if a flag is not a bandit', () => { | ||
| const precomputedConfiguration = client.getBanditAction('not-a-bandit-flag', 'default'); | ||
| expect(precomputedConfiguration).toEqual({ action: null, variation: 'control' }); |
There was a problem hiding this comment.
also consider a situation where a flag IS a bandit flag, but they got assigned a non-bandit variation.
Also we should be using mocks to make sure the proper event loggers are called or not
There was a problem hiding this comment.
also consider a situation where a flag IS a bandit flag, but they got assigned a non-bandit variation.
For the precomputed response, wouldn't that be the same situation? The flag would be in flags but not in bandits
There was a problem hiding this comment.
Also we should be using mocks to make sure the proper event loggers are called or not
Updated! And I also added the happy path case of a bandit flag with a bandit variation
| const precomputedFlag = this.getPrecomputedFlag(flagKey); | ||
| if (!precomputedFlag) { | ||
| logger.warn(`${loggerPrefix} No assigned variation. Flag not found: ${flagKey}`); | ||
| return { variation: defaultValue, action: null }; |
There was a problem hiding this comment.
Returning this if the precomputed flag is not found, otherwise returning the precomputed flag assignment if the bandit could not be evaluated. Makes sense
greghuels
force-pushed
the
greg/FF-4105/no-bandit-flag-2
branch
from
a83d1e1 to
7dbd361
Compare
| } catch (error) { | ||
| logger.error(`${loggerPrefix} Error logging bandit action: ${error}`); | ||
| } | ||
| return { variation: assignedVariation, action: banditEvent.action }; |
There was a problem hiding this comment.
Returning the banditEvent.action is interesting here. Are you counting on it being undefined | null? If so please return that directly - or is it preforming randomization in this case?
There was a problem hiding this comment.
I wonder if we ax this line all together, just keeping that final return with optional chaining and null coalesce:
return { variation: assignedVariation, action: banditEvent?.action ?? null }There was a problem hiding this comment.
sorry banditEvaluation.action
Tests will pass after Eppo-exp/sdk-test-data#123 is merged
Fixes scenario where a bandit flag is assigned to a non-bandit variation. Instead of returning the programmatic default, we want to return the assigned variation when
getBanditActionis called.See also: Eppo-exp/sdk-test-data#123