Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cookie persistence, also fix some tests that are broken in modern browsers #192

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions lib/loglevel.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,12 @@
if (typeof storedLevel === undefinedType) {
try {
var cookie = window.document.cookie;
var location = cookie.indexOf(
encodeURIComponent(storageKey) + "=");
var cookieName = encodeURIComponent(storageKey);
var location = cookie.indexOf(cookieName + "=");
if (location !== -1) {
storedLevel = /^([^;]+)/.exec(cookie.slice(location))[1];
storedLevel = /^([^;]+)/.exec(
cookie.slice(location + cookieName.length + 1)
)[1];
Comment on lines -161 to +166
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual bugfix. The rest is just cleaning up bad test code.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this makes sense. Any idea why this worked in phantomjs though?

I imagine in practice we haven't noticed this because modern browsers all support local storage, and have for a long time, so the cookie fallback logic isn't really used much nowadays.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this makes sense. Any idea why this worked in phantomjs though?

It didn't! One of the reasons I switched to showing the tests as skipped was to verify for myself that none of the cookie tests run in PhantomJS. It turns out document.cookie is just always empty and cannot be set, which causes all the cookie-related and persistence-related tests to be skipped.

}
} catch (ignore) {}
}
Expand Down
4 changes: 4 additions & 0 deletions test/default-level-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ define(['test/test-helpers'], function(testHelpers) {
});

describe("If no level is saved", function() {
beforeEach(function () {
testHelpers.clearStoredLevels();
});

it("new level is always set", function(log) {
log.setDefaultLevel("trace");
expect(log).toBeAtLevel("trace");
Expand Down
4 changes: 4 additions & 0 deletions test/get-current-level-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ define(['test/test-helpers'], function(testHelpers) {
});

describe("If no level is saved", function() {
beforeEach(function() {
testHelpers.clearStoredLevels();
});

it("current level is the default level", function(log) {
log.setDefaultLevel("trace");
expect(log.getLevel()).toBe(log.levels.TRACE);
Expand Down
6 changes: 4 additions & 2 deletions test/local-storage-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -180,21 +180,23 @@ define(['test/test-helpers'], function(testHelpers) {
describeIf(testHelpers.isCookieStorageAvailable() && testHelpers.isLocalStorageAvailable(),
"if localStorage and cookies are both available", function () {

it("the level stored in cookies is ignored if a local storage level is set", function () {
it("the level stored in cookies is ignored if a local storage level is set", function (log, done) {
testHelpers.setCookieStoredLevel("info");
testHelpers.setLocalStorageStoredLevel("debug");

testHelpers.withFreshLog(function (log) {
expect(log).toBeAtLevel("debug");
done();
});
});

it("the level stored in cookies is used if no local storage level is set", function () {
it("the level stored in cookies is used if no local storage level is set", function (log, done) {
testHelpers.setCookieStoredLevel("info");
window.localStorage.clear();

testHelpers.withFreshLog(function (log) {
expect(log).toBeAtLevel("info");
done();
});
});

Expand Down
21 changes: 18 additions & 3 deletions test/multiple-logger-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,24 @@ define(['test/test-helpers'], function(testHelpers) {
expect(function() { log.getLogger(function(){}); }).toThrow();
expect(function() { log.getLogger(null); }).toThrow();
expect(function() { log.getLogger(undefined); }).toThrow();
if (window.Symbol) {
expect(function() { log.getLogger(Symbol()); }).toThrow();
}
});

// NOTE: this test is the same as the similarly-named test in
// `node-integration.js` (which only runs in Node.js). If making
// changes here, be sure to adjust that test as well.
it(typeof Symbol !== "undefined", "supports using symbols as names", function(log) {
var s1 = Symbol("a-symbol");
var s2 = Symbol("a-symbol");

var logger1 = log.getLogger(s1);
var defaultLevel = logger1.getLevel();
logger1.setLevel(log.levels.TRACE);

var logger2 = log.getLogger(s2);

// Should be unequal: same name, but different symbol instances
expect(logger1).not.toEqual(logger2);
expect(logger2.getLevel()).toEqual(defaultLevel);
});
});

Expand Down
3 changes: 3 additions & 0 deletions test/node-integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ describe("loglevel included via node", function () {
expect(console.info).toHaveBeenCalledWith("test message");
});

// NOTE: this test is the same as the similarly-named test in
// `multiple-logger-test.js` (which only runs in browsers). If making
// changes here, be sure to adjust that test as well.
it("supports using symbols as names", function() {
var log = require('../lib/loglevel');

Expand Down
38 changes: 28 additions & 10 deletions test/test-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ define(function () {
window.localStorage.clear();
}
if (self.isCookieStorageAvailable()) {
var storedKeys = window.document.cookie.match(/(?:^|;\s)(loglevel(\:\w+)?)(?=\=)/g);
var storedKeys = window.document.cookie.match(/(?:^|;\s)(loglevel(%3a\w+)?)(?=\=)/ig);
if (storedKeys) {
for (var i = 0; i < storedKeys.length; i++) {
window.document.cookie = storedKeys[i] + "=; expires=Thu, 01 Jan 1970 00:00:01 GMT;";
Expand All @@ -175,15 +175,15 @@ define(function () {
};

self.describeIf = function describeIf(condition, name, test) {
if (condition) {
jasmine.getEnv().describe(name, test);
}
var env = jasmine.getEnv();
var implementation = condition ? env.describe : env.xdescribe;
return implementation(name, test);
};

self.itIf = function itIf(condition, name, test) {
if (condition) {
jasmine.getEnv().it(name, test);
}
var env = jasmine.getEnv();
var implementation = condition ? env.it : env.xit;
return implementation(name, test);
};

// Forcibly reloads loglevel and asynchronously hands the resulting log to
Expand All @@ -196,9 +196,27 @@ define(function () {
});
};

// Wraps Jasmine's it(name, test) call to reload the loglevel dependency for the given test
self.itWithFreshLog = function itWithFreshLog(name, test) {
jasmine.getEnv().it(name, function(done) {
// Wraps Jasmine's `it(name, test)` call to reload the loglevel module
// for the given test. An optional boolean first argument causes this to
// behave like `itIf()` instead of `it()`.
//
// Normal usage:
// itWithFreshLog("test name", function(log) {
// // test code
// });
//
// Conditional usage:
// itWithFreshLog(shouldRunTest(), "test name", function(log) {
// // test code
// });
self.itWithFreshLog = function itWithFreshLog(condition, name, test) {
if (!test) {
test = name;
name = condition;
condition = true;
}

self.itIf(condition, name, function(done) {
function runTest (log) {
if (test.length > 1) {
return test(log, done);
Expand Down