From 4644afc5be2dac90465e16a485ef5c34ff694da3 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Mon, 26 Sep 2022 17:19:52 -0500 Subject: [PATCH] add dispatch unit tests and minor refactoring (#602) * fix bug around invokeCallback Co-authored-by: Seth Silesky --- .changeset/brown-islands-judge.md | 5 + .../src/analytics/__tests__/dispatch.test.ts | 125 ++++++++++++++++++ packages/core/src/analytics/dispatch.ts | 11 +- .../core/src/callback/__tests__/index.test.ts | 16 +-- packages/core/src/callback/index.ts | 12 +- 5 files changed, 151 insertions(+), 18 deletions(-) create mode 100644 .changeset/brown-islands-judge.md create mode 100644 packages/core/src/analytics/__tests__/dispatch.test.ts diff --git a/.changeset/brown-islands-judge.md b/.changeset/brown-islands-judge.md new file mode 100644 index 000000000..289c8df90 --- /dev/null +++ b/.changeset/brown-islands-judge.md @@ -0,0 +1,5 @@ +--- +'@segment/analytics-core': patch +--- + +Fix bug where delay and pTimeout are coupled diff --git a/packages/core/src/analytics/__tests__/dispatch.test.ts b/packages/core/src/analytics/__tests__/dispatch.test.ts new file mode 100644 index 000000000..1111d0cf5 --- /dev/null +++ b/packages/core/src/analytics/__tests__/dispatch.test.ts @@ -0,0 +1,125 @@ +/** + * Properly type mocked functions to make it easy to do assertions + * for example, myModule.mock.calls[0] will have the typed parameters instead of any. + * + * TODO: share with rest of project + */ +type JestMockedFn = Fn extends (...args: infer Args) => infer ReturnT + ? jest.Mock + : never + +const isOnline = jest.fn().mockReturnValue(true) +const isOffline = jest.fn().mockReturnValue(false) + +jest.mock('../../connection', () => ({ + isOnline, + isOffline, +})) + +const fetcher: JestMockedFn = jest.fn() +jest.mock('node-fetch', () => fetcher) + +const invokeCallback: JestMockedFn< + typeof import('../../callback')['invokeCallback'] +> = jest.fn() +jest.mock('../../callback', () => ({ + invokeCallback: invokeCallback, +})) + +import { EventQueue } from '../../queue/event-queue' +import { Emitter } from '../../emitter' +import { dispatch, getDelay } from '../dispatch' +import { PriorityQueue } from '../../priority-queue' +import { CoreContext } from '../../context' + +let emitter!: Emitter +let queue!: EventQueue +const dispatchSingleSpy = jest.spyOn(EventQueue.prototype, 'dispatchSingle') +const dispatchSpy = jest.spyOn(EventQueue.prototype, 'dispatch') +const screenCtxMatcher = expect.objectContaining>({ + event: { type: 'screen' }, +}) +describe('Dispatch', () => { + beforeEach(() => { + jest.resetAllMocks() + dispatchSingleSpy.mockImplementationOnce((ctx) => Promise.resolve(ctx)) + invokeCallback.mockImplementationOnce((ctx) => Promise.resolve(ctx)) + dispatchSpy.mockImplementationOnce((ctx) => Promise.resolve(ctx)) + queue = new EventQueue(new PriorityQueue(4, [])) + queue.isEmpty = jest.fn().mockReturnValue(false) + emitter = new Emitter() + }) + + it('should not dispatch if client is currently offline and retries are *disabled* for the main event queue', async () => { + isOnline.mockReturnValue(false) + isOffline.mockReturnValue(true) + + const ctx = await dispatch({ type: 'screen' }, queue, emitter, { + retryQueue: false, + }) + expect(ctx).toEqual(screenCtxMatcher) + const called = Boolean( + dispatchSingleSpy.mock.calls.length || dispatchSpy.mock.calls.length + ) + expect(called).toBeFalsy() + }) + + it('should be allowed to dispatch if client is currently offline and retries are *enabled* for the main event queue', async () => { + isOnline.mockReturnValue(false) + isOffline.mockReturnValue(true) + + await dispatch({ type: 'screen' }, queue, emitter, { + retryQueue: true, + }) + const called = Boolean( + dispatchSingleSpy.mock.calls.length || dispatchSpy.mock.calls.length + ) + expect(called).toBeTruthy() + }) + + it('should call dispatchSingle correctly if queue is empty', async () => { + queue.isEmpty = jest.fn().mockReturnValue(true) + await dispatch({ type: 'screen' }, queue, emitter) + expect(dispatchSingleSpy).toBeCalledWith(screenCtxMatcher) + expect(dispatchSpy).not.toBeCalled() + }) + + it('should call dispatch correctly if queue has items', async () => { + await dispatch({ type: 'screen' }, queue, emitter) + expect(dispatchSpy).toBeCalledWith(screenCtxMatcher) + expect(dispatchSingleSpy).not.toBeCalled() + }) + + it('should only call invokeCallback if callback is passed', async () => { + await dispatch({ type: 'screen' }, queue, emitter) + expect(invokeCallback).not.toBeCalled() + + const cb = jest.fn() + await dispatch({ type: 'screen' }, queue, emitter, { callback: cb }) + expect(invokeCallback).toBeCalledTimes(1) + }) + it('should call invokeCallback with correct args', async () => { + const cb = jest.fn() + await dispatch({ type: 'screen' }, queue, emitter, { + callback: cb, + }) + expect(dispatchSpy).toBeCalledWith(screenCtxMatcher) + expect(invokeCallback).toBeCalledTimes(1) + const [ctx, _cb] = invokeCallback.mock.calls[0] + expect(ctx).toEqual(screenCtxMatcher) + expect(_cb).toBe(cb) + }) +}) + +describe(getDelay, () => { + it('should calculate the amount of time to delay before invoking the callback', () => { + const aShortTimeAgo = Date.now() - 200 + const timeout = 5000 + expect(Math.round(getDelay(aShortTimeAgo, timeout))).toBe(4800) + }) + + it('should have a sensible default', () => { + const aShortTimeAgo = Date.now() - 200 + expect(Math.round(getDelay(aShortTimeAgo))).toBe(100) + }) +}) diff --git a/packages/core/src/analytics/dispatch.ts b/packages/core/src/analytics/dispatch.ts index 3645d02af..787b96511 100644 --- a/packages/core/src/analytics/dispatch.ts +++ b/packages/core/src/analytics/dispatch.ts @@ -12,6 +12,12 @@ type DispatchOptions = { retryQueue?: boolean } +/* The amount of time in ms to wait before invoking the callback. */ +export const getDelay = (startTimeInEpochMS: number, timeoutInMS?: number) => { + const elapsedTime = Date.now() - startTimeInEpochMS + // increasing the timeout increases the delay by almost the same amount -- this is weird legacy behavior. + return Math.max((timeoutInMS ?? 300) - elapsedTime, 0) +} /** * Push an event into the dispatch queue and invoke any callbacks. * @@ -40,15 +46,12 @@ export async function dispatch( } else { dispatched = await queue.dispatch(ctx) } - const elapsedTime = Date.now() - startTime - const timeoutInMs = options?.timeout if (options?.callback) { dispatched = await invokeCallback( dispatched, options.callback, - Math.max((timeoutInMs ?? 300) - elapsedTime, 0), - timeoutInMs + getDelay(startTime, options.timeout) ) } if (options?.debug) { diff --git a/packages/core/src/callback/__tests__/index.test.ts b/packages/core/src/callback/__tests__/index.test.ts index dc6d562d8..8ceebf2d8 100644 --- a/packages/core/src/callback/__tests__/index.test.ts +++ b/packages/core/src/callback/__tests__/index.test.ts @@ -19,37 +19,37 @@ describe(invokeCallback, () => { }) // Fixes GitHub issue: https://github.com/segmentio/analytics-next/issues/409 - // A.JS classic waited for the timeout before invoking callback, + // A.JS classic waited for the timeout/delay before invoking callback, // so keep same behavior in A.JS next. - it('calls the callback after a timeout', async () => { + it('calls the callback after a delay', async () => { const ctx = new CoreContext({ type: 'track', }) const fn = jest.fn() - const timeout = 100 + const delay = 100 const startTime = Date.now() - const returned = await invokeCallback(ctx, fn, timeout) + const returned = await invokeCallback(ctx, fn, delay) const endTime = Date.now() expect(fn).toHaveBeenCalled() - expect(endTime - startTime).toBeGreaterThanOrEqual(timeout - 1) + expect(endTime - startTime).toBeGreaterThanOrEqual(delay - 1) expect(returned).toBe(ctx) }) - it('ignores the callback after a timeout', async () => { + it('ignores the callback if it takes too long to resolve', async () => { const ctx = new CoreContext({ type: 'track', }) const slow = (_ctx: CoreContext): Promise => { return new Promise((resolve) => { - setTimeout(resolve, 200) + setTimeout(resolve, 1100) }) } - const returned = await invokeCallback(ctx, slow, 0, 50) + const returned = await invokeCallback(ctx, slow, 0) expect(returned).toBe(ctx) const logs = returned.logs() diff --git a/packages/core/src/callback/index.ts b/packages/core/src/callback/index.ts index cdad20627..1fca70a94 100644 --- a/packages/core/src/callback/index.ts +++ b/packages/core/src/callback/index.ts @@ -22,14 +22,14 @@ function sleep(timeoutInMs: number): Promise { } /** - * @param delayTimeout - The amount of time in ms to wait before invoking the callback. - * @param timeout - The maximum amount of time in ms to allow the callback to run for. + * @param ctx + * @param callback - the function to invoke + * @param delay - aka "timeout". The amount of time in ms to wait before invoking the callback. */ export function invokeCallback( ctx: CoreContext, callback: Callback, - delayTimeout: number, - timeout?: number + delay: number ): Promise { const cb = () => { try { @@ -40,9 +40,9 @@ export function invokeCallback( } return ( - sleep(delayTimeout) + sleep(delay) // pTimeout ensures that the callback can't cause the context to hang - .then(() => pTimeout(cb(), timeout ?? 1000)) + .then(() => pTimeout(cb(), 1000)) .catch((err) => { ctx?.log('warn', 'Callback Error', { error: err }) ctx?.stats?.increment('callback_error')