feat(testing): add sanitization of pending promises

ext/lib.rs

@@ -7,6 +7,14 @@ pub mod extensions {
     pub use sable_ext_timers as timers;
     pub use sable_ext_web as web;
 
+    use deno_core::OpMetricsSummaryTracker;
+    use runtime::RuntimeState;
+    use std::rc::Rc;
+    use std::time::Instant;
+    use std::time::SystemTime;
+    use testing::PromiseMetricsSummaryTracker;
+    use timers::TimerQueue;
+
     deno_core::extension!(
         sable,
         ops = [
@@ -27,7 +35,10 @@ pub mod extensions {
             timers::op_create_timer,
             testing::op_bench_fn,
             testing::op_diff_str,
-            testing::op_test_async_ops_sanitization,
+            testing::op_get_outstanding_ops,
+            testing::op_get_pending_promises,
+            testing::op_set_promise_sanitization_hook,
+            testing::op_set_promise_sanitized_test_name,
             web::op_encoding_normalize_label,
             web::op_encoding_decode_utf8,
             web::op_encoding_decode_single,
@@ -62,23 +73,24 @@ pub mod extensions {
         ],
         state = |state| {
             // sable_ext_runtime
-            state.put(runtime::RuntimeState::Default);
+            state.put(RuntimeState::Default);
 
             // sable_ext_perf
-            state.put(std::time::Instant::now());
-            state.put(std::time::SystemTime::now());
+            state.put(Instant::now());
+            state.put(SystemTime::now());
 
             // sable_ext_timers
-            state.put(timers::TimerQueue::new());
+            state.put(TimerQueue::new());
 
             // sable_ext_testing
-            state.put::<Option<std::rc::Rc<deno_core::OpMetricsSummaryTracker>>>(None);
+            state.put::<Option<Rc<OpMetricsSummaryTracker>>>(None);
+            state.put::<Option<Rc<PromiseMetricsSummaryTracker>>>(None);
         }
     );
 
     deno_core::extension!(
         sable_cleanup,
         esm_entry_point = "ext:sable_cleanup/cleanup.js",
-        esm = ["cleanup.js",],
+        esm = ["cleanup.js"],
     );
 }

ext/testing/lib.rs

@@ -1,7 +1,10 @@
-use deno_core::{op2, v8, OpMetricsSummaryTracker, OpState};
+use deno_core::{op2, v8, JsRuntime, OpMetricsSummaryTracker, OpState};
 use diff::{PrettyDiffBuilder, PrettyDiffBuilderConfig};
 use std::{rc::Rc, time::Instant};
 
+mod promise_tracker;
+pub use promise_tracker::PromiseMetricsSummaryTracker;
+
 mod diff;
 use imara_diff::{diff, intern::InternedInput, Algorithm};
 
@@ -58,13 +61,77 @@ pub fn op_diff_str(#[string] before: &str, #[string] after: &str) -> String {
 
 /** Returns whether there are no async ops running in the background */
 #[op2(fast)]
-pub fn op_test_async_ops_sanitization(state: &OpState) -> bool {
-    let metrics_tracker = state.borrow::<Option<Rc<OpMetricsSummaryTracker>>>();
-    match metrics_tracker {
-        None => true,
+#[bigint]
+pub fn op_get_outstanding_ops(
+    #[state] op_metrics_tracker: &Option<Rc<OpMetricsSummaryTracker>>,
+) -> u64 {
+    match op_metrics_tracker {
+        None => 0,
         Some(tracker) => {
             let summary = tracker.aggregate();
-            summary.ops_completed_async == summary.ops_dispatched_async
+            summary.ops_dispatched_async - summary.ops_completed_async
         }
     }
 }
+
+#[op2(fast)]
+#[bigint]
+pub fn op_get_pending_promises(
+    #[state] promise_metrics_tracker: &Option<Rc<PromiseMetricsSummaryTracker>>,
+) -> u64 {
+    match promise_metrics_tracker {
+        None => 0,
+        Some(tracker) => tracker.metrics().map_or(0, |metrics| {
+            debug_assert!(
+                metrics.promises_initialized >= metrics.promises_resolved,
+                "Initialized promises should be greater or equal to resolved promises"
+            );
+            metrics.promises_initialized - metrics.promises_resolved
+        }),
+    }
+}
+
+extern "C" fn sanitization_promise_hook<'a, 'b>(
+    hook_type: v8::PromiseHookType,
+    promise: v8::Local<'a, v8::Promise>,
+    _: v8::Local<'b, v8::Value>,
+) {
+    let scope = unsafe { &mut v8::CallbackScope::new(promise) };
+    let state = JsRuntime::op_state_from(scope); // scopes deref into &Isolate
+    let mut state = state.borrow_mut();
+
+    let metrics_tracker = state
+        .borrow_mut::<Option<Rc<PromiseMetricsSummaryTracker>>>()
+        .as_ref()
+        .unwrap();
+
+    let promise_id = promise.get_identity_hash();
+
+    match hook_type {
+        v8::PromiseHookType::Init => {
+            let mut metrics = metrics_tracker.metrics_mut();
+            metrics.initialized(promise_id);
+        }
+        v8::PromiseHookType::Resolve => {
+            let Some(mut metrics) = metrics_tracker.metrics_mut_with_promise(promise_id) else {
+                // We don't want to track promises that we didn't initialize
+                return;
+            };
+            metrics.resolved(promise_id);
+        }
+        _ => {}
+    }
+}
+
+#[op2(fast)]
+pub fn op_set_promise_sanitized_test_name(state: &mut OpState, #[string] test_name: String) {
+    let Some(tracker) = state.borrow_mut::<Option<Rc<PromiseMetricsSummaryTracker>>>() else {
+        return;
+    };
+    tracker.track(test_name);
+}
+
+#[op2]
+pub fn op_set_promise_sanitization_hook(scope: &mut v8::HandleScope) {
+    scope.set_promise_hook(sanitization_promise_hook);
+}

ext/testing/mod.js

@@ -1,50 +1,35 @@
-import { op_test_async_ops_sanitization, op_diff_str, op_runtime_state, op_bench_fn } from "ext:core/ops";
+import {
+	op_bench_fn,
+	op_diff_str,
+	op_runtime_state,
+	op_set_promise_sanitization_hook,
+	op_set_promise_sanitized_test_name,
+	op_get_outstanding_ops,
+	op_get_pending_promises,
+} from "ext:core/ops";
 
 import { Printer } from "ext:sable/console/printer.js";
 import { styles } from "ext:sable/utils/ansi.js";
-import { textWidth } from "ext:sable/utils/text_width.js";
 
-/**
- * Error which gets thrown whenever:
- *  - TestContext is leaking async ops
- *  - Test gets started when async ops are still pending
- */
-class TestContextLeakingAsyncOpsError extends Error {
+class TestContextLeakingPendingPromisesError extends Error {
 	/**
-	 * @param {TestContext=} testContext
-	 * @param {boolean=} isAsync - whether given testContext callback returned a promise
+	 * @param {TestContext} testContext
+	 * @param {number} pendingPromises
+	 * @param {number} promiseLeniency
 	 */
-	constructor(testContext, isAsync) {
-		// TODO(Im-Beast): Replace this with pretty errors after they happen
-		let message = `
-You wanted to create a test, but there are still asynchronous ops running.
-Please make sure they've completed before you create the test.`;
-
-		if (testContext) {
-			message = `
-At least one asynchronous operation was started in ${testContext.title} but never completed!
-Please await all your promises or resolve test promise when every asynchronous operation has finished:`;
-
-			if (isAsync) {
-				message += `
-test('${testContext.title}', async (ctx) => {
-  ...
---^^^ ops leak somewhere around here, are you sure you awaited every promise?
-});`;
-			} else {
-				const ptd = "-".repeat(textWidth(testContext.title));
-
-				message += `
-test('${testContext.title}', (ctx) => {
---------${ptd}^ this test is not asynchronous, but leaks asynchronous ops
-  ...
---^^^ ops leak somewhere around here, are you sure this test was meant to be synchronous?
-});`;
-			}
-		}
-
-		super(message);
+	constructor(testContext, pendingPromises, promiseLeniency) {
+		super(`Test ${testContext.title} is leaking pending promises. ${pendingPromises} promises pending while ${promiseLeniency} have been allowed.`);
+		this.name = "TestContextPendingPromisesError";
+	}
+}
 
+class TestContextLeakingAsyncOpsError extends Error {
+	/**
+	 * @param {TestContext} testContext
+	 */
+	constructor(testContext) {
+			super(`At least one asynchronous operation was started in ${testContext.title} but never completed!
+Please await all your promises or resolve test promise when every asynchronous operation has finished:`)
 		this.name = "TestContextLeakingAsyncOpsError";
 	}
 }
@@ -54,25 +39,10 @@ class TestContextInvalidUsageError extends Error {
 	 * @param {TestContext} testContext
 	 */
 	constructor(testContext) {
-		const ptd = "-".repeat(textWidth(testContext.title));
-		const ctd = "-".repeat(textWidth(testContext.currentlyTested.title));
-
-		// TODO(Im-Beast): Replace this with pretty errors after they happen
 		super(
-			`
-You're using context from different step!
-Please use the step from current callback:
-test('${testContext.title}', (ctx) => {
-----------${ptd}^^^ you're using this
-  ...
-  ctx.test('${testContext.currentlyTested.title}', (ctx) => {
-----------------${ctd}^^^ instead of this
-    ...
-  });
-  ...
-});`,
+			`You're using context from different step in ${testContext.title}!
+Please use the context of the current callback.`,
 		);
-
 		this.name = "TestContextInvalidUsageError";
 	}
 }
@@ -89,30 +59,8 @@ class TestContextInUseError extends Error {
 		const currentTitle = testContext.currentlyTested.title;
 		const triedTitle = tried.title;
 
-		const ctd = "-".repeat(textWidth(currentTitle));
-		const cts = " ".repeat(textWidth(currentTitle));
-		const ttd = "-".repeat(textWidth(triedTitle));
-
-		// TODO(Im-Beast): Replace this with pretty errors after they happen
 		super(
-			`
-You started another sub-test when previous one didn't finish! (${parentTitle} is ${locked})
-Please check if you're not awaiting async sub-test:
-test('${parentTitle}', async (ctx) => {
-  ...
-  vvv${ctd}--- you're not awaiting it |
-  ctx.test('${currentTitle}', async (ctx) => { |
-               ${cts}^^^^^------------/
-                          but this is async
-    ...
-  });
-  ...
-      vvvv${ttd}------------- which in turn crashes here
-  ctx.test('${triedTitle}', (ctx) => {
-    ...
-  });
-  ...
-});`,
+			`You tried to start another sub-test (${triedTitle}) when previous one (${currentTitle}) didn't finish! (${parentTitle} is ${locked});`,
 		);
 
 		this.name = "TestContextInUseError";
@@ -374,12 +322,17 @@ class TestContext {
 
 	/**
 	 * @param {TestContext} testContext - currently evaluated TestContext
-	 * @param {boolean} async - whether TestContext returned a promise
+	 * @param {number} promiseLeniency - maximum amount of allowed pending promises
 	 * @throws when async ops are still pending
 	 */
-	static sanitizeAsyncOps(testContext = undefined, async = false) {
-		if (!op_test_async_ops_sanitization()) {
-			throw new TestContextLeakingAsyncOpsError(testContext, async);
+	static sanitizeAsyncOps(testContext = undefined, promiseLeniency = 0) {
+		if (op_get_outstanding_ops()) {
+			throw new TestContextLeakingAsyncOpsError(testContext);
+		}
+
+		const pendingPromises = op_get_pending_promises();
+		if (pendingPromises > promiseLeniency) {
+			throw new TestContextLeakingPendingPromisesError(testContext, pendingPromises, promiseLeniency);
 		}
 	}
 
@@ -419,21 +372,32 @@ class TestContext {
 		const testContext = new TestContext(name, parent);
 
 		parent?.lock(testContext);
+		op_set_promise_sanitized_test_name(name);
+		op_set_promise_sanitization_hook();
 		const response = callback(testContext);
-
 		if (response instanceof Promise) {
 			return response.then(() => {
+				// We allow two pending promises for the test itself
+				// And second for awaiting the test
+				// In case this assumption is wrong, program will still throw at the end of the event loop
+				TestContext.sanitizeAsyncOps(testContext, 2);
 				testContext.finish();
-				parent?.unlock(testContext);
-				TestContext.sanitizeAsyncOps(testContext, true);
-			});
+				if (parent) {
+					parent.unlock(testContext);
+					op_set_promise_sanitized_test_name(parent.name);
+				}
+			})
 		} else {
+			TestContext.sanitizeAsyncOps(testContext, 0);
 			testContext.finish();
-			parent?.unlock(testContext);
-			TestContext.sanitizeAsyncOps(testContext, false);
+			if (parent) {
+				parent.unlock(testContext);
+				op_set_promise_sanitized_test_name(parent.name);
+			}
 		}
 	}
 
+	// TODO(Im-Beast): Add TestContext.fails
 	/**
 	 * Create new sub-test with given callback
 	 * @param {string} name
@@ -671,11 +635,10 @@ function test(name, callback) {
 	}
 
 	if (runtimeState !== "test") {
-		Sable.bench = noop;
+		Sable.testing.test = noop;
 		return;
 	}
 
-	TestContext.sanitizeAsyncOps();
 	return TestContext.test(name, callback);
 }
 
@@ -686,7 +649,7 @@ function bench(name, callback) {
 	}
 
 	if (runtimeState !== "bench") {
-		Sable.bench = noop;
+		Sable.testing.bench = noop;
 		return;
 	}