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

Include hardhat tests #408

Merged
merged 20 commits into from
Apr 26, 2024
Merged

Include hardhat tests #408

merged 20 commits into from
Apr 26, 2024

Conversation

fvictorio
Copy link
Member

@fvictorio fvictorio commented Apr 25, 2024

This PR copies a subset of the hardhat-core tests into our repo and runs them using the workspace version of EDR. Reviewing the PR is of course impossible, so I'll just detail what I did; please tell me if you think I missed something.

Copying the code

This was the easy part. I copied hardhat-core as hardhat-tests, removed a lot of files (mainly src/, but also sample-projects/, scripts/, CHANGELOG.md, etc.). I also wrote a short README to explain what this directory is about.

Running with Hardhat as a dependency

After removing the src directory the tests of course stopped working. To make them work again I installed hardhat as a dev dependency, and then transformed all the relative imports like import "../../../src/foo/bar.ts into import "hardhat/foo/bar.ts".

After doing this, I could run the tests again. Most of them passed; the ones that didn't were assuming some things about the directory structure that was no longer true. But all of these were also tests that didn't use EDR, so I removed them.

Using the workspace version of EDR

Luckily, doing this was super straightforward. I just added this to the root's package.json:

  "pnpm": {
    "overrides": {
      "hardhat@2>@nomicfoundation/edr": "workspace:*"
    }
  }

Something I don't like about this is that this can't be done just for the hardhat-tests workspace. This seems to be a pnpm limitation. But I think it's highly unlikely that we 1) add another workspace that uses hardhat as a dependency and 2) that that workspace needs the npm version of EDR instead of the local one. So I think this risk is fine.

To verify that this was indeed using the local version, I added an assert!(false) to the Provider::with_config method in edr_napi, which is the entry point used by Hardhat, and ran the tests. They panicked, as expected.

The only remaining problem was that this could stop working for some reason in the future and we wouldn't notice it because the npm version of EDR would be used and things would be fine anyway. To prevent this, I added a test in hardhat-tests/test/edr-override.ts that checks that the local version is used. Please check it to see what it does.

Removing unnecessary tests

Finally, I wanted to remove all the tests that weren't related to the Hardhat network. Most of them were the tests outside test/internal/hardhat-network, as expected. But to check that this is the case, before removing each of them I ran them with a local version of EDR that panics and checked that they passed. Then I did the same thing with the tests I didn't remove: I ran each of them separately and checked that they panicked.

Workflow

I added a .github/workflows/hardhat-tests.yml workflow. Nothing fancy here: it runs the lint and test scripts of hardhat-tests and caches the remote RPC calls and the stack traces artifacts.

Copy link

changeset-bot bot commented Apr 25, 2024

⚠️ No Changeset found

Latest commit: 483d2ad

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@fvictorio fvictorio added the no changeset needed This PR doesn't require a changeset label Apr 25, 2024
@Wodann
Copy link
Member

Wodann commented Apr 25, 2024

Is there an easy way to synchronise between Hardhat and EDR; e.g. using git subtree?

Copy link
Member

@Wodann Wodann left a comment

Choose a reason for hiding this comment

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

Thanks for the write-up. Very helpful!

I posted one question, but otherwise LGTM once CI is successful

@fvictorio
Copy link
Member Author

Is there an easy way to synchronise between Hardhat and EDR; e.g. using git subtree?

I'm not sure, but we are not going to keep them synchronized anyway. The way to think about it is that these tests are our responsibility now, and they are (eventually) going to be removed from the Hardhat repo.

@fvictorio fvictorio merged commit 47ed058 into main Apr 26, 2024
25 checks passed
@fvictorio fvictorio deleted the include-hardhat-tests branch April 26, 2024 08:11
@Wodann Wodann added this to the EDR v0.3.6 milestone Apr 29, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
no changeset needed This PR doesn't require a changeset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants