From 850a98ce25acab051ef5695b575f5722179053d8 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Tue, 2 Aug 2022 15:55:11 +0100 Subject: [PATCH] fix(test): race condition for cancelled tests (#15233) --- cli/tests/testdata/test/uncaught_errors.out | 16 +++++++-- cli/tests/testdata/test/uncaught_errors_1.ts | 4 +++ cli/tools/test.rs | 35 ++++++++------------ 3 files changed, 32 insertions(+), 23 deletions(-) diff --git a/cli/tests/testdata/test/uncaught_errors.out b/cli/tests/testdata/test/uncaught_errors.out index 882a5d6dd9..2eae72e214 100644 --- a/cli/tests/testdata/test/uncaught_errors.out +++ b/cli/tests/testdata/test/uncaught_errors.out @@ -1,6 +1,9 @@ -running 2 tests from ./test/uncaught_errors_1.ts +running 3 tests from ./test/uncaught_errors_1.ts foo 1 ... FAILED ([WILDCARD]) foo 2 ... ok ([WILDCARD]) +foo 3 ... +Uncaught error from ./test/uncaught_errors_1.ts FAILED +foo 3 ... cancelled (0ms) running 3 tests from ./test/uncaught_errors_2.ts bar 1 ... ok ([WILDCARD]) bar 2 ... FAILED ([WILDCARD]) @@ -15,6 +18,14 @@ error: Error: foo 1 message ^ at [WILDCARD]/test/uncaught_errors_1.ts:2:9 +./test/uncaught_errors_1.ts (uncaught error) +error: (in promise) Error: foo 3 message + Promise.reject(new Error("foo 3 message")); + ^ + at [WILDCARD]/test/uncaught_errors_1.ts:8:18 +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. + bar 2 => ./test/uncaught_errors_2.ts:3:6 error: Error: bar 2 throw new Error("bar 2"); @@ -38,10 +49,11 @@ It most likely originated from a dangling promise, event/timeout handler or top- FAILURES foo 1 => ./test/uncaught_errors_1.ts:1:6 +./test/uncaught_errors_1.ts (uncaught error) bar 2 => ./test/uncaught_errors_2.ts:3:6 bar 3 => ./test/uncaught_errors_2.ts:6:6 ./test/uncaught_errors_3.ts (uncaught error) -FAILED | 2 passed | 4 failed ([WILDCARD]) +FAILED | 2 passed | 6 failed ([WILDCARD]) error: Test failed diff --git a/cli/tests/testdata/test/uncaught_errors_1.ts b/cli/tests/testdata/test/uncaught_errors_1.ts index ea3c557e4c..166b23ac3f 100644 --- a/cli/tests/testdata/test/uncaught_errors_1.ts +++ b/cli/tests/testdata/test/uncaught_errors_1.ts @@ -3,3 +3,7 @@ Deno.test("foo 1", () => { }); Deno.test("foo 2", () => {}); + +Deno.test("foo 3", () => { + Promise.reject(new Error("foo 3 message")); +}); diff --git a/cli/tools/test.rs b/cli/tools/test.rs index cbb5d0b531..e42ea673b2 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>> = - 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::() { sender.send(TestEvent::UncaughtError( - origin.clone(), + origin, Box::new(error.downcast::().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) => {