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

Better error message when URL is an empty string #2730

Closed
fvictorio opened this issue May 18, 2022 · 11 comments · Fixed by #4096
Closed

Better error message when URL is an empty string #2730

fvictorio opened this issue May 18, 2022 · 11 comments · Fixed by #4096
Assignees
Labels
status:ready This issue is ready to be worked on type:improvement

Comments

@fvictorio
Copy link
Member

An empty string in the forking URL or in a network URL produces this error message:

Uncaught TypeError [ERR_INVALID_URL]: Invalid URL:

We should handle this as a special case and print a better error message. Also, this is not caught during config validation; instead, it happens the first time a request is sent.

This comment also seems to suggest that an "only absolute URLs are supported" message can be shown when an empty string is used; maybe that depends on the OS or it happens in other scenarios.

@github-actions
Copy link
Contributor

This issue is also being tracked on Linear.

We use Linear to manage our development process, but we keep the conversations on Github.

LINEAR-ID: 76fbcb73-8707-4c4e-a9b0-d45fce9a2b58

@github-actions
Copy link
Contributor

This issue was marked as stale because it didn't have any activity in the last 30 days. If you think it's still relevant, please leave a comment indicating so. Otherwise, it will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jun 17, 2022
@github-actions
Copy link
Contributor

This issue was closed because it has been stalled for 7 days with no activity.

@fvictorio fvictorio reopened this Jul 6, 2022
@fvictorio fvictorio added not-stale and removed Stale labels Jul 6, 2022
@fvictorio fvictorio removed their assignment Dec 27, 2022
@fvictorio fvictorio added priority:medium type:improvement status:ready This issue is ready to be worked on good-first-issue Good for newcomers. Guidance available if needed and removed priority:medium labels Dec 28, 2022
@ZainGulbaz
Copy link

Hi @fvictorio , Can you please assign this issue to me? Thanks.

@fvictorio
Copy link
Member Author

Done! Let me know if you need help.

@ZainGulbaz
Copy link

Hi @fvictorio , I have made a pull request for it. You can have a look. Thanks.

@ZainGulbaz
Copy link

#3825 better message when url is an empty string.

@kshyun28
Copy link
Contributor

Hello @fvictorio, I'm looking at the list of good first issues and am interested in taking a stab at this issue if its okay.

P.S. your evm-puzzles were really helpful in making me understand evm at a practical level :)

@kshyun28
Copy link
Contributor

kshyun28 commented Jun 30, 2023

I have a question, am trying to run tests in the hardhat-core package. It's requiring me both ALCHEMY_URL and INFURA_URL.
image

I've added a file packages/hardhat-core/.env and populated with the following, but still shows the same error.

INFURA_URL=https://sepolia.infura.io/v3/[REDACTED]
ALCHEMY_URL=https://eth-sepolia.g.alchemy.com/v2/[REDACTED]
  • Where should I be setting the environment variables?
  • And is using Sepolia HTTP url correct, or should I use another network (Mainnet, Goerli, etc.)?

EDIT:

  • Was able to run the tests by exporting variables to my cli export ALCHEMY_URL=...
  • Have used mainnet rpc urls after seeing tests fail on Sepolia rpc. Might be obvious, but wasn't clear to me as a first time potential contributor.

@fvictorio
Copy link
Member Author

Hey @kshyun28, sorry for not responding sooner and thanks for the PR!

Have used mainnet rpc urls after seeing tests fail on Sepolia rpc. Might be obvious, but wasn't clear to me as a first time potential contributor.

That's a great point. Do you want to send a separate PR improving that warning? Ideally we would change the environment variables names too, but that will be a huge change and maybe it's not worth it

your evm-puzzles were really helpful in making me understand evm at a practical level

Super glad to read that 😄

@kshyun28
Copy link
Contributor

kshyun28 commented Jul 7, 2023

That's a great point. Do you want to send a separate PR improving that warning? Ideally we would change the environment variables names too, but that will be a huge change and maybe it's not worth it

I'm okay with sending an additional PR to improve the warning message. Currently I'm thinking TEST RUN INCOMPLETE: You need to define the env variable INFURA_URL to an Ethereum mainnet HTTP provider

I'd also suggest to add a section in CONTRIBUTING.md on configuring environment variables for tests as needed. A note/hint could work, since I only knew that environment variables are needed after attempting to run the test.

Might have to review if steps to configure env variables are the same in windows, but export VARIABLE=value should work on mac/linux.

CONTRIBUTING.md

## Testing 

All tests are written using [mocha](https://mochajs.org) and [chai](https://www.chaijs.com).

### Environment variables

Some tests require having `INFURA_URL` and `ALCHEMY_URL` configured. 
You can set them in your terminal using `export ALCHEMY_URL=value INFURA_URL=value`.

### Per-package

You can run a package's tests by executing `yarn test` inside its folder.

### Entire project

You can run all the tests at once by running `yarn test` from the root folder.

@fvictorio fvictorio assigned fvictorio and unassigned ZainGulbaz Jul 17, 2023
@fvictorio fvictorio removed the good-first-issue Good for newcomers. Guidance available if needed label Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:ready This issue is ready to be worked on type:improvement
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants