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

Use Vitest to enable browser testing #198

Closed
wants to merge 3 commits into from

Conversation

jonkoops
Copy link
Contributor

This PR replaces Jest with Vitest to enable testing in both browsers, and in Node.js. Since Jest does not allow testing in actual browsers, but only synthetic ones trough JSDOM.

Vitest has a couple of advantages over Jest:

  1. Testing against browsers, and Node.js (this is what we care about most).
  2. Built in support for TypeScript.
  3. Jest Compatible (no need to change the tests).

@jonkoops jonkoops requested a review from a team as a code owner August 17, 2023 16:50
@jonkoops
Copy link
Contributor Author

@frederikprijck I know you have stated that you'd prefer to use Jest, as it is the default choice for testing within Auth0. However, considering Jest does not allow testing in browsers, I think it might warrant an exception for this package. WDYT?

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

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

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.

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 🎉

@frederikprijck
Copy link
Member

frederikprijck commented Aug 17, 2023

Can you elaborate what you mean with warranting for this SDK? We have alot more browser based JavaScript SDKs, who have a lot more browser dependend code, i would argue it would make more sense there.

We aren't married to jest, but i introduced jest for consistency and not sure this library requires vitest.

Not saying we cant use vitest, just not sure we should, jest hasnt caused issues for us and the consistency simplifies maintenance for us, even our vue repo uses jest for that reason (https://github.com/auth0/auth0-vue/blob/main/package.json#L25)

@jonkoops
Copy link
Contributor Author

jonkoops commented Aug 17, 2023

Can you elaborate what you mean with warranting for this SDK?

Well, as you mentioned before, this library is mostly intended for browser applications (says so right in the tagline as well). Right now, it is however not being tested against any real world browsers. Rather, it's being tested in Node.js, and with JSDOM, which attempts to simulate a browser-like environment (but still not a real browser).

So mostly, the motivation here is to have things tested in real browsers (and also Node.js), so the results are more accurate of real-world usage.

We have alot more browser based JavaScript SDKs, who have a lot more browser dependend code, i would argue it would make more sense there.

I think it makes sense for both, getting as close as possible to having the test environment match what users are running is always beneficial in terms of accuracy.

@jonkoops
Copy link
Contributor Author

Side-note. In terms of maintenance, I think running the tests in browsers would actually allow us to get rid of the watch mode for the build + serving debug HTML. Reason being, that we can now test things in the browser through Vite, which allows interactive debugging there.

@frederikprijck
Copy link
Member

frederikprijck commented Aug 18, 2023

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 in our team that solve the same thing (as mentioned, we can always use browserstack for now). It even looks like a non-experimental browser mode is not on the readmap for Vitest 1.0.

Regardless, appreciate the effort and I do like Vitests (and Vite) myself (and some others in our team also prefer it over jest). But also keep in mind the intention of this major was not to reconsider all of our tools, so I try to be mindful of that as well.

@jonkoops jonkoops deleted the browser-testing branch August 18, 2023 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants