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

fix: Add better version detection #125

Merged
merged 2 commits into from
Feb 8, 2023

Conversation

ngirardin
Copy link
Contributor

@ngirardin ngirardin commented Jan 22, 2023

This is a follow-up PR for #118 which have broken some installs:

It seems to fix:

and probably:

In some cases, vitest need to be started using spawn(vitestCommand.cmd, [...vitestCommand.args, '-v'], envs)), and in other by using spawn(process.execPath, [vitestCommand.cmd, ...vitestCommand.args, '-v'], envs)).

There are lot of OS / Node install combinations, and I don't have a clear picture of which to use and when.

The easiest fix that I could find, was to try both commands and see which one worked.

It's not the cleanest solution, but I'm open to suggestions :)

@ngirardin ngirardin changed the title Add better version detection fix: Add better version detection Jan 22, 2023
@ngirardin ngirardin marked this pull request as ready for review January 23, 2023 19:50
@ngirardin
Copy link
Contributor Author

@zxch3n any thoughts on the fix? (I can't add you as a reviewer)

Copy link
Member

@zxch3n zxch3n left a comment

Choose a reason for hiding this comment

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

This fix should use try and catch to avoid throwing in each step, right?

src/pure/utils.ts Outdated Show resolved Hide resolved
src/pure/utils.ts Outdated Show resolved Hide resolved
@arimgibson
Copy link

Hey @ngirardin , let me know if you need help making those suggestions from @zxch3n ; happy to help move this forward if there's anything I can do!

@ngirardin
Copy link
Contributor Author

Thanks @zxch3n for the review.

I've made some changes to the code:

  • log the commands that are spawned
  • added a message to invite users to open an issue and provide their logs if no version is detected
  • trying both of the spawn methods for starting npx (when command == null)
  • added some tests (WIP needs to mock spawn to test all the cases)
  • added a check when doing the line.match to avoid crashing if the version is not found in the output

@arimgibson, sorry for the delay, please let me know if you see something to change on this code :)

@zxch3n zxch3n merged commit c43516f into vitest-dev:main Feb 8, 2023
@zxch3n zxch3n mentioned this pull request Feb 9, 2023
@ngirardin ngirardin deleted the better-version-detection branch February 9, 2023 08:30
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