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

test: migrate to vitest #349

Merged
merged 6 commits into from
Jul 16, 2024
Merged

test: migrate to vitest #349

merged 6 commits into from
Jul 16, 2024

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Jan 5, 2024

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

Ref #229 (comment)

@aduh95
Copy link
Contributor

aduh95 commented Jan 6, 2024

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
Copy link
Member Author

merceyz commented Jan 6, 2024

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
Copy link
Contributor

aduh95 commented Jan 6, 2024

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
Copy link
Member Author

merceyz commented Jan 6, 2024

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.

Copy link
Member Author

@merceyz merceyz Jan 6, 2024

Choose a reason for hiding this comment

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

Tests fail with a module not found error without this patch.

@arcanis Clipanion could probably use the imports field for this.

Copy link
Member Author

@merceyz merceyz Jan 7, 2024

Choose a reason for hiding this comment

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

PR fixing this issue: arcanis/clipanion#155

.gitignore Show resolved Hide resolved
@merceyz
Copy link
Member Author

merceyz commented Jan 20, 2024

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

I tested using node:test (#357) but didn't finish it as, in my opinion, this PR provides a better experience.

@merceyz merceyz reopened this Jan 21, 2024
@merceyz merceyz marked this pull request as ready for review March 8, 2024 19:41
@wojtekmaj
Copy link
Contributor

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.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Can you please fix the conflicts?

package.json Outdated Show resolved Hide resolved
@aduh95 aduh95 merged commit 85e556a into nodejs:main Jul 16, 2024
13 checks passed
@merceyz merceyz deleted the merceyz/test/vitest branch July 16, 2024 21:45
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.

3 participants