Skip to content

Commit 1741978

Browse files
jaissica12jaissica12
committed
fix stub leaks and timing races in session manager tests
1 parent a42fbf9 commit 1741978

File tree

2 files changed

+48
-18
lines changed

2 files changed

+48
-18
lines changed

‎src/sessionManager.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,6 @@ export default function SessionManager(
140140
Messages.InformationMessages.AbandonEndSession
141141
);
142142
mpInstance._timeOnSiteTimer?.resetTimer();
143-
144143
return;
145144
}
146145

@@ -152,7 +151,6 @@ export default function SessionManager(
152151
Messages.InformationMessages.NoSessionToEnd
153152
);
154153
mpInstance._timeOnSiteTimer?.resetTimer();
155-
156154
return;
157155
}
158156

@@ -168,9 +166,9 @@ export default function SessionManager(
168166
performSessionEnd();
169167
} else {
170168
self.setSessionTimer();
169+
mpInstance._timeOnSiteTimer?.resetTimer();
171170
}
172-
}
173-
mpInstance._timeOnSiteTimer?.resetTimer();
171+
}
174172
};
175173

176174
this.setSessionTimer = function (): void {
@@ -218,6 +216,10 @@ export default function SessionManager(
218216
* @returns true if the session has expired, false otherwise
219217
*/
220218
function hasSessionTimedOut(lastEventTimestamp: number, sessionTimeout: number): boolean {
219+
if (!lastEventTimestamp || !sessionTimeout || sessionTimeout <= 0) {
220+
return false;
221+
}
222+
221223
const sessionTimeoutInMilliseconds: number = sessionTimeout * 60000;
222224
const timeSinceLastEvent: number = Date.now() - lastEventTimestamp;
223225

‎test/src/tests-session-manager.ts‎

Lines changed: 42 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,14 @@ describe('SessionManager', () => {
294294
});
295295

296296
describe('#hasSessionTimedOut', () => {
297+
beforeEach(() => {
298+
clock = sinon.useFakeTimers(now.getTime());
299+
});
300+
301+
afterEach(() => {
302+
clock.restore();
303+
});
304+
297305
it('should return true when elapsed time exceeds session timeout', () => {
298306
const timePassed = 35 * (MILLIS_IN_ONE_SEC * 60); // 35 minutes
299307

@@ -329,9 +337,7 @@ describe('SessionManager', () => {
329337
});
330338

331339
it('should work consistently with both in-memory and persisted timestamps', () => {
332-
const now = new Date();
333-
const thirtyOneMinutesAgo = new Date();
334-
thirtyOneMinutesAgo.setMinutes(now.getMinutes() - 31);
340+
const thirtyOneMinutesAgo = new Date(now.getTime() - (31 * MILLIS_IN_ONE_SEC * 60));
335341

336342
mParticle.init(apiKey, window.mParticle.config);
337343
const mpInstance = mParticle.getInstance();
@@ -346,7 +352,7 @@ describe('SessionManager', () => {
346352

347353
// Test with persistence (via endSession)
348354
const newSessionId = mpInstance._Store.sessionId;
349-
sinon.stub(mpInstance._Persistence, 'getPersistence').returns({
355+
const getPersistenceStub = sinon.stub(mpInstance._Persistence, 'getPersistence').returns({
350356
gs: {
351357
les: thirtyOneMinutesAgo.getTime(),
352358
sid: newSessionId,
@@ -357,17 +363,18 @@ describe('SessionManager', () => {
357363

358364
// Session should have ended (same timeout logic)
359365
expect(mpInstance._Store.sessionId).to.equal(null);
366+
367+
// Clean up stub
368+
getPersistenceStub.restore();
360369
});
361370

362371
it('should return true when elapsed time equals session timeout exactly', () => {
363-
const now = new Date();
364-
const exactlyThirtyMinutesAgo = new Date();
365-
exactlyThirtyMinutesAgo.setMinutes(now.getMinutes() - 30);
372+
const exactlyThirtyMinutesAgo = new Date(now.getTime() - (30 * MILLIS_IN_ONE_SEC * 60));
366373

367374
mParticle.init(apiKey, window.mParticle.config);
368375
const mpInstance = mParticle.getInstance();
369376

370-
sinon.stub(mpInstance._Persistence, 'getPersistence').returns({
377+
const getPersistenceStub = sinon.stub(mpInstance._Persistence, 'getPersistence').returns({
371378
gs: {
372379
les: exactlyThirtyMinutesAgo.getTime(),
373380
sid: 'TEST-ID',
@@ -378,6 +385,9 @@ describe('SessionManager', () => {
378385

379386
// At exactly 30 minutes, session should be expired
380387
expect(mpInstance._Store.sessionId).to.equal(null);
388+
389+
// Clean up stub
390+
getPersistenceStub.restore();
381391
});
382392
});
383393

@@ -522,7 +532,7 @@ describe('SessionManager', () => {
522532

523533
// Session Manager relies on persistence to determine last event sent (LES) time
524534
// Also requires sid to verify session exists
525-
sinon.stub(mpInstance._Persistence, 'getPersistence').returns({
535+
const getPersistenceStub = sinon.stub(mpInstance._Persistence, 'getPersistence').returns({
526536
gs: {
527537
les: twentyMinutesAgo,
528538
sid: 'fake-session-id',
@@ -539,6 +549,9 @@ describe('SessionManager', () => {
539549
// When session is not timed out, setSessionTimer is called to keep track
540550
// of current session timeout
541551
expect(timerSpy.getCalls().length).to.equal(1);
552+
553+
// Clean up stub
554+
getPersistenceStub.restore();
542555
});
543556

544557
it('should force a session end when override is used', () => {
@@ -694,7 +707,7 @@ describe('SessionManager', () => {
694707
mParticle.init(apiKey, window.mParticle.config);
695708

696709
const mpInstance = mParticle.getInstance();
697-
sinon.stub(mpInstance._Persistence, 'getPersistence').returns({
710+
const getPersistenceStub = sinon.stub(mpInstance._Persistence, 'getPersistence').returns({
698711
gs: {
699712
sid: 'cookie-session-id',
700713
},
@@ -707,6 +720,9 @@ describe('SessionManager', () => {
707720
expect(mpInstance._Store.sessionId).to.equal(
708721
'cookie-session-id'
709722
);
723+
724+
// Clean up stub
725+
getPersistenceStub.restore();
710726
});
711727

712728
it('should end session if the session timeout limit has been reached', () => {
@@ -926,7 +942,7 @@ describe('SessionManager', () => {
926942
const mpInstance = mParticle.getInstance();
927943

928944
// Session Manager relies on persistence check sid (Session ID)
929-
sinon.stub(mpInstance._Persistence, 'getPersistence').returns({
945+
const getPersistenceStub = sinon.stub(mpInstance._Persistence, 'getPersistence').returns({
930946
gs: {
931947
sid: null,
932948
},
@@ -941,6 +957,9 @@ describe('SessionManager', () => {
941957

942958
mpInstance._SessionManager.startNewSessionIfNeeded();
943959
expect(startNewSessionSpy.called).to.equal(true);
960+
961+
// Clean up stub
962+
getPersistenceStub.restore();
944963
});
945964

946965
it('should NOT call startNewSession if sessionId is undefined and Persistence is undefined', () => {
@@ -970,7 +989,7 @@ describe('SessionManager', () => {
970989
const mpInstance = mParticle.getInstance();
971990

972991
// Session Manager relies on persistence check sid (Session ID)
973-
sinon.stub(mpInstance._Persistence, 'getPersistence').returns({
992+
const getPersistenceStub = sinon.stub(mpInstance._Persistence, 'getPersistence').returns({
974993
gs: {
975994
sid: 'sid-from-persistence',
976995
},
@@ -981,14 +1000,17 @@ describe('SessionManager', () => {
9811000
mpInstance._SessionManager.startNewSessionIfNeeded();
9821001

9831002
mpInstance._Store.sessionId = 'sid-from-persistence';
1003+
1004+
// Clean up stub
1005+
getPersistenceStub.restore();
9841006
});
9851007

9861008
it('should set sessionId from Persistence if Store.sessionId is undefined', () => {
9871009
mParticle.init(apiKey, window.mParticle.config);
9881010
const mpInstance = mParticle.getInstance();
9891011

9901012
// Session Manager relies on persistence check sid (Session ID)
991-
sinon.stub(mpInstance._Persistence, 'getPersistence').returns({
1013+
const getPersistenceStub = sinon.stub(mpInstance._Persistence, 'getPersistence').returns({
9921014
gs: {
9931015
sid: 'sid-from-persistence',
9941016
},
@@ -1006,6 +1028,9 @@ describe('SessionManager', () => {
10061028
mpInstance._Store.sessionId = 'sid-from-persistence';
10071029

10081030
expect(startNewSessionSpy.called).to.equal(true);
1031+
1032+
// Clean up stub
1033+
getPersistenceStub.restore();
10091034
});
10101035

10111036
it('should NOT call startNewSession if Store.sessionId and Persistence are null', () => {
@@ -1075,7 +1100,7 @@ describe('SessionManager', () => {
10751100

10761101
// Session Manager relies on persistence to determine last time seen (LES)
10771102
// Also requires sid to verify session exists
1078-
sinon.stub(mpInstance._Persistence, 'getPersistence').returns({
1103+
const getPersistenceStub = sinon.stub(mpInstance._Persistence, 'getPersistence').returns({
10791104
gs: {
10801105
les: now,
10811106
sid: 'fake-session-id',
@@ -1093,6 +1118,9 @@ describe('SessionManager', () => {
10931118
// Persistence isn't necessary for this feature, but we should test
10941119
// to see that it is called in case this ever needs to be refactored
10951120
expect(persistenceSpy.called).to.equal(true);
1121+
1122+
// Clean up stub
1123+
getPersistenceStub.restore();
10961124
});
10971125

10981126
it('should call identify when SDKConfig.identifyRequest differs from getCurrentUser().userIdentities on page refresh', async () => {

0 commit comments

Comments
 (0)