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

feat: optionally auto-install fuelup during create fuels routine #2149

Merged
merged 29 commits into from May 6, 2024

Conversation

Dhaiwat10
Copy link
Member

@Dhaiwat10 Dhaiwat10 commented Apr 23, 2024

Summary

This PR adds a new feature to the create-fuels CLI tool which detects if the user has already installed fuelup or not. If the user does not have fuelup installed, it gives the users the choice to let the tool install fuelup for them.

Closes #2099

@Dhaiwat10 Dhaiwat10 self-assigned this Apr 23, 2024
@Dhaiwat10 Dhaiwat10 added the feat label Apr 23, 2024
Copy link
Contributor

github-actions bot commented Apr 23, 2024

This PR is published in NPM with version 0.0.0-pr-2149-20240427110619

.github/workflows/pr-release.yaml Outdated Show resolved Hide resolved
packages/create-fuels/package.json Outdated Show resolved Hide resolved
packages/create-fuels/src/cli.ts Outdated Show resolved Hide resolved
nedsalk
nedsalk previously approved these changes Apr 24, 2024
Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

Nice job! I think we want to guide users a bit more on the process though

packages/create-fuels/src/cli.ts Outdated Show resolved Hide resolved
packages/create-fuels/src/cli.ts Outdated Show resolved Hide resolved
@Dhaiwat10 Dhaiwat10 requested a review from maschad April 25, 2024 09:16
Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

Great work!

Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

Should we add some tests?

@Dhaiwat10
Copy link
Member Author

@arboleya That wouldn't be a bad idea. What should we test? That the curl command is being called?

@nedsalk
Copy link
Contributor

nedsalk commented May 1, 2024

@Dhaiwat10 You can mock execSync and have two tests:

  1. Have execSync('fuelup --version') fail, verify that it was called later on with curl https://install.fuel.network | sh
  2. Have execSync('fuelup --version') succeed, verify that it wasn't called later on with curl https://install.fuel.network | sh

You'll also have to mock execSync('curl https://install.fuel.network | sh') to do nothing so that the tests don't run fuelup installs all the time.

@arboleya
Copy link
Member

arboleya commented May 1, 2024

@nedsalk Exactly. 👌

@Dhaiwat10 The idea is to validate that conditionals, prompts, and warnings/errors are all working correctly. The coverage report can help you understand the parts of the code that aren't under test and target those.

You might have to mock promptForFuelUpInstall, tryInstallFuelup, etc.

As integration tests may not fit here, you could also unit-test functions in isolation.

@Dhaiwat10
Copy link
Member Author

I think we are safe to merge this one since @petertonysmith94 kindly went ahead and added a comprehensive suite of tests for the tool when I was away last week: #2237

@Dhaiwat10 Dhaiwat10 enabled auto-merge (squash) May 6, 2024 14:22
Copy link
Contributor

github-actions bot commented May 6, 2024

Coverage Report:

Lines Branches Functions Statements
79.28%(-0.25%) 69.05%(-0.23%) 77.19%(-0.24%) 79.4%(-0.25%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/create-fuels/src/cli.ts 71.81%
(-9.44%)
52.17%
(-6.8%)
57.89%
(-10.86%)
72.56%
(-9.25%)
🔴 ✨ packages/create-fuels/src/lib.ts 15.38%
(+15.38%)
0%
(+0%)
0%
(+0%)
15.38%
(+15.38%)

@Dhaiwat10 Dhaiwat10 merged commit 7c3ef04 into master May 6, 2024
19 checks passed
@Dhaiwat10 Dhaiwat10 deleted the dp/install-fuelup branch May 6, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider auto-installing fuelup during create fuels routine
6 participants