Skip to content

Commit 9d8ecc3

Browse files
authored
fix: Cleanup test cookies (#381)
* fix: Cleanup amp_cookie_test after exceptions to prevent cookies excess * Remove console log * warning * Fix logging * improve warning when error thrown for cookie check * Add tests * Make tests more concise
1 parent 4f70c24 commit 9d8ecc3

File tree

2 files changed

+67
-12
lines changed

2 files changed

+67
-12
lines changed

src/base-cookie.js

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import Constants from './constants';
22
import base64Id from './base64Id';
3+
import utils from './utils';
34

45
const get = (name) => {
56
try {
@@ -48,15 +49,20 @@ const set = (name, value, opts) => {
4849

4950
// test that cookies are enabled - navigator.cookiesEnabled yields false positives in IE, need to test directly
5051
const areCookiesEnabled = (opts = {}) => {
51-
const uid = String(new Date());
52+
const cookieName = Constants.COOKIE_TEST_PREFIX + base64Id();
53+
let _areCookiesEnabled = false;
5254
try {
53-
const cookieName = Constants.COOKIE_TEST_PREFIX + base64Id();
55+
const uid = String(new Date());
5456
set(cookieName, uid, opts);
55-
const _areCookiesEnabled = get(cookieName + '=') === uid;
57+
utils.log.info(`Testing if cookies available`);
58+
_areCookiesEnabled = get(cookieName + '=') === uid;
59+
} catch (e) {
60+
utils.log.warn(`Error thrown when checking for cookies. Reason: "${e}"`);
61+
} finally {
62+
utils.log.warn(`Cleaning up cookies availability test`);
5663
set(cookieName, null, opts);
57-
return _areCookiesEnabled;
58-
} catch (e) {} /* eslint-disable-line no-empty */
59-
return false;
64+
}
65+
return _areCookiesEnabled;
6066
};
6167

6268
export default {

test/base-cookie.js

Lines changed: 55 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
import sinon from 'sinon';
22
import cookie from '../src/base-cookie';
3+
import base64Id from '../src/base64Id';
4+
import Constants from '../src/constants';
5+
import utils from '../src/utils';
36
import { mockCookie, restoreCookie, getCookie } from './mock-cookie';
47

58
describe('cookie', function () {
@@ -43,18 +46,64 @@ describe('cookie', function () {
4346
});
4447

4548
it('should return null when attempting to retrieve a cookie that does not exist', () => {
46-
assert.equal(cookie.get('key='), null);
49+
assert.isNull(cookie.get('key='));
4750
});
4851
});
4952

5053
describe('areCookiesEnabled', () => {
51-
it('return false when it cannot write to a cookie', () => {
52-
mockCookie({ disabled: true });
53-
assert.equal(cookie.areCookiesEnabled(), false);
54+
before(() => {
55+
sinon.stub(Math, 'random').returns(1);
5456
});
57+
after(() => {
58+
sinon.restore();
59+
});
60+
afterEach(() => {
61+
restoreCookie();
62+
sinon.restore();
63+
});
64+
65+
describe('when it can write to a cookie', () => {
66+
it('should return true', () => {
67+
assert.isTrue(cookie.areCookiesEnabled());
68+
});
69+
70+
it('should cleanup cookies', () => {
71+
const cookieName = Constants.COOKIE_TEST_PREFIX + base64Id();
72+
cookie.areCookiesEnabled();
73+
assert.isNull(cookie.get(`${cookieName}=`), null);
74+
});
75+
});
76+
77+
describe('when it cannot write to a cookie', () => {
78+
beforeEach(() => {
79+
mockCookie({ disabled: true });
80+
});
81+
82+
it('should return false', () => {
83+
assert.isFalse(cookie.areCookiesEnabled());
84+
});
85+
86+
it('should cleanup cookies', () => {
87+
const cookieName = Constants.COOKIE_TEST_PREFIX + base64Id();
88+
89+
cookie.areCookiesEnabled();
90+
assert.isNull(cookie.get(`${cookieName}=`));
91+
});
92+
});
93+
94+
describe('when error is thrown during check', () => {
95+
it('should cleanup cookies', () => {
96+
const stubLogInfo = sinon.stub(utils.log, 'info').throws('Stubbed Exception');
97+
const spyLogWarning = sinon.spy(utils.log, 'warn');
98+
const cookieName = Constants.COOKIE_TEST_PREFIX + base64Id();
99+
const res = cookie.areCookiesEnabled();
100+
assert.isFalse(res);
101+
assert.isTrue(spyLogWarning.calledWith('Error thrown when checking for cookies. Reason: "Stubbed Exception"'));
102+
assert.isNull(cookie.get(`${cookieName}=`));
55103

56-
it('should return true when it can write to a cookie', () => {
57-
assert.equal(cookie.areCookiesEnabled(), true);
104+
stubLogInfo.restore();
105+
spyLogWarning.restore();
106+
});
58107
});
59108
});
60109
});

0 commit comments

Comments
 (0)