fix(test): race condition for cancelled tests (#15233)

1 files changed, 14 insertions, 21 deletions

diff --git a/cli/tools/test.rs b/cli/tools/test.rs
index cbb5d0b53..e42ea673b 100644
--- a/cli/tools/test.rs
+++ b/cli/tools/test.rs
@@ -34,7 +34,6 @@ use deno_core::futures::stream;
 use deno_core::futures::FutureExt;
 use deno_core::futures::StreamExt;
 use deno_core::parking_lot::Mutex;
-use deno_core::parking_lot::RwLock;
 use deno_core::serde_json::json;
 use deno_core::url::Url;
 use deno_core::ModuleSpecifier;
@@ -1123,11 +1122,7 @@ async fn test_specifiers(
   let sender = TestEventSender::new(sender);
   let concurrent_jobs = options.concurrent_jobs;
   let fail_fast = options.fail_fast;
-  let tests: Arc<RwLock<IndexMap<usize, TestDescription>>> =
-    Arc::new(RwLock::new(IndexMap::new()));
-  let mut test_steps = IndexMap::new();
 
-  let tests_ = tests.clone();
   let join_handles =
     specifiers_with_mode.iter().map(move |(specifier, mode)| {
       let ps = ps.clone();
@@ -1136,7 +1131,6 @@ async fn test_specifiers(
       let mode = mode.clone();
       let mut sender = sender.clone();
       let options = options.clone();
-      let tests = tests_.clone();
 
       tokio::task::spawn_blocking(move || {
         let origin = specifier.to_string();
@@ -1151,18 +1145,9 @@ async fn test_specifiers(
         if let Err(error) = file_result {
           if error.is::<JsError>() {
             sender.send(TestEvent::UncaughtError(
-              origin.clone(),
+              origin,
               Box::new(error.downcast::<JsError>().unwrap()),
             ))?;
-            for desc in tests.read().values() {
-              if desc.origin == origin {
-                sender.send(TestEvent::Result(
-                  desc.id,
-                  TestResult::Cancelled,
-                  0,
-                ))?
-              }
-            }
           } else {
             return Err(error);
           }
@@ -1181,6 +1166,8 @@ async fn test_specifiers(
   let handler = {
     tokio::task::spawn(async move {
       let earlier = Instant::now();
+      let mut tests = IndexMap::new();
+      let mut test_steps = IndexMap::new();
       let mut tests_with_result = HashSet::new();
       let mut summary = TestSummary::new();
       let mut used_only = false;
@@ -1189,7 +1176,7 @@ async fn test_specifiers(
         match event {
           TestEvent::Register(description) => {
             reporter.report_register(&description);
-            tests.write().insert(description.id, description);
+            tests.insert(description.id, description);
           }
 
           TestEvent::Plan(plan) => {
@@ -1204,7 +1191,7 @@ async fn test_specifiers(
           }
 
           TestEvent::Wait(id) => {
-            reporter.report_wait(tests.read().get(&id).unwrap());
+            reporter.report_wait(tests.get(&id).unwrap());
           }
 
           TestEvent::Output(output) => {
@@ -1213,7 +1200,7 @@ async fn test_specifiers(
 
           TestEvent::Result(id, result, elapsed) => {
             if tests_with_result.insert(id) {
-              let description = tests.read().get(&id).unwrap().clone();
+              let description = tests.get(&id).unwrap().clone();
               match &result {
                 TestResult::Ok => {
                   summary.passed += 1;
@@ -1226,7 +1213,7 @@ async fn test_specifiers(
                   summary.failures.push((description.clone(), error.clone()));
                 }
                 TestResult::Cancelled => {
-                  summary.failed += 1;
+                  unreachable!("should be handled in TestEvent::UncaughtError");
                 }
               }
               reporter.report_result(&description, &result, elapsed);
@@ -1236,7 +1223,13 @@ async fn test_specifiers(
           TestEvent::UncaughtError(origin, error) => {
             reporter.report_uncaught_error(&origin, &error);
             summary.failed += 1;
-            summary.uncaught_errors.push((origin, error));
+            summary.uncaught_errors.push((origin.clone(), error));
+            for desc in tests.values() {
+              if desc.origin == origin && tests_with_result.insert(desc.id) {
+                summary.failed += 1;
+                reporter.report_result(desc, &TestResult::Cancelled, 0);
+              }
+            }
           }
 
           TestEvent::StepRegister(description) => {