fix(test): handle dispatched exceptions from test functions (#18853)

Fixes #18852.

cli/tests/integration/test_tests.rs

@@ -425,6 +425,12 @@ itest!(uncaught_errors {
   exit_code: 1,
 });
 
+itest!(report_error {
+  args: "test --quiet test/report_error.ts",
+  output: "test/report_error.out",
+  exit_code: 1,
+});
+
 itest!(check_local_by_default {
   args: "test --quiet test/check_local_by_default.ts",
   output: "test/check_local_by_default.out",

cli/tests/testdata/test/report_error.out

@@ -0,0 +1,23 @@
+running 2 tests from [WILDCARD]/report_error.ts
+foo ...
+Uncaught error from [WILDCARD]/report_error.ts FAILED
+foo ... cancelled (0ms)
+bar ... cancelled (0ms)
+
+ ERRORS 
+
+[WILDCARD]/report_error.ts (uncaught error)
+error: Error: foo
+  reportError(new Error("foo"));
+              ^
+    at [WILDCARD]/report_error.ts:2:15
+This error was not caught from a test and caused the test runner to fail on the referenced module.
+It most likely originated from a dangling promise, event/timeout handler or top-level code.
+
+ FAILURES 
+
+[WILDCARD]/report_error.ts (uncaught error)
+
+FAILED | 0 passed | 3 failed ([WILDCARD])
+
+error: Test failed

cli/tests/testdata/test/report_error.ts

@@ -0,0 +1,6 @@
+Deno.test("foo", () => {
+  reportError(new Error("foo"));
+  console.log(1);
+});
+
+Deno.test("bar", () => {});

cli/tools/bench.rs

@@ -489,14 +489,7 @@ async fn bench_specifier(
   }))?;
   for (desc, function) in benchmarks {
     sender.send(BenchEvent::Wait(desc.id))?;
-    let promise = {
-      let scope = &mut 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 = worker.js_runtime.resolve_value(promise).await?;
+    let result = worker.js_runtime.call_and_await(&function).await?;
     let scope = &mut worker.js_runtime.handle_scope();
     let result = v8::Local::new(scope, result);
     let result = serde_v8::from_v8::<BenchResult>(scope, result)?;

cli/tools/test.rs

@@ -997,14 +997,7 @@ pub async fn test_specifier(
     }
     sender.send(TestEvent::Wait(desc.id))?;
     let earlier = SystemTime::now();
-    let promise = {
-      let scope = &mut 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 = match worker.js_runtime.resolve_value(promise).await {
+    let result = match worker.js_runtime.call_and_await(&function).await {
       Ok(r) => r,
       Err(error) => {
         if error.is::<JsError>() {

core/inspector.rs

@@ -11,7 +11,6 @@ use crate::futures::channel::mpsc::UnboundedSender;
 use crate::futures::channel::oneshot;
 use crate::futures::future::select;
 use crate::futures::future::Either;
-use crate::futures::future::Future;
 use crate::futures::prelude::*;
 use crate::futures::stream::SelectAll;
 use crate::futures::stream::StreamExt;
@@ -82,6 +81,7 @@ pub struct JsRuntimeInspector {
   flags: RefCell<InspectorFlags>,
   waker: Arc<InspectorWaker>,
   deregister_tx: Option<oneshot::Sender<()>>,
+  is_dispatching_message: RefCell<bool>,
 }
 
 impl Drop for JsRuntimeInspector {
@@ -141,18 +141,6 @@ impl v8::inspector::V8InspectorClientImpl for JsRuntimeInspector {
   }
 }
 
-/// Polling `JsRuntimeInspector` allows inspector to accept new incoming
-/// connections and "pump" messages in different sessions.
-///
-/// It should be polled on tick of event loop, ie. in `JsRuntime::poll_event_loop`
-/// function.
-impl Future for JsRuntimeInspector {
-  type Output = ();
-  fn poll(self: Pin<&mut Self>, cx: &mut Context) -> Poll<()> {
-    self.poll_sessions(Some(cx)).unwrap()
-  }
-}
-
 impl JsRuntimeInspector {
   /// Currently Deno supports only a single context in `JsRuntime`
   /// and thus it's id is provided as an associated contant.
@@ -182,6 +170,7 @@ impl JsRuntimeInspector {
       flags: Default::default(),
       waker,
       deregister_tx: None,
+      is_dispatching_message: Default::default(),
     }));
     let mut self_ = self__.borrow_mut();
     self_.v8_inspector = Rc::new(RefCell::new(
@@ -224,6 +213,10 @@ impl JsRuntimeInspector {
     self__
   }
 
+  pub fn is_dispatching_message(&self) -> bool {
+    *self.is_dispatching_message.borrow()
+  }
+
   pub fn context_destroyed(
     &mut self,
     scope: &mut HandleScope,
@@ -246,7 +239,7 @@ impl JsRuntimeInspector {
     self.sessions.borrow().has_blocking_sessions()
   }
 
-  fn poll_sessions(
+  pub fn poll_sessions(
     &self,
     mut invoker_cx: Option<&mut Context>,
   ) -> Result<Poll<()>, BorrowMutError> {
@@ -304,7 +297,9 @@ impl JsRuntimeInspector {
         match sessions.established.poll_next_unpin(cx) {
           Poll::Ready(Some(session_stream_item)) => {
             let (v8_session_ptr, msg) = session_stream_item;
+            *self.is_dispatching_message.borrow_mut() = true;
             InspectorSession::dispatch_message(v8_session_ptr, msg);
+            *self.is_dispatching_message.borrow_mut() = false;
             continue;
           }
           Poll::Ready(None) => break,

core/ops_builtin_v8.rs

@@ -713,22 +713,17 @@ fn op_dispatch_exception(
 ) {
   let state_rc = JsRuntime::state(scope);
   let mut state = state_rc.borrow_mut();
-  state
-    .dispatched_exceptions
-    .push_front(v8::Global::new(scope, exception.v8_value));
-  // Only terminate execution if there are no inspector sessions.
-  if state.inspector.is_none() {
-    scope.terminate_execution();
+  if let Some(inspector) = &state.inspector {
+    let inspector = inspector.borrow();
+    // TODO(nayeemrmn): Send exception message to inspector sessions here.
+
+    // This indicates that the op is being called from a REPL. Skip termination.
+    if inspector.is_dispatching_message() {
       return;
     }
-
-  // FIXME(bartlomieju): I'm not sure if this assumption is valid... Maybe when
-  // inspector is polling on pause?
-  if state.inspector().try_borrow().is_ok() {
-    scope.terminate_execution();
-  } else {
-    // If the inspector is borrowed at this time, assume an inspector is active.
   }
+  state.dispatched_exception = Some(v8::Global::new(scope, exception.v8_value));
+  scope.terminate_execution();
 }
 
 #[op(v8)]

core/runtime.rs

@@ -44,7 +44,6 @@ use smallvec::SmallVec;
 use std::any::Any;
 use std::cell::RefCell;
 use std::collections::HashMap;
-use std::collections::VecDeque;
 use std::ffi::c_void;
 use std::option::Option;
 use std::pin::Pin;
@@ -176,7 +175,7 @@ pub struct JsRuntimeState {
   /// instead of any other exceptions.
   // TODO(nayeemrmn): This is polled in `exception_to_err_result()` which is
   // flimsy. Try to poll it similarly to `pending_promise_rejections`.
-  pub(crate) dispatched_exceptions: VecDeque<v8::Global<v8::Value>>,
+  pub(crate) dispatched_exception: Option<v8::Global<v8::Value>>,
   pub(crate) inspector: Option<Rc<RefCell<JsRuntimeInspector>>>,
   waker: AtomicWaker,
 }
@@ -349,7 +348,7 @@ impl JsRuntime {
       op_state: op_state.clone(),
       waker: AtomicWaker::new(),
       have_unpolled_ops: false,
-      dispatched_exceptions: Default::default(),
+      dispatched_exception: None,
       // Some fields are initialized later after isolate is created
       inspector: None,
       global_realm: None,
@@ -946,6 +945,27 @@ impl JsRuntime {
     )
   }
 
+  /// Call a function. If it returns a promise, run the event loop until that
+  /// promise is settled. If the promise rejects or there is an uncaught error
+  /// in the event loop, return `Err(error)`. Or return `Ok(<await returned>)`.
+  pub async fn call_and_await(
+    &mut self,
+    function: &v8::Global<v8::Function>,
+  ) -> Result<v8::Global<v8::Value>, Error> {
+    let promise = {
+      let scope = &mut self.handle_scope();
+      let cb = function.open(scope);
+      let this = v8::undefined(scope).into();
+      let promise = cb.call(scope, this, &[]);
+      if promise.is_none() || scope.is_execution_terminating() {
+        let undefined = v8::undefined(scope).into();
+        return exception_to_err_result(scope, undefined, false);
+      }
+      v8::Global::new(scope, promise.unwrap())
+    };
+    self.resolve_value(promise).await
+  }
+
   /// Takes a snapshot. The isolate should have been created with will_snapshot
   /// set to true.
   ///
@@ -1195,7 +1215,7 @@ impl JsRuntime {
 
     if has_inspector {
       // We poll the inspector first.
-      let _ = self.inspector().borrow_mut().poll_unpin(cx);
+      let _ = self.inspector().borrow().poll_sessions(Some(cx)).unwrap();
     }
 
     self.pump_v8_message_loop()?;
@@ -1518,19 +1538,14 @@ pub(crate) fn exception_to_err_result<T>(
     // to use the exception that was passed to it rather than the exception that
     // was passed to this function.
     let state = state_rc.borrow();
-    exception = state
-      .dispatched_exceptions
-      .back()
-      .map(|exception| v8::Local::new(scope, exception.clone()))
-      .unwrap_or_else(|| {
-        // Maybe make a new exception object.
-        if was_terminating_execution && exception.is_null_or_undefined() {
+    exception = if let Some(exception) = &state.dispatched_exception {
+      v8::Local::new(scope, exception.clone())
+    } else if was_terminating_execution && exception.is_null_or_undefined() {
       let message = v8::String::new(scope, "execution terminated").unwrap();
       v8::Exception::error(scope, message)
     } else {
       exception
-        }
-      });
+    };
   }
 
   let mut js_error = JsError::from_v8_exception(scope, exception);
@@ -1738,7 +1753,7 @@ impl JsRuntime {
     status = module.get_status();
 
     let has_dispatched_exception =
-      !state_rc.borrow_mut().dispatched_exceptions.is_empty();
+      state_rc.borrow_mut().dispatched_exception.is_some();
     if has_dispatched_exception {
       // This will be overrided in `exception_to_err_result()`.
       let exception = v8::undefined(tc_scope).into();