Add js integration test for solidity test runner

[fvictorio]

This PR is mainly about adding a minimal typescript integration test for the solidity test runner, but that triggered an avalanche of changes. I'll explain them (mostly) in causality order.

edr-helpers

The integration test has to use the runSolidityTests function, which has a low-level, callback-based interface. It's easy to create a wrapper around it, but we are already doing things like that in other parts of the codebase (the benchmarks, the v2 plugin). Similarly, building the input for that function from Hardhat's compilation output is done in a similar way in multiple places.

To deduplicate that code, I created an internal @nomicfoundation/edr-helpers package (the name doesn't really matter because it won't be published) under js/helpers. This has two functions: runAllSolidityTests, which returns a promise with all the test results (but also accepts a progress callback), and buildSolidityTestsInput, which builds the input and has an optional predicate callback to figure out which artifacts correspond to test contracts.

These helpers are now used in the benchmark package, in the Hardhat v2 plugin, and in the integration test that this PR adds.

The tests in edr_napi still have a somewhat duplicated logic. The reason for this is that the helpers depend on that package, so using the helpers there would create a circular dependency. Circular dependencies are, I think, doable in this case, but I got a problem, didn't look into it, and just duplicated the function. I also think that it makes sense for these tests to use the function more directly. You could argue that the proper solution is for the helpers to live under edr_napi, but this opens a different can of worms, and I don't love the idea of adding public API for internal reasons.

Root build script

EDIT: outdated section. Some of this description is still valid, but see the rest of the discussion below to see what changed.

Adding the helpers and using them in other packages means that the helpers need to be compiled before they are used. (I think it's technically possible to import typescript files directly, since they are not going to be used by javascript code anyway, but to be honest I don't know how to properly do that.)

I could've just expanded every relevant prebuild script (or whatever) to cd into the helpers directory and build them, but that seemed too much work and brittle. Instead, I added a build script in the root package.json which runs the build scripts in all the packages. This is done by pnpm in topological order, meaning that the dependencies are built before the dependents. I then modified pre- scripts like pretest and prebenchmark to just cd into the root and build everything. And I made sure that all the compilations are done incrementally, so that all this building is not unnecessarily slow. This is the same approach we had in Hardhat; I think it works well enough.

One caveat here is that hardhat-tests needs edr_napi to be built because of the pnpm override, but it's not an explicit dependency so there are no guarantees that it will be built before it. To make sure that was the case, I added it as an explicit dependency, even if it's not really used by the tests code.

Another impact of this change is that I changed the build scripts in edr_napi. The build script was the slow one used before publishing, and that didn't play well with this approach. So I added a build:publish script, used it in the release workflows, and changed build to be the normal one used for development. This is the riskiest change in the PR, please pay special attention to it.

Other changes

Some other related changes:

• I added a step to the linting CI job to run the root build script. This is perhaps redundant, but it doesn't hurt.
• The benchmark workflows had some unnecessary usages of the --silent flag. That was a problem when trying to figure out why something wasn't working. I removed the ones that were not necessary, and replaced -s with the longer --silent because I had no idea what -s meant.

Javascript packages are all over the place

We now have these javascript packages:

• hardhat-tests
• hardhat-solidity-tests
• crates/tools/js/benchmark
• js/helpers and js/integration-tests/*

This is not great. I think we should move all of them under the root's js directory that this PR adds. But we should wait until we don't have any major divergent feature branches, otherwise moving that much code around can cause some annoying merge conflicts.

[changeset-bot]

⚠️ No Changeset found

Latest commit: d725d4f

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

[agostbiro]

Thanks for the PR and the detailed write up! I've tried the tests and LGTM, except for changing the default build behavior in edr_napi:

Another impact of this change is that I changed the build scripts in edr_napi. The build script was the slow one used before publishing, and that didn't play well with this approach. So I added a build:publish script, used it in the release workflows, and changed build to be the normal one used for development. This is the riskiest change in the PR, please pay special attention to it.

I feel this is too risky. It's an easy mistake to use build instead of build:publish. This would lead to releasing a performance regression and we'd most likely not detect it until users complained. So I'd prefer to keep the default behavior as publish unless there is a way to check somehow before releasing that the artifacts were built with the publish profile, but I'm not sure how. Wdyt?

[Wodann]

One question, but otherwise LGTM!

I don't have the same concern about using a non-default, dedicated target/script for publishable builds. This is a common pattern in the industry: making the default development workflow the standard and using an alternative target for CI/CD scripts when publishing.

[Wodann]

Why do we need to build twice?

[fvictorio]

Because the build script builds with a non-publish profile, but we want to benchmark with the super-optimized profile.

[agostbiro]

I don't have the same concern about using a non-default, dedicated target/script for publishable builds. This is a common pattern in the industry: making the default development workflow the standard and using an alternative target for CI/CD scripts when publishing.

Counterpoint: in the napi-rs project template, the build command builds the published version.

[Wodann]

Counterpoint: in the napi-rs project template, the build command builds the published version.

To clarify, I'm not stating that no one uses the default build target for publishing. I'm just saying that it's not uncommon for projects to have a non-default publish/final build profile that does additional optimisations and/or build steps for publishing. As such, in my mind, the risk of misconfiguration of CD exists either way. Thus, I don't have the same sense of increased risk one way or another.

[fvictorio]

@Wodann that's a valid point, but I went ahead and reverted the change anyway (c72eec9). The reason is that the negative impact of publishing a non-completely-optimized version would be very high, and so anything that reduces that risk seems worth it (I guess it's debatable whether this change actually reduces risk, but I think it does). The trade-off here is that, during local development, one might accidentally use a publish build, but that's easy to notice because it's so fucking slow.

The proper solution, of course, would be to run the benchmarks before publishing. But that's way harder to do, and we can't do it for all builds.