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

chore: migrate from Jest to Vitest for ESM support #1663

Merged
merged 20 commits into from
Sep 7, 2024
Merged

Conversation

ghiscoding
Copy link
Owner

@ghiscoding ghiscoding commented Sep 5, 2024

  • migrating to Vitest so that we can eventually drop ESM builds and this change is required since Jest only supports CJS by default (their ESM support is very experimental and hard to use, so switching to Vitest is in many ways easier).
  • another import thing to note is that recommended v8-to-istanbul reporter is acting differently and line counts is different, for example the coverage with Istanbul is for an if and its content but doesn't include the termination } (however it's included with v8) and so for that reason the new codecov report shows nearly 10k more lines

Copy link

stackblitz bot commented Sep 5, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

codecov bot commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.7%. Comparing base (f6e4605) to head (ebcf4e8).
Report is 21 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1663     +/-   ##
========================================
- Coverage    99.7%   99.7%   -0.0%     
========================================
  Files         199     187     -12     
  Lines       21895   31085   +9190     
  Branches     7319    9782   +2463     
========================================
+ Hits        21827   30987   +9160     
- Misses         68      98     +30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ghiscoding ghiscoding marked this pull request as draft September 5, 2024 19:07
@ghiscoding ghiscoding marked this pull request as ready for review September 6, 2024 02:24
@ghiscoding
Copy link
Owner Author

ghiscoding commented Sep 6, 2024

@zewa666
I'm curious, are you using JSDOM or happy-dom with Vitest? My current setup is a mix of both happy-dom and JSDOM, so my Vitest config currently has environment: 'happy-dom' and I also need jsdom-global. There currently 2 problems that I have

  1. if I change to environment: 'jsdom', it behaves weirdly, it sometimes execute the unit tests twice and sometimes doesn't complete
  2. if I try to remove all JSDOM related, just jsdom-globals now, it fails 31 of my test suites, I will probably have to rewrite a few unit tests since I assume happy-dom might be different compared to jsdom
    • I found @happy-dom/global-registrator which is maybe what I need to replace jsdom-globals. similar to this post, will try tonight after work

So it's a little weird, I have to go with a mix of both, I'll take another look at it tonight but so far the testing only works with the mix config... so are you using happy-dom and/or jsdom?

EDIT

as per this open issue but it looks like it's a known problem with Vite and happy-dom was suggested as an alternative. However I find happy-dom is still kind of incomplete (for example, DOM elements don't include aria labels and others, a couple more issues too)... so if the mix of both works, let's keep that and merge baby merge (1 step closer to ESM only)

@ghiscoding ghiscoding merged commit d8403ea into master Sep 7, 2024
8 checks passed
@ghiscoding ghiscoding deleted the chore/vitest branch September 7, 2024 03:13
@zewa666
Copy link
Contributor

zewa666 commented Sep 7, 2024

I did not have that issue. besides are you sure jsdom is the problem since you already had it working with jest?

I personally would stay away from happydom due to even less coverage than jsdom, which shows itself especially with testing-library. but if it works for the slickgrid lib itself ... well why not ;)

@ghiscoding
Copy link
Owner Author

ghiscoding commented Sep 7, 2024

I wouldn't mind using strictly JSDOM but like I said above when I set it as the environment in Vitest, then some tests aren't passing but when I set it to happy-dom and still use jsdom-global then all is good... It's kinda weird but like you said, if it works then let's keep that setup and forget about it :)

import 'jsdom-global/register';
import * as matchers from 'jest-extended';

@zewa666
Copy link
Contributor

zewa666 commented Sep 7, 2024

would you mind creating a branch where only jsdom is acitvated? I'd really like to give it a closer look as it might also show some potential blockers for my own app

@ghiscoding
Copy link
Owner Author

ghiscoding commented Sep 7, 2024

it's just this line to change from happy-dom to jsdom

- environment: 'happy-dom',
+ environment: 'jsdom',

export default defineConfig({
test: {
// clearMocks: true,
deps: {
interopDefault: false,
},
environment: 'happy-dom',

@zewa666
Copy link
Contributor

zewa666 commented Sep 7, 2024

oh ok thought you mentioned adding additional stuff for happydom. I'll give it a try

@zewa666
Copy link
Contributor

zewa666 commented Sep 9, 2024

@ghiscoding so I've tried switching to jsdom and all tests are passing nicely for me. I dunno, perhaps try to nuke the node_modules folder and a fresh install could be the difference?

@ghiscoding
Copy link
Owner Author

ghiscoding commented Sep 9, 2024

@zewa666 hmm that's strange because I tried on work laptop today and same results, I have 202 tests failing with this error

'removeEventListener' called on an object that is not a valid instance of EventTarget.

I didn't think of nuking the node_modules, so I just tried it and still the same result on my side. I think the problem is that I use Custom Events in the project and JSDOM doesn't work well with EventTarget properly with Custom Events because of this JDOM issue, I tried to mock it via this post from the same issue but that doesn't help. So I think that when I set happy-dom, its EventTarget mock is better and so when I mix them both then it passes.

image

it's also a lot quicker when it passes

85sec vs 171sec

image

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