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

Use default value for rpcCachePath when running solidity tests #6464

Open
wants to merge 2 commits into
base: v-next
Choose a base branch
from

Conversation

antico5
Copy link
Contributor

@antico5 antico5 commented Mar 11, 2025

config.solidityTest.rpcCachePath defaults to edr-cache, which is the directory that edr uses when running a node in fork mode.

The motivation for this is that by default, running solidity tests with fork were not using any cache, taking a long time on each run

Closes #6459

@antico5 antico5 requested a review from galargh March 11, 2025 18:27
Copy link

vercel bot commented Mar 11, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hardhat ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 12, 2025 5:59pm

Copy link

changeset-bot bot commented Mar 11, 2025

🦋 Changeset detected

Latest commit: 9f93405

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
hardhat Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -121,6 +121,11 @@ export async function resolveSolidityTestUserConfig(
testsPath = typeof testsPath === "object" ? testsPath.solidity : testsPath;
testsPath ??= "test";

const solidityTest = {
rpcCachePath: "edr-cache",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we be using a subdirectory under our global hardhat cache directory?

Do you also happen to know why "there are no cache cleanup implications."?

I'm a little confused as to who owns/is responsible for this cache dir. Is it hardhat or is it edr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may be missing some context, but what I understood was to use the cache directory that edr is using. It's hardcoded in this line, and it seems that it can't be changed.

I don't know why we should or should not be cleaning this cache directory. Maybe @kanej or @alcuadrado can validate?

Copy link
Member

Choose a reason for hiding this comment

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

No, let's use the local cache. We can explore later if using the global one in some situations makes sense. But the global one is more opaque.

Copy link
Member

@alcuadrado alcuadrado Mar 12, 2025

Choose a reason for hiding this comment

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

cleaning is done by the user with hardhat clean. That cache is never automatically cleaned.

This is how HH2 works, and we never received complains about it.

Copy link
Member

@galargh galargh Mar 12, 2025

Choose a reason for hiding this comment

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

Ah, sorry, I meant - shouldn't we use hre.config.paths.cache instead of the edr-cache?

I saw the other comment

@@ -121,6 +121,11 @@ export async function resolveSolidityTestUserConfig(
testsPath = typeof testsPath === "object" ? testsPath.solidity : testsPath;
testsPath ??= "test";

const solidityTest = {
rpcCachePath: "edr-cache",
Copy link
Member

Choose a reason for hiding this comment

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

This should be inside config.paths.cache. A subfolder I mean.

There should be a single cache directory in the root of the project.

Copy link
Member

@alcuadrado alcuadrado left a comment

Choose a reason for hiding this comment

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

Left some comments re the location of the cache

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

4 participants