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
amplitude · Apr 30, 2021 · 9d8ecc3 · 9d8ecc3
1 parent 4f70c24
commit 9d8ecc3
Show file tree

Hide file tree

Showing 2 changed files with 67 additions and 12 deletions.
diff --git a/src/base-cookie.js b/src/base-cookie.js
@@ -1,5 +1,6 @@
 import Constants from './constants';
 import base64Id from './base64Id';
+import utils from './utils';
 
 const get = (name) => {
   try {
@@ -48,15 +49,20 @@ const set = (name, value, opts) => {
 
 // test that cookies are enabled - navigator.cookiesEnabled yields false positives in IE, need to test directly
 const areCookiesEnabled = (opts = {}) => {
-  const uid = String(new Date());
+  const cookieName = Constants.COOKIE_TEST_PREFIX + base64Id();
+  let _areCookiesEnabled = false;
   try {
-    const cookieName = Constants.COOKIE_TEST_PREFIX + base64Id();
+    const uid = String(new Date());
     set(cookieName, uid, opts);
-    const _areCookiesEnabled = get(cookieName + '=') === uid;
+    utils.log.info(`Testing if cookies available`);
+    _areCookiesEnabled = get(cookieName + '=') === uid;
+  } catch (e) {
+    utils.log.warn(`Error thrown when checking for cookies. Reason: "${e}"`);
+  } finally {
+    utils.log.warn(`Cleaning up cookies availability test`);
     set(cookieName, null, opts);
-    return _areCookiesEnabled;
-  } catch (e) {} /* eslint-disable-line no-empty */
-  return false;
+  }
+  return _areCookiesEnabled;
 };
 
 export default {

diff --git a/test/base-cookie.js b/test/base-cookie.js
@@ -1,5 +1,8 @@
 import sinon from 'sinon';
 import cookie from '../src/base-cookie';
+import base64Id from '../src/base64Id';
+import Constants from '../src/constants';
+import utils from '../src/utils';
 import { mockCookie, restoreCookie, getCookie } from './mock-cookie';
 
 describe('cookie', function () {
@@ -43,18 +46,64 @@ describe('cookie', function () {
     });
 
     it('should return null when attempting to retrieve a cookie that does not exist', () => {
-      assert.equal(cookie.get('key='), null);
+      assert.isNull(cookie.get('key='));
     });
   });
 
   describe('areCookiesEnabled', () => {
-    it('return false when it cannot write to a cookie', () => {
-      mockCookie({ disabled: true });
-      assert.equal(cookie.areCookiesEnabled(), false);
+    before(() => {
+      sinon.stub(Math, 'random').returns(1);
     });
+    after(() => {
+      sinon.restore();
+    });
+    afterEach(() => {
+      restoreCookie();
+      sinon.restore();
+    });
+
+    describe('when it can write to a cookie', () => {
+      it('should return true', () => {
+        assert.isTrue(cookie.areCookiesEnabled());
+      });
+
+      it('should cleanup cookies', () => {
+        const cookieName = Constants.COOKIE_TEST_PREFIX + base64Id();
+        cookie.areCookiesEnabled();
+        assert.isNull(cookie.get(`${cookieName}=`), null);
+      });
+    });
+
+    describe('when it cannot write to a cookie', () => {
+      beforeEach(() => {
+        mockCookie({ disabled: true });
+      });
+
+      it('should return false', () => {
+        assert.isFalse(cookie.areCookiesEnabled());
+      });
+
+      it('should cleanup cookies', () => {
+        const cookieName = Constants.COOKIE_TEST_PREFIX + base64Id();
+
+        cookie.areCookiesEnabled();
+        assert.isNull(cookie.get(`${cookieName}=`));
+      });
+    });
+
+    describe('when error is thrown during check', () => {
+      it('should cleanup cookies', () => {
+        const stubLogInfo = sinon.stub(utils.log, 'info').throws('Stubbed Exception');
+        const spyLogWarning = sinon.spy(utils.log, 'warn');
+        const cookieName = Constants.COOKIE_TEST_PREFIX + base64Id();
+        const res = cookie.areCookiesEnabled();
+        assert.isFalse(res);
+        assert.isTrue(spyLogWarning.calledWith('Error thrown when checking for cookies. Reason: "Stubbed Exception"'));
+        assert.isNull(cookie.get(`${cookieName}=`));
 
-    it('should return true when it can write to a cookie', () => {
-      assert.equal(cookie.areCookiesEnabled(), true);
+        stubLogInfo.restore();
+        spyLogWarning.restore();
+      });
     });
   });
 });