Skip to content

fix(hooks): Always recompute decision after resolution of ready promise #91

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Dec 11, 2020
Merged

fix(hooks): Always recompute decision after resolution of ready promise #91

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
478408e
fix(hooks): Always recompute decision after resolution of ready promise
Dec 7, 2020
b90202b
Remove comment & comma
Dec 8, 2020
8527c27
Remove setState functions from useEffect dependency arrays
Dec 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 8 additions & 16 deletions src/Experiment.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ describe('<OptimizelyExperiment>', () => {

// Simulate client becoming ready: onReady resolving, firing config update notification
resolver.resolve({ success: true });
(optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();

await optimizelyMock.onReady();

Expand All @@ -101,7 +100,6 @@ describe('<OptimizelyExperiment>', () => {

// Simulate client becoming ready: onReady resolving, firing config update notification
resolver.resolve({ success: true });
(optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();

await optimizelyMock.onReady();

Expand All @@ -121,9 +119,8 @@ describe('<OptimizelyExperiment>', () => {
// while it's waiting for onReady()
expect(component.text()).toBe('');

// Simulate client becoming ready: onReady resolving, firing config update notification
// Simulate client becoming ready
resolver.resolve({ success: true });
(optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();

await optimizelyMock.onReady();

Expand All @@ -144,9 +141,8 @@ describe('<OptimizelyExperiment>', () => {
// while it's waiting for onReady()
expect(component.text()).toBe('');

// Simulate client becoming ready: onReady resolving, firing config update notification
// Simulate client becoming ready
resolver.resolve({ success: true });
(optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();

await optimizelyMock.onReady();

Expand Down Expand Up @@ -212,9 +208,8 @@ describe('<OptimizelyExperiment>', () => {
// while it's waiting for onReady()
expect(component.text()).toBe('');

// Simulate client becoming ready: onReady resolving, firing config update notification
// Simulate client becoming ready
resolver.resolve({ success: true });
(optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();

await optimizelyMock.onReady();

Expand All @@ -237,9 +232,8 @@ describe('<OptimizelyExperiment>', () => {
// while it's waiting for onReady()
expect(component.text()).toBe('');

// Simulate client becoming ready: onReady resolving, firing config update notification
// Simulate client becoming ready
resolver.resolve({ success: true });
(optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();

await optimizelyMock.onReady();

Expand Down Expand Up @@ -285,9 +279,8 @@ describe('<OptimizelyExperiment>', () => {
// while it's waiting for onReady()
expect(component.text()).toBe('');

// Simulate client becoming ready: onReady resolving, firing config update notification
// Simulate client becoming ready
resolver.resolve({ success: true });
(optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();

await optimizelyMock.onReady();

Expand All @@ -298,11 +291,11 @@ describe('<OptimizelyExperiment>', () => {
expect(component.text()).toBe('variationResult');

// capture the OPTIMIZELY_CONFIG_UPDATE function
const updateFn = (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1];
// change the return value of activate
const mockActivate = optimizelyMock.activate as jest.Mock;
mockActivate.mockImplementationOnce(() => 'newVariation');

const updateFn = (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1];
updateFn();
expect(optimizelyMock.activate).toBeCalledTimes(2);

Expand All @@ -324,10 +317,9 @@ describe('<OptimizelyExperiment>', () => {
expect(optimizelyMock.onReady).toHaveBeenCalledWith({ timeout: 100 });
// while it's waiting for onReady()
expect(component.text()).toBe('');
resolver.resolve({ success: true });

// Simulate config update update notification firing after datafile becomes available.
(optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
// Simulate client becoming ready
resolver.resolve({ success: true });

await optimizelyMock.onReady();

Expand Down
10 changes: 1 addition & 9 deletions src/Feature.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ describe('<OptimizelyFeature>', () => {

// Simulate client becoming ready
resolver.resolve({ success: true });
(optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();

await optimizelyMock.onReady();

Expand All @@ -106,7 +105,6 @@ describe('<OptimizelyFeature>', () => {

// Simulate client becoming ready
resolver.resolve({ success: true });
(optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();

await optimizelyMock.onReady();

Expand All @@ -133,7 +131,6 @@ describe('<OptimizelyFeature>', () => {

// Simulate client becoming ready
resolver.resolve({ success: true });
(optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();

await optimizelyMock.onReady();

Expand All @@ -160,7 +157,6 @@ describe('<OptimizelyFeature>', () => {

// Simulate client becoming ready
resolver.resolve({ success: true });
(optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();

await optimizelyMock.onReady();

Expand All @@ -185,7 +181,6 @@ describe('<OptimizelyFeature>', () => {

// Simulate client becoming ready
resolver.resolve({ success: true });
(optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();

await optimizelyMock.onReady();

Expand All @@ -212,9 +207,7 @@ describe('<OptimizelyFeature>', () => {
expect(component.text()).toBe('');

// Simulate client becoming ready
const updateFn = (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1];
resolver.resolve({ success: true });
updateFn();

await optimizelyMock.onReady();

Expand All @@ -232,6 +225,7 @@ describe('<OptimizelyFeature>', () => {
foo: 'baz',
}));

const updateFn = (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1];
updateFn();

component.update();
Expand All @@ -257,7 +251,6 @@ describe('<OptimizelyFeature>', () => {
expect(component.text()).toBe('');

// Simulate client becoming ready
(optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
resolver.resolve({ success: true });

await optimizelyMock.onReady();
Expand Down Expand Up @@ -304,7 +297,6 @@ describe('<OptimizelyFeature>', () => {
resolver.resolve({ success: false, reason: 'fail', dataReadyPromise: Promise.resolve() });

// Simulate config update notification firing after datafile fetched
(optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
await optimizelyMock.onReady().then(res => res.dataReadyPromise);

component.update();
Expand Down
58 changes: 42 additions & 16 deletions src/hooks.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,20 +147,24 @@ describe('hooks', () => {
});

it('should respect the timeout option passed', async () => {
activateMock.mockReturnValue('12345');
activateMock.mockReturnValue(null);
readySuccess = false;

const component = Enzyme.mount(
<OptimizelyProvider optimizely={optimizelyMock}>
<MyExperimentComponent options={{ timeout: mockDelay }} />
</OptimizelyProvider>
);
expect(component.text()).toBe('null|false|false'); // initial render

await optimizelyMock.onReady();
component.update();
expect(component.text()).toBe('null|false|true'); // when didTimeout
readySuccess = true;
// Simulate CONFIG_UPDATE notification, causing decision state to recompute
notificationListenerCallbacks[0]();

// Simulate datafile fetch completing after timeout has already passed
// Activate now returns a variation
activateMock.mockReturnValue('12345');
// Wait for completion of dataReadyPromise
await optimizelyMock.onReady().then(res => res.dataReadyPromise);
component.update();
expect(component.text()).toBe('12345|true|true'); // when clientReady
Expand Down Expand Up @@ -243,7 +247,7 @@ describe('hooks', () => {
expect(mockLog).toHaveBeenCalledWith('12345');
});

it('should re-render after the client becomes ready and triggers a config update notification', async () => {
it('should re-render after the client becomes ready', async () => {
readySuccess = false;
let resolveReadyPromise: (result: { success: boolean; dataReadyPromise: Promise<any> }) => void;
const readyPromise: Promise<any> = new Promise(res => {
Expand All @@ -266,10 +270,14 @@ describe('hooks', () => {
expect(mockLog).toHaveBeenCalledWith(null);

mockLog.mockReset();

// Simulate datafile fetch completing after timeout has already passed
// Activate now returns a variation
activateMock.mockReturnValue('12345');
// Simulate CONFIG_UPDATE notification, causing decision state to recompute
notificationListenerCallbacks[0]();
resolveReadyPromise!({ success: true, dataReadyPromise: Promise.resolve() });
// Wait for completion of dataReadyPromise
const dataReadyPromise = Promise.resolve();
resolveReadyPromise!({ success: true, dataReadyPromise });
await dataReadyPromise;
component.update();

expect(mockLog).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -380,22 +388,36 @@ describe('hooks', () => {
});

it('should respect the timeout option passed', async () => {
isFeatureEnabledMock.mockReturnValue(true);
isFeatureEnabledMock.mockReturnValue(false);
featureVariables = {};
readySuccess = false;

const component = Enzyme.mount(
<OptimizelyProvider optimizely={optimizelyMock}>
<MyFeatureComponent options={{ timeout: mockDelay }} />
</OptimizelyProvider>
);
expect(component.text()).toBe('false|{}|false|false'); // initial render

await optimizelyMock.onReady();
component.update();
expect(component.text()).toBe('false|{}|false|true'); // when didTimeout
readySuccess = true;
// Simulate CONFIG_UPDATE notification, causing decision state to recompute
notificationListenerCallbacks[0]();

// Simulate datafile fetch completing after timeout has already passed
// isFeatureEnabled now returns true, getFeatureVariables returns variable values
isFeatureEnabledMock.mockReturnValue(true);
featureVariables = mockFeatureVariables;
// Wait for completion of dataReadyPromise
await optimizelyMock.onReady().then(res => res.dataReadyPromise);
component.update();

// Simulate datafile fetch completing after timeout has already passed
// Activate now returns a variation
activateMock.mockReturnValue('12345');
// Wait for completion of dataReadyPromise
await optimizelyMock.onReady().then(res => res.dataReadyPromise);
component.update();

expect(component.text()).toBe('true|{"foo":"bar"}|true|true'); // when clientReady
});

Expand Down Expand Up @@ -480,7 +502,7 @@ describe('hooks', () => {
expect(mockLog).toHaveBeenCalledWith(false);
});

it('should re-render after the client becomes ready and triggers a config update notification', async () => {
it('should re-render after the client becomes ready', async () => {
readySuccess = false;
let resolveReadyPromise: (result: { success: boolean; dataReadyPromise: Promise<any> }) => void;
const readyPromise: Promise<any> = new Promise(res => {
Expand All @@ -503,10 +525,14 @@ describe('hooks', () => {
expect(mockLog).toHaveBeenCalledWith(false);

mockLog.mockReset();

// Simulate datafile fetch completing after timeout has already passed
// isFeatureEnabled now returns true
isFeatureEnabledMock.mockReturnValue(true);
// Simulate CONFIG_UPDATE notification, causing decision state to recompute
notificationListenerCallbacks[0]();
resolveReadyPromise!({ success: true, dataReadyPromise: Promise.resolve() });
// Wait for completion of dataReadyPromise
const dataReadyPromise = Promise.resolve();
resolveReadyPromise!({ success: true, dataReadyPromise });
await dataReadyPromise;
component.update();

expect(mockLog).toHaveBeenCalledTimes(1);
Expand Down
Loading