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

refactor: Solidity test runner progress callback completion #563

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions crates/edr_napi/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,15 @@ export interface TestContract {
*/
libraries: Array<string>
}
/**
* 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(testSuites: Array<TestSuite>, gasReport: boolean, progressCallback: (result: SuiteResult) => void): void
export interface SubscriptionEvent {
filterId: bigint
result: any
Expand Down Expand Up @@ -542,13 +551,6 @@ export class Response {
get solidityTrace(): RawTrace | null
get traces(): Array<RawTrace>
}
/** 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: (...args: any[]) => any)
/**Runs the given test suites. */
runTests(testSuites: Array<TestSuite>): Promise<Array<SuiteResult>>
}
export class RawTrace {
trace(): Array<TracingMessage | TracingStep | TracingMessageResult>
}
4 changes: 2 additions & 2 deletions crates/edr_napi/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ if (!nativeBinding) {
throw new Error(`Failed to load native binding`)
}

const { SpecId, EdrContext, MineOrdering, Provider, Response, SuccessReason, ExceptionalHalt, TestStatus, SolidityTestRunner, RawTrace } = nativeBinding
const { SpecId, EdrContext, MineOrdering, Provider, Response, SuccessReason, ExceptionalHalt, TestStatus, runSolidityTests, RawTrace } = nativeBinding

module.exports.SpecId = SpecId
module.exports.EdrContext = EdrContext
Expand All @@ -320,5 +320,5 @@ module.exports.Response = Response
module.exports.SuccessReason = SuccessReason
module.exports.ExceptionalHalt = ExceptionalHalt
module.exports.TestStatus = TestStatus
module.exports.SolidityTestRunner = SolidityTestRunner
module.exports.runSolidityTests = runSolidityTests
module.exports.RawTrace = RawTrace
130 changes: 50 additions & 80 deletions crates/edr_napi/src/solidity_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,95 +12,65 @@ use napi::{
},
tokio,
tokio::runtime,
Env, JsFunction,
JsFunction,
};
use napi_derive::napi;

use crate::solidity_tests::{
runner::build_runner, test_results::SuiteResult, test_suite::TestSuite,
};

/// Executes solidity tests.
/// 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.
// False positive from Clippy. The function is exposed through the FFI.
#[allow(dead_code)]
#[napi]
pub struct SolidityTestRunner {
/// The callback to call with the results as they become available.
results_callback_fn: ThreadsafeFunction<SuiteResult>,
pub fn run_solidity_tests(
test_suites: Vec<TestSuite>,
gas_report: bool,
}

// The callback has to be passed in the constructor because it's not `Send`.
#[napi]
impl SolidityTestRunner {
#[doc = "Creates a new instance of the SolidityTestRunner. The callback function will be called with suite results as they finish."]
#[napi(constructor)]
pub fn new(env: Env, gas_report: bool, results_callback: JsFunction) -> napi::Result<Self> {
let mut results_callback_fn: ThreadsafeFunction<_, ErrorStrategy::CalleeHandled> =
results_callback.create_threadsafe_function(
// Unbounded queue size
0,
|ctx: ThreadSafeCallContext<SuiteResult>| Ok(vec![ctx.value]),
)?;

// Allow the event loop to exit before the function is destroyed
results_callback_fn.unref(&env)?;

Ok(Self {
results_callback_fn,
gas_report,
})
}

#[doc = "Runs the given test suites."]
#[napi]
pub async fn run_tests(&self, test_suites: Vec<TestSuite>) -> napi::Result<Vec<SuiteResult>> {
let test_suites = test_suites
.into_iter()
.map(|item| Ok((item.id.try_into()?, item.contract.try_into()?)))
.collect::<Result<Vec<_>, napi::Error>>()?;
let runner = build_runner(test_suites, self.gas_report)?;

let (tx_results, mut rx_results) =
tokio::sync::mpsc::unbounded_channel::<(String, forge::result::SuiteResult)>();
let (tx_end_result, rx_end_result) = tokio::sync::oneshot::channel();

let callback_fn = self.results_callback_fn.clone();
let runtime = runtime::Handle::current();
runtime.spawn(async move {
let mut results = Vec::<(String, forge::result::SuiteResult)>::new();

while let Some(name_and_suite_result) = rx_results.recv().await {
results.push(name_and_suite_result.clone());
// Blocking mode won't block in our case because the function was created with
// unlimited queue size https://github.com/nodejs/node-addon-api/blob/main/doc/threadsafe_function.md#blockingcall--nonblockingcall
let call_status = callback_fn.call(
Ok(name_and_suite_result.into()),
ThreadsafeFunctionCallMode::Blocking,
);
// This should always succeed since we're using an unbounded queue. We add an
// assertion for completeness.
assert!(
matches!(call_status, napi::Status::Ok),
"Failed to call callback with status {call_status:?}"
);
}

let js_suite_results = results
.into_iter()
.map(Into::into)
.collect::<Vec<SuiteResult>>();
tx_end_result
.send(js_suite_results)
.expect("Failed to send end result");
});

runner.test_hardhat(Arc::new(EverythingFilter), tx_results);

let results = rx_end_result.await.map_err(|_err| {
napi::Error::new(napi::Status::GenericFailure, "Failed to receive end result")
})?;

Ok(results)
}
#[napi(ts_arg_type = "(result: SuiteResult) => void")] progress_callback: JsFunction,
) -> napi::Result<()> {
let results_callback_fn: ThreadsafeFunction<_, ErrorStrategy::Fatal> = progress_callback
.create_threadsafe_function(
// Unbounded queue size
0,
|ctx: ThreadSafeCallContext<SuiteResult>| Ok(vec![ctx.value]),
)?;

let test_suites = test_suites
.into_iter()
.map(|item| Ok((item.id.try_into()?, item.contract.try_into()?)))
.collect::<Result<Vec<_>, napi::Error>>()?;
let runner = build_runner(test_suites, gas_report)?;

let (tx_results, mut rx_results) =
tokio::sync::mpsc::unbounded_channel::<(String, forge::result::SuiteResult)>();

let runtime = runtime::Handle::current();
runtime.spawn(async move {
while let Some(name_and_suite_result) = rx_results.recv().await {
let callback_arg = name_and_suite_result.into();
// Blocking mode won't block in our case because the function was created with
// unlimited queue size https://github.com/nodejs/node-addon-api/blob/main/doc/threadsafe_function.md#blockingcall--nonblockingcall
let call_status =
results_callback_fn.call(callback_arg, ThreadsafeFunctionCallMode::Blocking);
// This should always succeed since we're using an unbounded queue. We add an
// assertion for completeness.
assert!(
matches!(call_status, napi::Status::Ok),
"Failed to call callback with status {call_status:?}"
);
}
});

// Returns immediately after test suite execution is started
runner.test_hardhat(Arc::new(EverythingFilter), tx_results);

Ok(())
}

struct EverythingFilter;
Expand Down
24 changes: 12 additions & 12 deletions crates/edr_napi/test/solidity-tests.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
import { assert } from "chai";
import { mkdtemp } from "fs/promises";
import { tmpdir } from "os";
import path from "path";

import {
SolidityTestRunner,
TestSuite,
TestContract,
ArtifactId,
SuiteResult,
runSolidityTests,
} from "..";

describe("Solidity Tests", () => {
Expand All @@ -18,18 +15,21 @@ describe("Solidity Tests", () => {
loadContract("./artifacts/PaymentFailureTest.json"),
];

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

const resultsFromCallback: Array<SuiteResult> = [];
const runner = new SolidityTestRunner(gasReport, (...args) => {
resultsFromCallback.push(args[1] as SuiteResult);
runSolidityTests(testSuites, gasReport, (result: SuiteResult) => {
resultsFromCallback.push(result);
if (resultsFromCallback.length === testSuites.length) {
resolve(resultsFromCallback);
}
});
});

const result = await runner.runTests(testSuites);
assert.equal(resultsFromCallback.length, testSuites.length);
assert.equal(resultsFromCallback.length, result.length);
assert.equal(results.length, testSuites.length);

for (let res of result) {
for (let res of results) {
if (res.name.includes("SetupConsistencyCheck")) {
assert.equal(res.testResults.length, 2);
assert.equal(res.testResults[0].status, "Success");
Expand Down
49 changes: 25 additions & 24 deletions crates/foundry/forge/src/multi_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,9 @@ impl MultiContractRunner {

/// Executes _all_ tests that match the given `filter`.
///
/// The method will immediately return and send the results to the given
/// channel as they're ready.
///
/// This will create the runtime based on the configured `evm` ops and
/// create the `Backend` before executing all contracts and their tests
/// in _parallel_.
Expand All @@ -202,7 +205,6 @@ impl MultiContractRunner {
filter: Arc<impl TestFilter + 'static>,
tx: tokio::sync::mpsc::UnboundedSender<(String, SuiteResult)>,
) {
let handle = tokio::runtime::Handle::current();
trace!("running all tests");

// The DB backend that serves all the data.
Expand All @@ -226,29 +228,28 @@ impl MultiContractRunner {
.into_iter()
.zip(std::iter::repeat((this, db, filter, tx)));

tokio::task::block_in_place(move || {
handle.block_on(async {
futures::stream::iter(args)
.for_each_concurrent(
Some(num_cpus::get()),
|((id, contract), (this, db, filter, tx))| async move {
tokio::task::spawn_blocking(move || {
let handle = tokio::runtime::Handle::current();
let result = this.run_tests_hardhat(
&id,
&contract,
db.clone(),
filter.as_ref(),
&handle,
);
let _ = tx.send((id.identifier(), result));
})
.await
.expect("failed to join task");
},
)
.await;
});
let handle = tokio::runtime::Handle::current();
handle.spawn(async {
futures::stream::iter(args)
.for_each_concurrent(
Some(num_cpus::get()),
|((id, contract), (this, db, filter, tx))| async move {
tokio::task::spawn_blocking(move || {
let handle = tokio::runtime::Handle::current();
let result = this.run_tests_hardhat(
&id,
&contract,
db.clone(),
filter.as_ref(),
&handle,
);
let _ = tx.send((id.identifier(), result));
})
.await
.expect("failed to join task");
},
)
.await;
});
}

Expand Down
16 changes: 12 additions & 4 deletions crates/tools/js/benchmark/solidity-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const fs = require("fs");
const { execSync } = require("child_process");
const path = require("path");
const simpleGit = require("simple-git");
const { SolidityTestRunner } = require("@nomicfoundation/edr");
const { runSolidityTests } = require("@nomicfoundation/edr");

const EXCLUDED_TEST_SUITES = new Set([
"StdChainsTest",
Expand Down Expand Up @@ -61,10 +61,18 @@ async function runForgeStdTests(forgeStdRepoPath) {
.map(loadContract.bind(null, hardhatConfig))
.filter((ts) => !EXCLUDED_TEST_SUITES.has(ts.id.name));

const runner = new SolidityTestRunner(gasReport, (...args) => {
console.error(`${args[1].name} took ${elapsedSec(start)} seconds`);
const results = await new Promise((resolve) => {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@agostbiro agostbiro Jul 29, 2024

Choose a reason for hiding this comment

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

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?).

const resultsFromCallback = [];

runSolidityTests(testSuites, gasReport, (result) => {
console.error(`${result.name} took ${elapsedSec(start)} seconds`);

resultsFromCallback.push(result);
if (resultsFromCallback.length === testSuites.length) {
resolve(resultsFromCallback);
}
});
});
const results = await runner.runTests(testSuites);
console.error("elapsed (s)", elapsedSec(start));

if (results.length !== EXPECTED_RESULTS) {
Expand Down
Loading
Loading