refactor(cli): move runTests() and runBenchmarks() to rust #18563
Conversation
There was a problem hiding this comment.
I like what this PR is changing. Seeing a 100 line count reduction is also welcome. @nayeemrmn could you check how long does it take to run tests now? While driving them from Rust is beneficial overall I have a slight worry that it might add additional overhead - calling hundreds of function from Rust will most likely affect the time it takes to run all the tests.
cli/worker.rs
Outdated
| let earlier = SystemTime::now(); | ||
| let promise = { | ||
| let scope = &mut self.worker.js_runtime.handle_scope(); | ||
| let cb = function.open(scope); | ||
| let this = v8::undefined(scope).into(); | ||
| let promise = cb.call(scope, this, &[]).unwrap(); | ||
| v8::Global::new(scope, promise) | ||
| }; | ||
| let result = self.worker.js_runtime.resolve_value(promise).await?; |
There was a problem hiding this comment.
I like this. This gives us a clear path to implementing test timeout, that will be able to handle blocking sync code. We can now spawn a separate "watchdog" thread that will call IsolateHandle::terminate_execution if the test doesn't complete before the deadline 👍
|
1.32.2: 2.987ms per test const n = 5000;
let start: number;
addEventListener("load", () => {
start = performance.now();
});
addEventListener("unload", () => {
console.log((performance.now() - start) / n);
});
for (let i = 0; i < n; i++) {
Deno.test(`${i}`, () => {
// @ts-expect-error _
console.assert(i == i.toString());
});
}
// 12th Gen Intel(R) Core(TM) i9-12900K |
This reverts commit 48d96b6.
|
@nayeemrmn I just checked on v1.32.4 and the issues that this PR fixes are no longer occurring. Should we remove this linkage? |
You can see it with Deno.test("1", () => { Promise.reject() });
Deno.test("2", () => {});
Deno.test("3", () => {});
Deno.test("4", () => {});
// deno test --filter 1 temp.ts |
There was a problem hiding this comment.
LGTM, besides fixing the issue at hand I think it's a step in the right direction to add timeouts to tests.
@dsherret do you wanna review before merging?
Stores the test/bench functions in rust op state during registration. The functions are wrapped in JS first so that they return a directly convertible `TestResult`/`BenchResult`. Test steps are still mostly handled in JS since they are pretty much invoked by the user. Allows removing a bunch of infrastructure for communicating between JS and rust. Allows using rust utilities for things like shuffling tests (`Vec::shuffle`). We can progressively move op and resource sanitization to rust as well. Fixes #17122. Fixes #17312.
Stores the test/bench functions in rust op state during registration. The functions are wrapped in JS first so that they return a directly convertible
TestResult/BenchResult. Test steps are still mostly handled in JS since they are pretty much invoked by the user. Allows removing a bunch of infrastructure for communicating between JS and rust. Allows using rust utilities for things like shuffling tests (Vec::shuffle). We can progressively move op and resource sanitization to rust as well.Fixes #17122.
Fixes #17312.