Fix cookie persistence, also fix some tests that are broken in modern browsers #192
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes it a bit more obvious when tests were not run in a given environment, and therefore which behaviors might need more manual testing.
This test suite was failing in environments that actually had persistence (depending what order tests were run in). Note that it does not fail on PhantomJS, and to see failures under the current tooling, you must run `grunt integration-test` and then view the tests in a real browser.
The test code for Node vs. browsers got out of sync at some point when/after support for naming loggers with Symbols was added. Since PhantomJS does not support symbols, the test was silently succeeding when run via `npm test` but failing in actual browsers on `grunt integration-test` (which clearly hasn't been used recently). This: - Fixes the test. - Makes it a separate test so it can clearly be marked as skipped in environments where it is not run. - Makes the test implementation match its Node.js-based counterpart and adds a comment indicating that any changes made to one should be made to the other.
Two tests in the cookies + localstorage suite were handling async operations incorrectly (I think this is a change in Jasmine v2, and I missed these in pimterry#190 because these tests don't run from the command line). This fixes them, which resolves one test failure. The other is still failing because of an actual bug in the library.
It turns out loading logger levels from cookies has been broken (probably for a long time!), but since the tests that run from `npm test` don't actually test cookies, this was never caught (those tests use PhantomJS, which does not allow access to cookies from `document.cookie`, so these tests were skipped). When running tests by running `grunt integration-test` and then opening the tests in an actual browser, you can see this test is failing. The cause is that the cookie value is read starting from beginning of the whole key-value pair, rather than the start of the cookie, so we'd read in a value like `"loglevel=INFO"` instead of just `"INFO"`. Incidentally, fixing this causes *other* tests to fail (uhoh), which I will address in the next commit.
The `clearStoredLevels()` helper did not work correctly for named loggers -- it looked for cookie names like `loglevel:whatever` instead of `loglevel:%3Awhatever` (cookie names and values are URL-encoded). This also fixes another test that started failing after an earlier fix, I think because of timing/ordering issues. Explicitly clearing storage before the test fixed the issue (the test was implicitly expecting nothing to be stored).
Mr0grog
commented
Jan 11, 2024
Comment on lines
-161
to
+166
| 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]; |
There was a problem hiding this comment.
This is the actual bugfix. The rest is just cleaning up bad test code.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
pimterry
approved these changes
Jan 11, 2024
Comment on lines
-161
to
+166
| 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]; |
There was a problem hiding this comment.
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.
Mr0grog
deleted the
we-are-good-at-baking-cookies-but-not-very-good-at-eating-them
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a bug where any levels that were saved in cookies were not loaded correctly. It also fixes a bunch of tests that were broken only in live/modern browsers and not in Node.js or PhantomJS (what you get when you run
npm test). To see these failures, you would have had to start a server withgrunt integration-testand then opened the Jasmine test runner from the server that starts in a real browser.The cookie issue is that we were reading the value starting from the beginning of the cookie’s key-pair, so instead of reading a value like
"INFO", we’d readloglevel="INFO", which is obviously not a valid level.This also fixes a number of other test failures when running tests in an actual browser, but all of them were problems with the tests rather than actual bugs in the library like the above cookie issue. Fixing the cookie issue also revealed some other bugs in the test, so this was a bit of an iterative process.
Another thing I did with the tests here was change the
describeIf/itIfhelpers to mark tests that don’t get run as skipped rather than hiding them from test output entirely, so it’s at least a little more obvious that some things are not getting tested.