Skip to content

Commit bfebb68

Browse files
mjc1283mjc1283
authored
refactor (datafile management): Remove timeout factory (#267)
Summary: This removes timeoutFactory as a required property in the config object accepted by the HTTPPollingDatafileManager constructor. timeoutFactory was only used as a way to control timeouts during test runs. Instead, use Jest's fake timer functionality. Test plan: Updated unit tests. Manually tested Issues: https://optimizely.atlassian.net/browse/OASIS-4669
1 parent 39c3b2d commit bfebb68

File tree

8 files changed

+91
-128
lines changed

8 files changed

+91
-128
lines changed

‎packages/datafile-manager/CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ Changes that have landed but are not yet released.
1212
- Updated polling behavior:
1313
- Start update interval timer immediately after request
1414
- When update interval timer fires during request, wait until request completes, then immediately start next request
15+
- Remove `timeoutFactory` property from `HTTPPollingDatafileManager` constructor argument object
1516

1617
## [0.2.0] - April 9, 2019
1718

‎packages/datafile-manager/__test__/browserDatafileManager.spec.ts‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,19 +17,18 @@
1717
import BrowserDatafileManager from '../src/browserDatafileManager'
1818
import * as browserRequest from '../src/browserRequest'
1919
import { Headers, AbortableRequest } from '../src/http'
20-
import TestTimeoutFactory from './testTimeoutFactory'
20+
import { advanceTimersByTime, getTimerCount } from './testUtils';
2121

2222
describe('browserDatafileManager', () => {
23-
const testTimeoutFactory: TestTimeoutFactory = new TestTimeoutFactory()
24-
2523
let makeGetRequestSpy: jest.SpyInstance<AbortableRequest, [string, Headers]>
2624
beforeEach(() => {
25+
jest.useFakeTimers()
2726
makeGetRequestSpy = jest.spyOn(browserRequest, 'makeGetRequest')
2827
})
2928

3029
afterEach(() => {
3130
jest.restoreAllMocks()
32-
testTimeoutFactory.cleanup()
31+
jest.clearAllTimers()
3332
})
3433

3534
it('calls makeGetRequest when started', async () => {
@@ -69,11 +68,10 @@ describe('browserDatafileManager', () => {
6968
const manager = new BrowserDatafileManager({
7069
sdkKey: '1234',
7170
autoUpdate: true,
72-
timeoutFactory: testTimeoutFactory,
7371
})
7472
manager.start()
7573
await manager.onReady()
76-
testTimeoutFactory.timeoutFns[0]()
74+
await advanceTimersByTime(300000)
7775
expect(makeGetRequestSpy).toBeCalledTimes(2)
7876
expect(makeGetRequestSpy.mock.calls[1][0]).toBe('https://cdn.optimizely.com/datafiles/1234.json')
7977
expect(makeGetRequestSpy.mock.calls[1][1]).toEqual({
@@ -96,12 +94,11 @@ describe('browserDatafileManager', () => {
9694
})
9795
const manager = new BrowserDatafileManager({
9896
sdkKey: '1234',
99-
timeoutFactory: testTimeoutFactory,
10097
})
10198
manager.start()
10299
await manager.onReady()
103100
// Should not set a timeout for a later update
104-
expect(testTimeoutFactory.timeoutFns.length).toBe(0)
101+
expect(getTimerCount()).toBe(0)
105102

106103
await manager.stop()
107104
})

‎packages/datafile-manager/__test__/httpPollingDatafileManager.spec.ts‎

Lines changed: 65 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
import HTTPPollingDatafileManager from '../src/httpPollingDatafileManager'
1818
import { Headers, AbortableRequest, Response } from '../src/http'
1919
import { DatafileManagerConfig } from '../src/datafileManager';
20-
import TestTimeoutFactory from './testTimeoutFactory'
20+
import { advanceTimersByTime, getTimerCount } from './testUtils'
2121

2222
jest.mock('../src/backoffController', () => {
2323
return jest.fn().mockImplementation(() => {
@@ -59,29 +59,23 @@ class TestDatafileManager extends HTTPPollingDatafileManager {
5959
}
6060

6161
describe('httpPollingDatafileManager', () => {
62-
const testTimeoutFactory: TestTimeoutFactory = new TestTimeoutFactory()
63-
64-
function createTestManager(config: DatafileManagerConfig): TestDatafileManager {
65-
return new TestDatafileManager({
66-
...config,
67-
timeoutFactory: testTimeoutFactory
68-
})
69-
}
62+
beforeEach(() => {
63+
jest.useFakeTimers()
64+
})
7065

7166
let manager: TestDatafileManager
7267
afterEach(async () => {
73-
testTimeoutFactory.cleanup()
74-
7568
if (manager) {
7669
manager.stop()
7770
}
7871
jest.clearAllMocks()
7972
jest.restoreAllMocks()
73+
jest.clearAllTimers()
8074
})
8175

8276
describe('when constructed with sdkKey and datafile and autoUpdate: true,', () => {
8377
beforeEach(() => {
84-
manager = createTestManager({ datafile: { foo: 'abcd' }, sdkKey: '123', autoUpdate: true })
78+
manager = new TestDatafileManager({ datafile: { foo: 'abcd' }, sdkKey: '123', autoUpdate: true })
8579
})
8680

8781
it('returns the passed datafile from get', () => {
@@ -118,7 +112,9 @@ describe('httpPollingDatafileManager', () => {
118112
})
119113
expect(manager.get()).toEqual({ foo: 'bar' })
120114
updateFn.mockReset()
121-
testTimeoutFactory.timeoutFns[0]()
115+
116+
await advanceTimersByTime(300000)
117+
122118
expect(manager.responsePromises.length).toBe(2)
123119
await manager.responsePromises[1]
124120
expect(updateFn).toBeCalledTimes(1)
@@ -131,7 +127,7 @@ describe('httpPollingDatafileManager', () => {
131127

132128
describe('when constructed with sdkKey and datafile and autoUpdate: false,', () => {
133129
beforeEach(() => {
134-
manager = createTestManager({ datafile: { foo: 'abcd' }, sdkKey: '123', autoUpdate: false })
130+
manager = new TestDatafileManager({ datafile: { foo: 'abcd' }, sdkKey: '123', autoUpdate: false })
135131
})
136132

137133
it('returns the passed datafile from get', () => {
@@ -160,13 +156,13 @@ describe('httpPollingDatafileManager', () => {
160156
datafile: { foo: 'bar' }
161157
})
162158
expect(manager.get()).toEqual({ foo: 'bar' })
163-
expect(testTimeoutFactory.timeoutFns.length).toBe(0)
159+
expect(getTimerCount()).toBe(0)
164160
})
165161
})
166162

167163
describe('when constructed with sdkKey and autoUpdate: true', () => {
168164
beforeEach(() => {
169-
manager = createTestManager({ sdkKey: '123', updateInterval: 1000, autoUpdate: true })
165+
manager = new TestDatafileManager({ sdkKey: '123', updateInterval: 1000, autoUpdate: true })
170166
})
171167

172168
describe('initial state', () => {
@@ -215,22 +211,32 @@ describe('httpPollingDatafileManager', () => {
215211
)
216212
manager.start()
217213
await manager.onReady()
218-
testTimeoutFactory.timeoutFns[0]()
214+
await advanceTimersByTime(1000)
215+
expect(manager.responsePromises.length).toBe(2)
219216
await manager.responsePromises[1]
220217
expect(manager.get()).toEqual({ foo: 'bar' })
221218
})
222219

223220
describe('live updates', () => {
224-
it('passes the update interval to its timeoutFactory setTimeout method', () => {
225-
manager.queuedResponses.push({
226-
statusCode: 200,
227-
body: '{"foo3": "bar3"}',
228-
headers: {},
229-
})
230-
const setTimeoutSpy: jest.SpyInstance<() => void, [() => void, number]> = jest.spyOn(testTimeoutFactory, 'setTimeout')
221+
it('sets a timeout to update again after the update interval', async () => {
222+
manager.queuedResponses.push(
223+
{
224+
statusCode: 200,
225+
body: '{"foo3": "bar3"}',
226+
headers: {},
227+
},
228+
{
229+
statusCode: 200,
230+
body: '{"foo4": "bar4"}',
231+
headers: {},
232+
},
233+
)
234+
const makeGetRequestSpy = jest.spyOn(manager, 'makeGetRequest')
231235
manager.start()
232-
expect(setTimeoutSpy).toBeCalledTimes(1)
233-
expect(setTimeoutSpy.mock.calls[0][1]).toBe(1000)
236+
expect(makeGetRequestSpy).toBeCalledTimes(1)
237+
await manager.responsePromises[0]
238+
await advanceTimersByTime(1000)
239+
expect(makeGetRequestSpy).toBeCalledTimes(2)
234240
})
235241

236242
it('emits update events after live updates', async () => {
@@ -260,15 +266,15 @@ describe('httpPollingDatafileManager', () => {
260266
expect(manager.get()).toEqual({ foo: 'bar' })
261267
expect(updateFn).toBeCalledTimes(0)
262268

263-
testTimeoutFactory.timeoutFns[0]()
269+
await advanceTimersByTime(1000)
264270
await manager.responsePromises[1]
265271
expect(updateFn).toBeCalledTimes(1)
266272
expect(updateFn.mock.calls[0][0]).toEqual({ datafile: { foo2: 'bar2' } })
267273
expect(manager.get()).toEqual({ foo2: 'bar2' })
268274

269275
updateFn.mockReset()
270276

271-
testTimeoutFactory.timeoutFns[1]()
277+
await advanceTimersByTime(1000)
272278
await manager.responsePromises[2]
273279
expect(updateFn).toBeCalledTimes(1)
274280
expect(updateFn.mock.calls[0][0]).toEqual({ datafile: { foo3: 'bar3' } })
@@ -285,13 +291,11 @@ describe('httpPollingDatafileManager', () => {
285291
abort() {},
286292
responsePromise,
287293
})
288-
const setTimeoutSpy = jest.spyOn(testTimeoutFactory, 'setTimeout')
289294

290295
manager.start()
291-
expect(setTimeoutSpy).toBeCalledTimes(1)
292296
expect(makeGetRequestSpy).toBeCalledTimes(1)
293297

294-
testTimeoutFactory.timeoutFns[0]()
298+
await advanceTimersByTime(1000)
295299
expect(makeGetRequestSpy).toBeCalledTimes(1)
296300

297301
resolveResponsePromise!({
@@ -301,7 +305,6 @@ describe('httpPollingDatafileManager', () => {
301305
})
302306
await responsePromise
303307
expect(makeGetRequestSpy).toBeCalledTimes(2)
304-
expect(setTimeoutSpy).toBeCalledTimes(2)
305308
})
306309
})
307310

@@ -317,10 +320,9 @@ describe('httpPollingDatafileManager', () => {
317320
manager.start()
318321
await manager.onReady()
319322

320-
expect(testTimeoutFactory.timeoutFns.length).toBe(1)
321-
expect(testTimeoutFactory.cancelFns.length).toBe(1)
323+
expect(getTimerCount()).toBe(1)
322324
manager.stop()
323-
expect(testTimeoutFactory.cancelFns[0]).toBeCalledTimes(1)
325+
expect(getTimerCount()).toBe(0)
324326
})
325327

326328
it('cancels reactions to a pending fetch when stop is called', async () => {
@@ -340,7 +342,9 @@ describe('httpPollingDatafileManager', () => {
340342
manager.start()
341343
await manager.onReady()
342344
expect(manager.get()).toEqual({ foo: 'bar' })
343-
testTimeoutFactory.timeoutFns[0]()
345+
346+
advanceTimersByTime(1000)
347+
344348
expect(manager.responsePromises.length).toBe(2)
345349
manager.stop()
346350
await manager.responsePromises[1]
@@ -383,9 +387,9 @@ describe('httpPollingDatafileManager', () => {
383387
expect(manager.responsePromises.length).toBe(1)
384388
await manager.responsePromises[0]
385389
// Not ready yet due to first request failed, but should have queued a live update
386-
expect(testTimeoutFactory.timeoutFns.length).toBe(1)
390+
expect(getTimerCount()).toBe(1)
387391
// Trigger the update, should fetch the next response which should succeed, then we get ready
388-
testTimeoutFactory.timeoutFns[0]()
392+
advanceTimersByTime(1000)
389393
await manager.onReady()
390394
expect(manager.get()).toEqual({ foo: 'bar' })
391395
})
@@ -416,7 +420,7 @@ describe('httpPollingDatafileManager', () => {
416420
// First response promise was for the initial 200 response
417421
expect(manager.responsePromises.length).toBe(1)
418422
// Trigger the queued update
419-
testTimeoutFactory.timeoutFns[0]()
423+
advanceTimersByTime(1000)
420424
// Second response promise is for the 304 response
421425
expect(manager.responsePromises.length).toBe(2)
422426
await manager.responsePromises[1]
@@ -443,7 +447,7 @@ describe('httpPollingDatafileManager', () => {
443447
manager.start()
444448
await manager.onReady()
445449
const makeGetRequestSpy = jest.spyOn(manager, 'makeGetRequest')
446-
testTimeoutFactory.timeoutFns[0]()
450+
advanceTimersByTime(1000)
447451
expect(makeGetRequestSpy).toBeCalledTimes(1)
448452
const firstCall = makeGetRequestSpy.mock.calls[0]
449453
const headers = firstCall[1]
@@ -458,7 +462,9 @@ describe('httpPollingDatafileManager', () => {
458462
const BackoffControllerMock = (BackoffController as unknown) as jest.Mock<BackoffController, []>
459463
const getDelayMock = BackoffControllerMock.mock.results[0].value.getDelay
460464
getDelayMock.mockImplementationOnce(() => 5432)
461-
const setTimeoutSpy = jest.spyOn(testTimeoutFactory, 'setTimeout')
465+
466+
const makeGetRequestSpy = jest.spyOn(manager, 'makeGetRequest')
467+
462468
manager.queuedResponses.push(
463469
{
464470
statusCode: 404,
@@ -468,8 +474,15 @@ describe('httpPollingDatafileManager', () => {
468474
)
469475
manager.start()
470476
await manager.responsePromises[0]
471-
expect(setTimeoutSpy).toBeCalledTimes(1)
472-
expect(setTimeoutSpy.mock.calls[0][1]).toBe(5432)
477+
expect(makeGetRequestSpy).toBeCalledTimes(1)
478+
479+
// Should not make another request after 1 second because the error should have triggered backoff
480+
advanceTimersByTime(1000)
481+
expect(makeGetRequestSpy).toBeCalledTimes(1)
482+
483+
// But after another 5 seconds, another request should be made
484+
advanceTimersByTime(5000)
485+
expect(makeGetRequestSpy).toBeCalledTimes(2)
473486
})
474487

475488
it('calls countError on the backoff controller when a non-success status code response is received', async () => {
@@ -531,7 +544,7 @@ describe('httpPollingDatafileManager', () => {
531544

532545
describe('when constructed with sdkKey and autoUpdate: false', () => {
533546
beforeEach(() => {
534-
manager = createTestManager({ sdkKey: '123', autoUpdate: false })
547+
manager = new TestDatafileManager({ sdkKey: '123', autoUpdate: false })
535548
})
536549

537550
it('after being started, fetches the datafile and resolves onReady', async () => {
@@ -555,7 +568,7 @@ describe('httpPollingDatafileManager', () => {
555568
manager.on('update', updateFn)
556569
manager.start()
557570
await manager.onReady()
558-
expect(testTimeoutFactory.timeoutFns.length).toBe(0)
571+
expect(getTimerCount()).toBe(0)
559572
})
560573

561574
// TODO: figure out what's wrong with this test
@@ -579,7 +592,7 @@ describe('httpPollingDatafileManager', () => {
579592

580593
describe('when constructed with sdkKey and a valid urlTemplate', () => {
581594
beforeEach(() => {
582-
manager = createTestManager({
595+
manager = new TestDatafileManager({
583596
sdkKey: '456',
584597
updateInterval: 1000,
585598
urlTemplate: 'https://localhost:5556/datafiles/%s',
@@ -602,22 +615,23 @@ describe('httpPollingDatafileManager', () => {
602615

603616
describe('when constructed with an update interval below the minimum', () => {
604617
beforeEach(() => {
605-
manager = createTestManager({ sdkKey: '123', updateInterval: 500, autoUpdate: true })
618+
manager = new TestDatafileManager({ sdkKey: '123', updateInterval: 500, autoUpdate: true })
606619
})
607620

608621
it('uses the default update interval', async () => {
622+
const makeGetRequestSpy = jest.spyOn(manager, 'makeGetRequest')
623+
609624
manager.queuedResponses.push({
610625
statusCode: 200,
611626
body: '{"foo3": "bar3"}',
612627
headers: {},
613628
})
614629

615-
const setTimeoutSpy: jest.SpyInstance<() => void, [() => void, number]> = jest.spyOn(testTimeoutFactory, 'setTimeout')
616-
617630
manager.start()
618631
await manager.onReady()
619-
expect(setTimeoutSpy).toBeCalledTimes(1)
620-
expect(setTimeoutSpy.mock.calls[0][1]).toBe(300000)
632+
expect(makeGetRequestSpy).toBeCalledTimes(1)
633+
await advanceTimersByTime(300000)
634+
expect(makeGetRequestSpy).toBeCalledTimes(2)
621635
})
622636
})
623637
})

0 commit comments

Comments
 (0)