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.

Sign up for GitHub

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

Use Vitest to enable browser testing #198

Closed
wants to merge 3 commits into from
Closed

Use Vitest to enable browser testing #198

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b0fe5ba
Use Vitest to enable browser testing
jonkoops Aug 17, 2023
10ed646
Use `-browsers` image for CircleCI
jonkoops Aug 17, 2023
b41e008
Remove Firefox from testing matrix
jonkoops Aug 17, 2023
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
11 changes: 7 additions & 4 deletions .circleci/config.yml
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if the changes to the CI here will pass, as I have not been able to run this myself on Circle CI. Works on my machine locally though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the tests will pass if we remove Firefox from the testing matrix. It looks like the images on CircleCI don't have the correct binaries for it 😞

Copy link
Member

@frederikprijck frederikprijck Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We typically use browserstack to test in browsers, to avoid having to set up such images on our CI and to have more flexibility with browsers to be tested.

We typically use unit tests in jest, and end-2-end tests that run in browserstack that test the functionality in multiple browsers. This repository doesnt do that (yet) because:

  • Our team only recently started taking over maintenance
  • It's pretty small and stable, and proven to work (and we have no plans to change implementations).

I'd rather move this to use browserstack and verify it can decode a token succesfully in all browsers through e2e tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would mean we are essentially duplicating the test suite though. One test suite for Node.js (Jest), and another for browsers (Browserstack). End-to-end testing is also, less appropriate here IMHO. Considering that this is a rather small library, and not an application.

Copy link
Member

@frederikprijck frederikprijck Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That would mean we are essentially duplicating the test suite though

I don't think we should duplicate the entire test suite. We are happy with verifying everything in JSDom and Node using unit tests, and verifying the happy path in actual browsers through actual DOM interactions (paste a token in an input, hit a button, print it, and make some assertions). This has proven to work for all of our SDKs, and it should also work for jwt-decode.

It would mean it aligns with what we use in all of our, non-application, libraries (we don't build apps, yet we use browserstack for all to verify they work in different browsers). And it also allows to offload the browsers to browserstack, instead on spinning them up on Circle CI (or GitHub Actions in the near future).

I am happy to reconsider vitest in the future for our team, but at least its browser mode should be non-expiremental before we start considering moving to it. I don't think we should throw experimental things in when we already use non-experimental tools that solve the same thing. It even looks like a non-experimental browser mode is not on the readmap for Vitest 1.0.

Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,14 @@ jobs:
name: Build
command: npm run build
- run:
name: Run Tests against browser
command: npm run test:browser
name: Run tests against Node.js
command: npm run test
- run:
name: Run Tests against node
command: npm run test:node
name: Run tests against Chrome
command: npm run test -- --browser=chrome
- run:
name: Run tests against Firefox
command: npm run test -- --browser=firefox
workflows:
build-and-test:
jobs:
Expand Down
18 changes: 0 additions & 18 deletions jest.config.ts
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions test/tests.ts → lib/index.test.ts
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since Vitest is Jest compatible (for the most part), we can just change the imports and it all magically works 🎉

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { jwtDecode, InvalidTokenError, JwtPayload } from "./../lib/index.js";
import { describe, expect, it } from "@jest/globals";
import { describe, expect, it } from "vitest";
import { InvalidTokenError, JwtPayload, jwtDecode } from "./index.js";

const token =
"eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJmb28iOiJiYXIiLCJleHAiOjEzOTMyODY4OTMsImlhdCI6MTM5MzI2ODg5M30.4-iaDojEVl0pJQMjrbM1EzUIfAZgsbK_kgnVyVxFSVo";
Expand Down
Loading