refactor: Solidity test runner progress callback completion

[agostbiro]

Problem

The Solidity test runner interface that is exposed from Rust looks like this currently (config options are WIP):

/** Executes solidity tests. */
export class SolidityTestRunner {
  /**Creates a new instance of the SolidityTestRunner. The callback function will be called with suite results as they finish. */
  constructor(gasReport: boolean, resultsCallback: (SuiteResult) => void)
  /**Runs the given test suites. */
  runTests(testSuites: Array<TestSuite>): Promise<Array<SuiteResult>>
}

The SolidityTestRunner makes no guarantees about when the progress callbacks are called. The only guarantee is that the callback will be scheduled to be called for each test suite as soon as the test suite finished executing. The scheduling is done by the NodeJS event loop and it can happen after the promise returned by runTests is resolved.

Normally NodeJS won’t exit as long as there are callbacks scheduled to be called by the event loop, but this can be overridden by calling process.exit() . So given code like this:

const testRunner = new SolidityTestsRunner(/* gasReport: */ false, () => { console.log("callback") })
const results = await testRunner.run()

// ...

process.exit()

It’s possible that the progress callback is never invoked.

As it turns out, the Hardhat CLI calls process.exit after all plugins have finished executing, which leads to progress reports not being printed sometimes in the Solidity tests plugin.

Solution

The process.exit call in Hardhat is probably there for a good reason, so I can see two solutions to this problem:

1. Make runTests return nothing, and have the callee accumulate results through the progress callback that receive each SuiteResult as it’s ready.
2. Make the promise returned by runTests only resolve after the callbacks have finished executing (as opposed to the current behavior which is to resolve after all test suites have finished executing).

I prefer the first solution, because it’s simpler on the Rust side and it gives full flexibility on the JS side where it’s easy to keep track of when all test suites have finished. E.g. one could promisify it as follows:

const testSuites: TestSuite[] = [...];

const results: Array<SuiteResult> = await new Promise((resolve) => {
  const gasReport = false;
  const resultsFromCallback: Array<SuiteResult> = [];

  runSolidityTests(testSuites, gasReport, (result: SuiteResult) => {
    resultsFromCallback.push(result);
    if (resultsFromCallback.length === testSuites.length) {
      resolve(resultsFromCallback);
    }
  });
});

// Calling `process.exit` here is no problem, because the promise only resolves 
// after all callbacks have fired.

And this could be modified to support event subscriptions or async progress callbacks without modifications on the Rust side.

Interface

The previous object-oriented interface had to be abandoned, because of lifetime issues with the JS progress callback once the runTests method was changed to return immediately after test execution started.

For background, we first wanted to have a single function to call to execute Solidity tests, but we also wanted to have this function return all the results. This meant the function had to be async. But the JsFunction callback passed into Rust is not Send which means it cannot be passed as argument into a napi-rs async function. As a workaround we added a SolidityTestRunner class, passed the JsFunction into its sync constructor and then added an async method to the class.

But because we held on the JsFunction in the SolidityTestRunner, we needed another workaround to let the event loop exit before the object is GC-ed by calling unref on the thread safe function wrapper for the JsFunction. When I changed the runTest method in this PR to return immediately after test execution started, this unref workaround was causing problems as the lifetime of the SolidityTestRunner object and the callback no longer aligned when called like this:

new Promise((resolve) => {
  const results = []

  const runner = new SolidityTestRunner((suiteResult) => {
    results.push(suiteResult)
    if (results.length === testSuites.length) {
       resolve(results)
    }
  })

  runner.runTests(testSuites)
})

With the unref workaround, the code above would panic with "thread safe function is closed" message and without the unref workaround there would be a noticeable delay between finishing test execution and the interpreter exiting. So I went back to the original design to just have a single free-standing function to run Solidity tests as the lifetimes are naturally aligned this way:

/**
 * Executes Solidity tests.
 *
 * The function will return as soon as test execution is started.
 * The progress callback will be called with the results of each test suite.
 * It is up to the caller to track how many times the callback is called to
 * know when all tests are done.
 */
export function runSolidityTests(test_suites: Array<TestSuite>, gas_report: boolean, progress_callback: (result: SuiteResult) => void): void

See full diff in crates/edr_napi/index.d.ts.

An alternative to the free standing function could be keeping the SolidityTestRunner and have runTests take the callback as argument, but I don't think having a SolidityTestRunner object is warranted (sans async workaround) as it'd have no other methods and it'd achieve the same operation with two calls instead of one. So it'd be just ceremony.

Considerations

• The promise result from runTests was originally introduced to make the JS interface more ergonomic, but it looks like it adds significantly more complexity on the Rust side than it saves on the JS side.

[changeset-bot]

⚠️ No Changeset found

Latest commit: b73fb80

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]

Thanks for the explanation, I'm ok with this change. And I guess the function will become something like this soon, right?

function runSolidityTests(artifacts: Array<Artifact>, test_suites: Array<ArtifactId>, gas_report: boolean, progress_callback: (result: SuiteResult) => void): void

[fvictorio]

LGTM on the interface side, but I didn't check the rust changes.

[agostbiro]

Thanks for the explanation, I'm ok with this change. And I guess the function will become something like this soon, right?

function runSolidityTests(artifacts: Array<Artifact>, test_suites: Array<ArtifactId>, gas_report: boolean, progress_callback: (result: SuiteResult) => void): void

Ok cheers and yeah that's what the interface would change to and gas_report will be soon replaced by a config object.

[Wodann]

Nice fix for this problem.

I had some nice to have comments and a recommendation regarding N-API.

[Wodann]

Consideration: As this is spawning a background process, it might be good to indicate this in the function name. The only thing I can come up with is spawnSolidityTestRunner.

Up to you whether you think this makes sense. The docs also outline this, in case the name is not self-explanatory.

[alcuadrado]

I'm ok with having this low-level interface being callback based, as we can then wrap it in something more idiomatic for js. But it should have a simple callback indicating the completion of the process.

I know we discussed leaving that responsibility to the user, but I think that would be less flexible/evolvable. E.g. if we implement a feature in edr that skips parts of the tests, every consumer would have to adapt the "counting the results until finish" code.

Note that the completion callback doesn't need to accumulate all the results. It could probably be () => void.

[agostbiro]

But it should have a simple callback indicating the completion of the process.

I like the idea and I can make that change, but tbh I'm a bit wary of doing it for the following reasons:

1. For now, the only users of this interface are in the EDR repo and there would be no usage of the completion callback.
2. We ran into the problem fixed by this PR, because we tried to anticipate usage patterns and provide a more ergonomic interface. This tells me that it's too early to polish the interface.

So I'd just add the completion callback to the interface design doc if that's ok?

[Wodann]

Nice to have: I see some duplication of this code with other call sites. We could provide a helper function in TS that accumulates the results as callbacks are called.

[agostbiro]

Yeah we will definitely want to have a JS wrapper at some point. It's not so clear how to handle it now especially assuming it's TS, so I'd defer this. E.g. is the wrapper in edr_napi (if yes how does the build tie together) or is it a separate package (how do we release that?).