Cookie tests #67

carylwyatt · 2024-04-18T20:51:39Z

I'm a little embarrassed by how little I have accomplished in the few days I've been working on this, but I'm still learning/reading about testing and there was a surprising amount of setup!

Notes:

I'm using vitest because firebird uses vite bundling, and vitest was built to be backwards-compatible for jest. Storybook uses jest, so I'm already familiar-ish with that syntax.
Vitest comes with some built-in testing environments for the browser: jsdom and happy-dom. I decided to use happy-dom since it's smaller/more lightweight than jsdom, and we are already using storybook for UI tests. I only need happy-dom for browser functions (window, document, etc.)
Using istanbul for coverage: npm run coverage gives the break down.
I've never written a mock or spy before, so there was a little learning curve on the difference between those.
It took me a few hours to realize the reason some of my tests were failing is because of a window/domain/protocol mismatch, but that's been taken care of for future tests

netlify · 2024-04-18T20:51:58Z

✅ Deploy Preview for hathitrust-firebird-common ready!

Name	Link
🔨 Latest commit	`cef1bcb`
🔍 Latest deploy log	https://app.netlify.com/sites/hathitrust-firebird-common/deploys/66391e19e0fd7f0008f97605
😎 Deploy Preview	https://deploy-preview-67--hathitrust-firebird-common.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

carylwyatt · 2024-04-29T14:22:03Z

src/js/lib/analytics.test.js

This is where things started falling apart.

The value of "trackingConsent" is a Svelte store variable and there's this function that subscribes to that store and sets/unsets matomo cookies when consent is given/rejected. What I really want to test here is whether or not that store is updated, not necessarily if the matomo cookies are set... but I was struggling to figure out what exactly to mock in order to do that.

I'm not sure I fully understand what you're trying to test, but it sounds like you want to test the underlying Svelte store variable separately from how you set/access it in cookies.js? Or perhaps that the store variable is set up correctly such that it's working the way you expect?

Ultimately, I'd be inclined not to try to test the store variable itself but rather test the observed effect - that the things that are subscribed to it for updates are actually receiving the updates. A couple options I can think of there:

in the test, subscribe to the store variable, set the store variable, and then test that the callback from your subscription is reached.

the other option would be testing the effect of the existing callbacks -- more of an integration test -- but I'm not sure what _paq.push(['setCookieConsentGiven']); (maybe this hooks into what the matomo js itself does?).

aelkiss

I added some thoughts; we should go through this and discuss at some point -- I think it will be easier if you talk me through some of what's going on.

That said, I would not be inclined to sit on this too long. I would be OK with going ahead and merging this now if:

we schedule a time to return to this and talk through some of the issues
we add a github action that runs these tests on push (I think that should be straightforward; hopefully would be very similar to https://github.com/hathitrust/feedback-collector/blob/main/.github/workflows/tests.yaml).

If we don't get this merged now and/or aren't running the tests regularly, then I'm worried they'll get stale and the effort will end up being wasted.

aelkiss · 2024-05-01T18:31:02Z

package.json

@@ -6,7 +6,8 @@
  "type": "module",
  "scripts": {
    "dev": "vite",
-    "test": "echo \"Error: no test specified\" && exit 1",
+    "test": "vitest",
+    "coverage": "vitest run --coverage.enabled --coverage.provider=istanbul",


Would likely be worth seeing if we can report coverage to coveralls (https://coveralls.io/). It looks like we could probably configure this to output coverage data in lcov format (https://github.com/istanbuljs/istanbuljs/blob/main/packages/istanbul-reports/lib/lcov/index.js) and then use the github action (https://github.com/marketplace/actions/coveralls-github-action) to report coverage. @Ronster2018 could likely help with this at some point.

aelkiss · 2024-05-01T18:35:08Z

src/js/lib/analytics.js

-    g.async = true;
-    g.src = `https://${this.service}/js/container_${this.container}.js`;
-    s.parentNode.insertBefore(g, s);
+    if (!testing) {


I might consider extracting this part of the setup to another function, e.g. if (!testing) { addAnalyticsScript(); } or something, and perhaps extracting some of the other parts of setup to other functions at the same time.

After I wrote all this garbage, I did some reading/youtube watching about testing, mocking, spying, best practices, and I realized I probably need to be wrapping some functions or using dependency injection instead of what I was trying to do (use the functions directly to try to avoid mocking).

One option here that would avoid the need for the testing parameter would be to get g.src from some other function. For example:

g.src = tracking.trackingScriptSource();

and then when we're testing, overriding that object to provide an empty script or something.

The other option would be to inject an object that does all the parts of adding the setup script, and inject an object in the test that does nothing. Not sure what the best option is.

aelkiss · 2024-05-01T18:52:18Z

src/js/lib/cookies.test.js

+           expect(document).not.toBeUndefined() 
+        })
+})
+describe('TestCookieJar', () => {


It might be useful to move TestCookieJar, PingCallbackDecorator along with anything else that is only used for testing to somewhere that makes that more clear, but I'm not sure what the conventions are here. Regardless, we should avoid putting them in the compiled index.js (not immediately obvious to me whether they are or not right now).

I would also say we probably don't need to test TestCookieJar itself, since it's only used in tests and presumably if it isn't working as expected those other tests will fail. Sometimes I've found it useful to add tests like this when initially implementing a mock object like this, but I wouldn't be inclined to add them at this point unless perhaps you ran into issues where having them was helpful in understanding why other tests were breaking or something

I think TestCookieJar only exists for the PingCallbackDecorator in storybook tests, so that's probably right. It felt like an easy place to get started, but the tests don't really make sense in context of the actual cookie functionality.

aelkiss · 2024-05-01T19:03:18Z

src/js/lib/cookies.test.js

+    })
+})
+
+describe('setMarketingAllowedCookie', () => {


Looking at this it seems like it's:

expecting that the marketing allowed cookie is NOT set before the first test

leaves the marketing consent cookie set after the second test

I think we would want to reset to a known state before each test -- is there something I'm missing that's already doing that? It looks like there is a resetCookieBanner function that we might want to use for it, but it is not getting called anywhere that I see right now.

resetCookieBanner is from a earlier version of the banner and I never removed it. But it could be handy as a afterAll() option in future tests!

aelkiss · 2024-05-01T19:08:08Z

src/js/lib/cookies.test.js

+        docCookies.removeItem('HT-tracking-cookie-consent')
+    })
+    it.skip('should use setTrackingAllowedCookie', () => {
+        // const mockSetTrackingAllowedCookie = vi.fn().mockImplementation('setTrackingAllowedCookie')


If I understand correctly, it sounds like what you're trying to test is that setSelectedConsent ends up calling setTrackingAllowedCookie. Is there a reason we need to test that in addition to testing the effect as in the below test (that we have HT-tracking-cookie-consent=true)? To me it seems like setTrackingAllowedCookie is an implementation detail we don't necessarily need to test.

It does seem like I was trying to do that! But I agree, I probably don't need to test it.

aelkiss · 2024-05-01T19:10:53Z

src/js/lib/cookies.test.js

+    })
+
+})
+describe('setTrackingAllowedCookie', () => {


It looks like although we export this function and its ilk, it's only used in cookies.js. I wonder if it would make sense to only test the functions we expect to use elsewhere like setSelectedConsent, allowAll, allowSelected, denyAll...

That is a much better strategy than what I'm doing 😅

aelkiss · 2024-05-01T19:16:12Z

vite.config.js

@@ -73,4 +73,10 @@ export default defineConfig({
      scss: scssOptions,
    },
  },
+  test: {
+    // coverage: {


Is this left over from something else, or showing how to enable coverage? (Guessing the former, since package.json already has a configured coverage command?)

Yes, I originally had coverage running with every test, but it was pretty annoying, so I moved the coverage part of the tests to its own command. I can remove this from the config.

aelkiss · 2024-05-01T19:32:13Z

src/js/lib/analytics.test.js

I'm not sure I fully understand what you're trying to test, but it sounds like you want to test the underlying Svelte store variable separately from how you set/access it in cookies.js? Or perhaps that the store variable is set up correctly such that it's working the way you expect?

Ultimately, I'd be inclined not to try to test the store variable itself but rather test the observed effect - that the things that are subscribed to it for updates are actually receiving the updates. A couple options I can think of there:

in the test, subscribe to the store variable, set the store variable, and then test that the callback from your subscription is reached.

the other option would be testing the effect of the existing callbacks -- more of an integration test -- but I'm not sure what _paq.push(['setCookieConsentGiven']); (maybe this hooks into what the matomo js itself does?).

aelkiss · 2024-05-02T16:17:19Z

src/js/lib/analytics.test.js

+        test('body element should have apps class', () => {
+            expect(document.body.classList).toContain('apps')
+        })
+        it('should configure analytics', () => {


I think it makes sense to test this so far as we are making sure that we're meeting the expectations for the matomo script.

carylwyatt · 2024-05-06T18:59:20Z

Will need to revisit the coverage/coveralls action in the future, but the tests are being run on PR/merge with main.

carylwyatt added 3 commits April 17, 2024 16:18

add tests for cookie functions

ca25574

more tests

fff740b

fix busted tests

18de807

carylwyatt added 3 commits April 18, 2024 16:15

more tests

bec56b9

add testing for analytics config

2ad7cb5

update

face0d3

aelkiss self-requested a review April 29, 2024 14:12

carylwyatt marked this pull request as ready for review April 29, 2024 14:17

carylwyatt commented Apr 29, 2024

View reviewed changes

aelkiss reviewed May 1, 2024

View reviewed changes

aelkiss reviewed May 2, 2024

View reviewed changes

carylwyatt added 12 commits May 3, 2024 10:42

update tests

97fb123

remove tests

53cba42

add one test

d4c8e97

clean up

eceb32d

add github action

a4aa748

fix action

9a20294

add coveralls actions

8da7eb4

comment out coveralls

7258f53

configure vite for coveralls

edb5730

update coverage file

f60c5a2

fix path

a47ffb7

test

cef1bcb

carylwyatt merged commit 958b4c7 into main May 6, 2024
8 of 10 checks passed

carylwyatt deleted the cookie-tests branch September 6, 2024 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cookie tests #67

Cookie tests #67

carylwyatt commented Apr 18, 2024

netlify bot commented Apr 18, 2024 •

edited

Loading

carylwyatt Apr 29, 2024

aelkiss May 1, 2024

aelkiss left a comment

aelkiss May 1, 2024

aelkiss May 1, 2024

carylwyatt May 2, 2024

aelkiss May 2, 2024

aelkiss May 1, 2024

aelkiss May 1, 2024

carylwyatt May 2, 2024

aelkiss May 1, 2024

carylwyatt May 2, 2024

aelkiss May 1, 2024

carylwyatt May 2, 2024

aelkiss May 1, 2024

carylwyatt May 2, 2024

aelkiss May 1, 2024

carylwyatt May 2, 2024

aelkiss May 1, 2024

aelkiss May 2, 2024

carylwyatt commented May 6, 2024

Cookie tests #67

Cookie tests #67

Conversation

carylwyatt commented Apr 18, 2024

netlify bot commented Apr 18, 2024 • edited Loading

✅ Deploy Preview for hathitrust-firebird-common ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aelkiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carylwyatt commented May 6, 2024

netlify bot commented Apr 18, 2024 •

edited

Loading