From dd59036a3e34ea35eb3fd651793f85777646f407 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 31 Oct 2022 14:50:15 +0100 Subject: [PATCH] fix: Include impression data status when not enabled (#122) ## What This PR fixes a bug in how impression data is reported when `impressionDataAll` is set to true: When a feature has `impressionData: false` but `impressionDataAll` is set to true, it will now report `impressionData: false` instead of `impressionData: undefined`. ## Why This is a bug because we should report the state of impression data for a feature when we can. In the original feature PR discussion, it was agreed to make `impressionData` a `boolean | undefined` where it would be reported as a boolean if we knew the state and `undefined` otherwise. It seems to have been an oversight in the tests. ## Clarifications ### What's with the try/catch blocks in the tests? It's explained more thoroughly in the [callback section of Jest's testing asynchronous code](https://jestjs.io/docs/asynchronous#callbacks), but in short: If you don't have the try/catch and one of the expectations fail, then the done callback is never called (because the function short circuits). While this does mean that the test fails (this is good), it fails with a message saying that it exceeded the set timeout and not because it failed an expectation (this is bad). To instead have the correct error message reported, you must catch the error and pass it to the done callback. This will make the test fail and report the correct error message. Now regarding using `finally`, I did consider that but ended up not doing it for the following reason: Calling the done callback requires the error for it to fail. This can't be done in a finally block because you don't have access to the error. In theory, you could probably declare an error variable that you set to undefined and then assign in the catch block, but that doesn't feel any cleaner to me. ## Commits * fix: tests expect false impression data status to be included * fix: fix tests to use try/catch as described in the Jest docs Refer to: https://jestjs.io/docs/asynchronous#callbacks * fix: update how the impressionData property gets set Set it to what's defined on the toggle if anything is. Otherwise, use undefined. * refactor: rename tests to better describe what they check for I had trouble understanding what the different tests were meant to check for, so I've renamed them into something (hopefully) more explanatory. --- src/index.test.ts | 42 ++++++++++++++++++++++++++---------------- src/index.ts | 4 ++-- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/src/index.test.ts b/src/index.test.ts index 6920520..a14e20a 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -1023,7 +1023,7 @@ test('Should pass under custom header clientKey', async () => { jest.advanceTimersByTime(999); }); -test('Should call isEnabled event when impressionData is true', (done) => { +test('Should emit impression events on isEnabled calls when impressionData is true', (done) => { const bootstrap = [ { name: 'impression', @@ -1084,7 +1084,7 @@ test('Should pass custom headers', async () => { }); }); -test('Should call getVariant event when impressionData is true', (done) => { +test('Should emit impression events on getVariant calls when impressionData is true', (done) => { const bootstrap = [ { name: 'impression-variant', @@ -1120,7 +1120,7 @@ test('Should call getVariant event when impressionData is true', (done) => { }); }); -test('Should not call isEnabled event when impressionData is false', (done) => { +test('Should not emit impression events on isEnabled calls when impressionData is false', (done) => { const bootstrap = [ { name: 'impression', @@ -1155,7 +1155,7 @@ test('Should not call isEnabled event when impressionData is false', (done) => { }); }); -test('Should call isEnabled event when impressionData is false and impressionDataAll is true', (done) => { +test('Should emit impression events on isEnabled calls when impressionData is false and impressionDataAll is true', (done) => { const bootstrap = [ { name: 'impression', @@ -1184,15 +1184,20 @@ test('Should call isEnabled event when impressionData is false and impressionDat }); client.on(EVENTS.IMPRESSION, (event: any) => { - expect(event.featureName).toBe('impression'); - expect(event.eventType).toBe('isEnabled'); - expect(event.impressionData).toBe(undefined); - client.stop(); - done(); + try { + expect(event.featureName).toBe('impression'); + expect(event.eventType).toBe('isEnabled'); + expect(event.impressionData).toBe(false); + client.stop(); + done(); + } catch (e) { + client.stop(); + done(e); + } }); }); -test('Should call isEnabled event when toggle is unknown and impressionDataAll is true', (done) => { +test('Should emit impression events on isEnabled calls when toggle is unknown and impressionDataAll is true', (done) => { const bootstrap = [ { name: 'impression', @@ -1230,7 +1235,7 @@ test('Should call isEnabled event when toggle is unknown and impressionDataAll i }); }); -test('Should call getVariant event when impressionData is false and impressionDataAll is true', (done) => { +test('Should emit impression events on getVariant calls when impressionData is false and impressionDataAll is true', (done) => { const bootstrap = [ { name: 'impression-variant', @@ -1259,11 +1264,16 @@ test('Should call getVariant event when impressionData is false and impressionDa }); client.on(EVENTS.IMPRESSION, (event: any) => { - expect(event.featureName).toBe('impression-variant'); - expect(event.eventType).toBe('getVariant'); - expect(event.impressionData).toBe(undefined); - client.stop(); - done(); + try { + expect(event.featureName).toBe('impression-variant'); + expect(event.eventType).toBe('getVariant'); + expect(event.impressionData).toBe(false); + client.stop(); + done(); + } catch (e) { + client.stop(); + done(e); + } }); }); diff --git a/src/index.ts b/src/index.ts index a12d406..d53f1e1 100644 --- a/src/index.ts +++ b/src/index.ts @@ -194,7 +194,7 @@ export class UnleashClient extends TinyEmitter { enabled, toggleName, IMPRESSION_EVENTS.IS_ENABLED, - toggle?.impressionData || undefined, + toggle?.impressionData ?? undefined, ); this.emit(EVENTS.IMPRESSION, event); } @@ -214,7 +214,7 @@ export class UnleashClient extends TinyEmitter { enabled, toggleName, IMPRESSION_EVENTS.GET_VARIANT, - toggle?.impressionData || undefined, + toggle?.impressionData ?? undefined, variant.name, ); this.emit(EVENTS.IMPRESSION, event);