feat: solidity test runner config

[agostbiro]

Config Options

This PR exposes Solidity test runner config options to TypeScript from Rust through NAPI-RS.

The basis of the config options are the Foundry testing configuration options documented here. There were some additional config options not covered there that are needed to construct a test runner. These are exposed based on the Foundry config struct that contains all configuration options (including build and deployment options which we don't need).

The following Foundry testing options are not exposed:

• verbosity
  • This controls the CLI output verbosity which is out of scope for EDR.
• etherscan_api_key
  • This is used for verifying deployed contracts and identifying unknown contracts in traces in fork mode. Both are out of scope for the EDR Solidity tests effort.
• threads
  • Number of logical cores to use for parallel test execution.
  • Not yet supported by forked Foundry version.
• show_progress
  • Whether to show test execution progress.
  • Not yet supported by forked Foundry version and probably out of scope for EDR.
• names
  • Print compiled contract names.
  • Build-related, out of scope for EDR.
• sizes
  • Print compiled contract sizes.
  • Build-related, out of scope for EDR.
• gas_reports
  • Specifies the contracts to print gas reports for.
  • Displaying info to the user is Hardhat’s responsibility, so this is out of scope for EDR.
  • How to expose data for gas reporting is a separate issue
• match-test, no-match-test, match-contract, no-match-contract, match-path, no-match-path
  • Only run test methods matching the above specifiers
  • Selecting which contracts to run is the responsibility of Hardhat, so these config options are out of scope for EDR.

Error Callback

In addition to the config options, this PR brings another change to the Solidity test runner interface: an error callback.

The reason for introducing this error callback is that building a test runner is async now to resolve forked EVM parameters, but the JS interface is sync and returns results with a progress callback. If we want to be able to propagate errors encountered while building a test runner we have two options:

1. Make the test runner async. This is possible, but we'd need to create a test runner class that takes the progress callback as argument first and then give it an async method for reasons explained in refactor: Solidity test runner progress callback completion #563.
2. Expose an error callback.

The second option is simpler and composes well with the Promise wrapper on the TS-side (just have to pass the reject arg of the promise as the error callback), so I went with this option.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 5f093f5

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]

Either is a workaround for lack of enums with fields in JS

[agostbiro]

The test failures are unrelated and fixed in #598

[Wodann]

Overall looks good.

I primarily would like to better understand why we cannot use an async fn run_solidity_tests for calling an async constructor, as this pattern is used in our other N-API bindings.

[fvictorio]

+1 to renaming the function to spawnSolidityTestRunner btw. I was thinking "startSolidityTestRunner", but I think I like "spawn" more.

[agostbiro]

+1 to renaming the function to spawnSolidityTestRunner btw. I was thinking "startSolidityTestRunner", but I think I like "spawn" more.

Ok, I'll do that in a follow up PR.

[agostbiro]

Actually on a second thought, spawnSolidityTestRunner is kinda misleading, because I'd expect based on this name that I spawn something and then I make requests to it to execute tests. I don't think it makes sense to spend more time on this now, and we'll want to do an interface review anyway with the Hardhat team, so I think we can come up with a better name then.

[fvictorio]

Sure, I'm ok with postponing that discussion!

[Wodann]

I proposed a solution to the mentioned async problem with N-API here. I'm not sure whether this also circumvents the original issue that we had (about a month ago) relating to process.exit().

I had a discussion with @fvictorio about Javascript's best practices around callbacks. If an API is explicitly designed to receive callbacks to track its result, it should only use callbacks. No promises.

In that case, could you maybe just rename the errorCallback to indicate that it will not be called for all errors but just for the spawning error?

I'm a proponent of renaming the runSolidityTests function to spawnSolidityTestRunner to make that clearer as well.

edited based on discussion with Franco

[agostbiro]

In that case, could you maybe just rename the errorCallback to indicate that it will not be called for all errors but just for the spawning error?

The doc says explicitly: "The error callback is called if an invalid configuration value is provided."