search

nodejs / corepack

Zero-runtime-dependency package acting as bridge between Node projects and their package managers
MIT License
2.46k stars 160 forks source link

test: migrate to vitest #349

Closed merceyz closed 1 month ago

merceyz commented 8 months ago

Reduces the amount of configuration and dependencies needed to run the tests with almost no changes to the code.

Ref https://github.com/nodejs/corepack/pull/229#discussion_r1064149939

aduh95 commented 8 months ago

I'm -0.5 on this, Vitest shares a lot of the same problems as Jest (you're not testing on Node.js, you're testing in Jest/Vitest runtime), I would rather like to see us move away from this and use node:test or Mocha instead.

merceyz commented 8 months ago

Indeed, nothing changes in that regard here, though to be fair most of the tests spawn Corepack in a node process so most of the work happens outside of the Jest/Vitest runtime.

This PR landing doesn't prevent migrating to node:test in the future, this PR could land and a follow-up PR could migrate to node:test.

aduh95 commented 8 months ago

most of the tests spawn Corepack in a node process so most of the work happens outside of the Jest/Vitest runtime.

Yes that’s true, I did not consider that.

It seems there are some changes which are not directly related to switching to Vitest in this PR, right? Do you know why the nock files need to change?

merceyz commented 8 months ago

Most of them were renamed because of https://github.com/nodejs/corepack/pull/349/files#diff-b017716ca1490209cba877efb506d6b0cd9d724dda50f33a7384a88da852067fR23

- http utils fetchUrlStream rejects with error
+ tests/httpUtils.test.ts > http utils fetchUrlStream > rejects with error

The ones that actually changed deserialises to the same content.

merceyz commented 7 months ago

I would rather like to see us move away from this and use node:test or Mocha instead.

I tested using node:test (https://github.com/nodejs/corepack/pull/357) but didn't finish it as, in my opinion, this PR provides a better experience.

wojtekmaj commented 1 month ago

While concerns about Jest/Vitest shared here may be valid, migration to Vitest will bring a clear improvement of our dev environment, and will enable me to proceed with more modernization actions that I have on my radar.