From dbd39cf65200b70f2d2c983d7bad013b3403d895 Mon Sep 17 00:00:00 2001 From: Tom Parker-Shemilt Date: Mon, 12 Dec 2022 15:09:12 +0000 Subject: [PATCH 1/2] Remove timeout_ms --- README.md | 11 --- serial_test/src/code_lock.rs | 11 +-- serial_test/src/lib.rs | 12 --- serial_test/src/parallel_code_lock.rs | 22 ++--- serial_test/src/parallel_file_lock.rs | 16 +--- serial_test/src/serial_code_lock.rs | 23 ++--- serial_test/src/serial_file_lock.rs | 11 +-- serial_test/tests/tests.rs | 2 +- serial_test_derive/src/lib.rs | 119 ++------------------------ serial_test_test/src/lib.rs | 8 -- 10 files changed, 33 insertions(+), 202 deletions(-) diff --git a/README.md b/README.md index 8541025..49a803e 100644 --- a/README.md +++ b/README.md @@ -53,14 +53,3 @@ extern crate serial_test; for earlier versions. You can then either add `#[serial]` or `#[serial(some_text)]` to tests as required. - -For each test, a timeout can be specified with the `timeout_ms` parameter to the [serial](macro@serial) attribute. Note that -the timeout is counted from the first invocation of the test, not from the time the previous test was completed. This can -lead to [some unpredictable behavior](https://github.com/palfrey/serial_test/issues/76) based on the number of parallel tests run on the system. -```rust -#[test] -#[serial(timeout_ms = 1000)] -fn test_serial_one() { - // Do things -} -``` \ No newline at end of file diff --git a/serial_test/src/code_lock.rs b/serial_test/src/code_lock.rs index ebd728b..4afb01d 100644 --- a/serial_test/src/code_lock.rs +++ b/serial_test/src/code_lock.rs @@ -5,7 +5,7 @@ use lazy_static::lazy_static; use log::debug; use std::{ sync::{atomic::AtomicU32, Arc}, - time::{Duration, Instant}, + time::Instant, }; pub(crate) struct UniqueReentrantMutex { @@ -55,7 +55,7 @@ impl Default for UniqueReentrantMutex { } } -pub(crate) fn check_new_key(name: &str, max_wait: Option) { +pub(crate) fn check_new_key(name: &str) { let start = Instant::now(); loop { #[cfg(all(feature = "logging"))] @@ -84,12 +84,5 @@ pub(crate) fn check_new_key(name: &str, max_wait: Option) { // If the try_entry fails, then go around the loop again // Odds are another test was also locking on the write and has now written the key - - if let Some(max_wait) = max_wait { - let duration = start.elapsed(); - if duration > max_wait { - panic!("Timeout waiting for '{}' {:?}", name, duration); - } - } } } diff --git a/serial_test/src/lib.rs b/serial_test/src/lib.rs index d4cb5d6..25a4967 100644 --- a/serial_test/src/lib.rs +++ b/serial_test/src/lib.rs @@ -46,18 +46,6 @@ //! } //! ```` //! -//! -//! For each test, a timeout can be specified with the `timeout_ms` parameter to the [serial](macro@serial) attribute. Note that -//! the timeout is counted from the first invocation of the test, not from the time the previous test was completed. This can -//! lead to [some unpredictable behavior](https://github.com/palfrey/serial_test/issues/76) based on the number of parallel tests run on the system. -//! ```rust -//! #[test] -//! #[serial(timeout_ms = 1000)] -//! fn test_serial_one() { -//! // Do things -//! } -//! ``` -//! //! ## Feature flags #![cfg_attr( feature = "docsrs", diff --git a/serial_test/src/parallel_code_lock.rs b/serial_test/src/parallel_code_lock.rs index 0d4da78..ff49254 100644 --- a/serial_test/src/parallel_code_lock.rs +++ b/serial_test/src/parallel_code_lock.rs @@ -3,15 +3,14 @@ use crate::code_lock::{check_new_key, LOCKS}; #[cfg(feature = "async")] use futures::FutureExt; -use std::{panic, time::Duration}; +use std::panic; #[doc(hidden)] pub fn local_parallel_core_with_return( name: &str, - max_wait: Option, function: fn() -> Result<(), E>, ) -> Result<(), E> { - check_new_key(name, max_wait); + check_new_key(name); let lock = LOCKS.get(name).unwrap(); lock.start_parallel(); @@ -26,8 +25,8 @@ pub fn local_parallel_core_with_return( } #[doc(hidden)] -pub fn local_parallel_core(name: &str, max_wait: Option, function: fn()) { - check_new_key(name, max_wait); +pub fn local_parallel_core(name: &str, function: fn()) { + check_new_key(name); let lock = LOCKS.get(name).unwrap(); lock.start_parallel(); @@ -44,10 +43,9 @@ pub fn local_parallel_core(name: &str, max_wait: Option, function: fn( #[cfg(feature = "async")] pub async fn local_async_parallel_core_with_return( name: &str, - max_wait: Option, fut: impl std::future::Future> + panic::UnwindSafe, ) -> Result<(), E> { - check_new_key(name, max_wait); + check_new_key(name); let lock = LOCKS.get(name).unwrap(); lock.start_parallel(); @@ -65,10 +63,9 @@ pub async fn local_async_parallel_core_with_return( #[cfg(feature = "async")] pub async fn local_async_parallel_core( name: &str, - max_wait: Option, fut: impl std::future::Future + panic::UnwindSafe, ) { - check_new_key(name, max_wait); + check_new_key(name); let lock = LOCKS.get(name).unwrap(); lock.start_parallel(); @@ -90,7 +87,7 @@ mod tests { #[test] fn unlock_on_assert_sync_without_return() { let _ = panic::catch_unwind(|| { - local_parallel_core("unlock_on_assert_sync_without_return", None, || { + local_parallel_core("unlock_on_assert_sync_without_return", || { assert!(false); }) }); @@ -108,7 +105,6 @@ mod tests { let _ = panic::catch_unwind(|| { local_parallel_core_with_return( "unlock_on_assert_sync_with_return", - None, || -> Result<(), Error> { assert!(false); Ok(()) @@ -131,8 +127,7 @@ mod tests { assert!(false); } async fn call_serial_test_fn() { - local_async_parallel_core("unlock_on_assert_async_without_return", None, demo_assert()) - .await + local_async_parallel_core("unlock_on_assert_async_without_return", demo_assert()).await } // as per https://stackoverflow.com/a/66529014/320546 let _ = panic::catch_unwind(|| { @@ -161,7 +156,6 @@ mod tests { async fn call_serial_test_fn() { local_async_parallel_core_with_return( "unlock_on_assert_async_with_return", - None, demo_assert(), ) .await; diff --git a/serial_test/src/parallel_file_lock.rs b/serial_test/src/parallel_file_lock.rs index fb4430e..40fba9d 100644 --- a/serial_test/src/parallel_file_lock.rs +++ b/serial_test/src/parallel_file_lock.rs @@ -1,4 +1,4 @@ -use std::{panic, time::Duration}; +use std::panic; #[cfg(feature = "async")] use futures::FutureExt; @@ -6,12 +6,7 @@ use futures::FutureExt; use crate::file_lock::make_lock_for_name_and_path; #[doc(hidden)] -pub fn fs_parallel_core( - name: &str, - _max_wait: Option, - path: Option<&str>, - function: fn(), -) { +pub fn fs_parallel_core(name: &str, path: Option<&str>, function: fn()) { make_lock_for_name_and_path(name, path).start_parallel(); let res = panic::catch_unwind(|| { function(); @@ -25,7 +20,6 @@ pub fn fs_parallel_core( #[doc(hidden)] pub fn fs_parallel_core_with_return( name: &str, - _max_wait: Option, path: Option<&str>, function: fn() -> Result<(), E>, ) -> Result<(), E> { @@ -44,7 +38,6 @@ pub fn fs_parallel_core_with_return( #[cfg(feature = "async")] pub async fn fs_async_parallel_core_with_return( name: &str, - _max_wait: Option, path: Option<&str>, fut: impl std::future::Future> + panic::UnwindSafe, ) -> Result<(), E> { @@ -63,7 +56,6 @@ pub async fn fs_async_parallel_core_with_return( #[cfg(feature = "async")] pub async fn fs_async_parallel_core( name: &str, - _max_wait: Option, path: Option<&str>, fut: impl std::future::Future + panic::UnwindSafe, ) { @@ -97,7 +89,6 @@ mod tests { let _ = panic::catch_unwind(|| { fs_parallel_core( "unlock_on_assert_sync_without_return", - None, Some(&lock_path), || { assert!(false); @@ -113,7 +104,6 @@ mod tests { let _ = panic::catch_unwind(|| { fs_parallel_core_with_return( "unlock_on_assert_sync_with_return", - None, Some(&lock_path), || -> Result<(), Error> { assert!(false); @@ -134,7 +124,6 @@ mod tests { async fn call_serial_test_fn(lock_path: &str) { fs_async_parallel_core( "unlock_on_assert_async_without_return", - None, Some(&lock_path), demo_assert(), ) @@ -164,7 +153,6 @@ mod tests { async fn call_serial_test_fn(lock_path: &str) { fs_async_parallel_core_with_return( "unlock_on_assert_async_with_return", - None, Some(&lock_path), demo_assert(), ) diff --git a/serial_test/src/serial_code_lock.rs b/serial_test/src/serial_code_lock.rs index dad9abe..96a47d7 100644 --- a/serial_test/src/serial_code_lock.rs +++ b/serial_test/src/serial_code_lock.rs @@ -1,15 +1,13 @@ #![allow(clippy::await_holding_lock)] use crate::code_lock::{check_new_key, LOCKS}; -use std::time::Duration; #[doc(hidden)] pub fn local_serial_core_with_return( name: &str, - max_wait: Option, function: fn() -> Result<(), E>, ) -> Result<(), E> { - check_new_key(name, max_wait); + check_new_key(name); let unlock = LOCKS.get(name).expect("key to be set"); // _guard needs to be named to avoid being instant dropped @@ -18,8 +16,8 @@ pub fn local_serial_core_with_return( } #[doc(hidden)] -pub fn local_serial_core(name: &str, max_wait: Option, function: fn()) { - check_new_key(name, max_wait); +pub fn local_serial_core(name: &str, function: fn()) { + check_new_key(name); let unlock = LOCKS.get(name).expect("key to be set"); // _guard needs to be named to avoid being instant dropped @@ -31,10 +29,9 @@ pub fn local_serial_core(name: &str, max_wait: Option, function: fn()) #[cfg(feature = "async")] pub async fn local_async_serial_core_with_return( name: &str, - max_wait: Option, fut: impl std::future::Future>, ) -> Result<(), E> { - check_new_key(name, max_wait); + check_new_key(name); let unlock = LOCKS.get(name).expect("key to be set"); // _guard needs to be named to avoid being instant dropped @@ -44,12 +41,8 @@ pub async fn local_async_serial_core_with_return( #[doc(hidden)] #[cfg(feature = "async")] -pub async fn local_async_serial_core( - name: &str, - max_wait: Option, - fut: impl std::future::Future, -) { - check_new_key(name, max_wait); +pub async fn local_async_serial_core(name: &str, fut: impl std::future::Future) { + check_new_key(name); let unlock = LOCKS.get(name).expect("key to be set"); // _guard needs to be named to avoid being instant dropped @@ -85,7 +78,7 @@ mod tests { let c = barrier.clone(); threads.push(thread::spawn(move || { c.wait(); - check_new_key("foo", None); + check_new_key("foo"); { let unlock = local_locks.get("foo").expect("read didn't work"); let mutex = unlock.value(); @@ -113,7 +106,7 @@ mod tests { #[test] fn unlock_on_assert() { let _ = std::panic::catch_unwind(|| { - local_serial_core("assert", None, || { + local_serial_core("assert", || { assert!(false); }) }); diff --git a/serial_test/src/serial_file_lock.rs b/serial_test/src/serial_file_lock.rs index dc224f8..1e58003 100644 --- a/serial_test/src/serial_file_lock.rs +++ b/serial_test/src/serial_file_lock.rs @@ -1,9 +1,7 @@ -use std::time::Duration; - use crate::file_lock::make_lock_for_name_and_path; #[doc(hidden)] -pub fn fs_serial_core(name: &str, _max_wait: Option, path: Option<&str>, function: fn()) { +pub fn fs_serial_core(name: &str, path: Option<&str>, function: fn()) { let mut lock = make_lock_for_name_and_path(name, path); lock.start_serial(); function(); @@ -13,7 +11,6 @@ pub fn fs_serial_core(name: &str, _max_wait: Option, path: Option<&str #[doc(hidden)] pub fn fs_serial_core_with_return( name: &str, - _max_wait: Option, path: Option<&str>, function: fn() -> Result<(), E>, ) -> Result<(), E> { @@ -28,7 +25,6 @@ pub fn fs_serial_core_with_return( #[cfg(feature = "async")] pub async fn fs_async_serial_core_with_return( name: &str, - _max_wait: Option, path: Option<&str>, fut: impl std::future::Future>, ) -> Result<(), E> { @@ -43,7 +39,6 @@ pub async fn fs_async_serial_core_with_return( #[cfg(feature = "async")] pub async fn fs_async_serial_core( name: &str, - _max_wait: Option, path: Option<&str>, fut: impl std::future::Future, ) { @@ -64,14 +59,14 @@ mod tests { #[test] fn test_serial() { - fs_serial_core("test", None, None, || {}); + fs_serial_core("test", None, || {}); } #[test] fn unlock_on_assert_sync_without_return() { let lock_path = path_for_name("unlock_on_assert_sync_without_return"); let _ = panic::catch_unwind(|| { - fs_serial_core("foo", None, Some(&lock_path), || { + fs_serial_core("foo", Some(&lock_path), || { assert!(false); }) }); diff --git a/serial_test/tests/tests.rs b/serial_test/tests/tests.rs index 63213f8..add8b10 100644 --- a/serial_test/tests/tests.rs +++ b/serial_test/tests/tests.rs @@ -2,7 +2,7 @@ use serial_test::local_serial_core; #[test] fn test_empty_serial_call() { - local_serial_core("beta", None, || { + local_serial_core("beta", || { println!("Bar"); }); } diff --git a/serial_test_derive/src/lib.rs b/serial_test_derive/src/lib.rs index c8773e2..46d8f17 100644 --- a/serial_test_derive/src/lib.rs +++ b/serial_test_derive/src/lib.rs @@ -62,18 +62,6 @@ use std::ops::Deref; /// `test_serial_one` and `test_serial_another` will be executed in serial, as will `test_serial_third` and `test_serial_fourth` /// but neither sequence will be blocked by the other /// -/// For each test, a timeout can be specified with the `timeout_ms` parameter to the [serial](macro@serial) attribute. Note that -/// the timeout is counted from the first invocation of the test, not from the time the previous test was completed. This can -/// lead to [some unpredictable behavior](https://github.com/palfrey/serial_test/issues/76) based on the number of parallel tests run on the system. -/// -/// ```` -/// #[test] -/// #[serial(timeout_ms = 1000)] -/// fn test_serial_one() { -/// // Do things -/// } -/// ```` -/// /// Nested serialised tests (i.e. a [serial](macro@serial) tagged test calling another) are supported #[proc_macro_attribute] #[proc_macro_error] @@ -216,7 +204,6 @@ impl ToTokens for QuoteOption { enum Arg { Name(String), - Timeout(u64), } fn get_raw_args(attr: proc_macro2::TokenStream) -> Vec { @@ -226,25 +213,7 @@ fn get_raw_args(attr: proc_macro2::TokenStream) -> Vec { match attrs.remove(0) { TokenTree::Ident(id) => { let name = id.to_string(); - if name == "timeout_ms" { - match attrs.first() { - Some(TokenTree::Punct(p)) if p.as_char() == '=' && !attrs.is_empty() => { - attrs.remove(0); - if let TokenTree::Literal(lit) = attrs.remove(0) { - let millis = lit - .to_string() - .parse::() - .expect("Not a valid duration for Timeout"); - raw_args.push(Arg::Timeout(millis)); - } else { - panic!("Timeout argument must be a literal duration"); - } - } - _ => raw_args.push(Arg::Name(name)), - } - } else { - raw_args.push(Arg::Name(name)); - } + raw_args.push(Arg::Name(name)); } TokenTree::Literal(literal) => { let string_literal = literal.to_string(); @@ -275,7 +244,6 @@ fn get_raw_args(attr: proc_macro2::TokenStream) -> Vec { #[derive(Default, Debug)] struct Config { name: String, - timeout: Option, path: Option, } @@ -292,9 +260,6 @@ fn get_core_key(attr: proc_macro2::TokenStream) -> Config { Arg::Name(name) => { c.path = Some(name); } - Arg::Timeout(timeout) => { - c.timeout = Some(timeout); - } } } c @@ -305,12 +270,7 @@ fn local_serial_core( input: proc_macro2::TokenStream, ) -> proc_macro2::TokenStream { let config = get_core_key(attr); - let timeout = if let Some(t) = config.timeout { - quote! { ::std::option::Option::Some(::std::time::Duration::from_millis(#t)) } - } else { - quote! { ::std::option::Option::None } - }; - let args: Vec> = vec![Box::new(config.name), Box::new(timeout)]; + let args: Vec> = vec![Box::new(config.name)]; serial_setup(input, args, "local") } @@ -319,26 +279,14 @@ fn local_parallel_core( input: proc_macro2::TokenStream, ) -> proc_macro2::TokenStream { let config = get_core_key(attr); - let timeout = if let Some(t) = config.timeout { - quote! { Some(::std::time::Duration::from_millis(#t)) } - } else { - quote! { None } - }; - let args: Vec> = vec![Box::new(config.name), Box::new(timeout)]; + let args: Vec> = vec![Box::new(config.name)]; parallel_setup(input, args, "local") } fn fs_args(attr: proc_macro2::TokenStream) -> Vec> { let config = get_core_key(attr); - let timeout = if let Some(_t) = config.timeout { - panic!("Timeout is not supported for file_serial"); - // quote! { ::std::option::Option::Some(::std::time::Duration::from_millis(#t)) } - } else { - quote! { ::std::option::Option::None } - }; vec![ Box::new(config.name), - Box::new(timeout), if let Some(path) = config.path { Box::new(QuoteOption(Some(path))) } else { @@ -497,7 +445,7 @@ mod tests { let compare = quote! { #[test] fn foo () { - serial_test::local_serial_core("", :: std :: option :: Option :: None, || {} ); + serial_test::local_serial_core("", || {} ); } }; assert_eq!(format!("{}", compare), format!("{}", stream)); @@ -514,61 +462,12 @@ mod tests { let compare = quote! { #[test] pub fn foo () { - serial_test::local_serial_core("", :: std :: option :: Option :: None, || {} ); - } - }; - assert_eq!(format!("{}", compare), format!("{}", stream)); - } - - #[test] - fn test_serial_with_timeout() { - let attrs = vec![ - TokenTree::Ident(format_ident!("timeout_ms")), - TokenTree::Punct(Punct::new('=', Spacing::Alone)), - TokenTree::Literal(Literal::u8_unsuffixed(42)), - ]; - let input = quote! { - #[test] - fn foo() {} - }; - let stream = local_serial_core( - proc_macro2::TokenStream::from_iter(attrs.into_iter()), - input, - ); - let compare = quote! { - #[test] - fn foo () { - serial_test::local_serial_core("", :: std :: option :: Option :: Some (::std::time::Duration::from_millis(42u64)), || {} ); + serial_test::local_serial_core("", || {} ); } }; assert_eq!(format!("{}", compare), format!("{}", stream)); } - #[test] - fn test_serial_with_name_and_timeout() { - let attrs = vec![ - TokenTree::Ident(format_ident!("foo")), - TokenTree::Punct(Punct::new(',', Spacing::Alone)), - TokenTree::Ident(format_ident!("timeout_ms")), - TokenTree::Punct(Punct::new('=', Spacing::Alone)), - TokenTree::Literal(Literal::u8_unsuffixed(42)), - ]; - let input = quote! { - #[test] - fn foo() {} - }; - let stream = local_serial_core( - proc_macro2::TokenStream::from_iter(attrs.into_iter()), - input, - ); - let compare = quote! { - #[test] - fn foo () { - serial_test::local_serial_core("foo", :: std :: option :: Option :: Some (::std::time::Duration::from_millis(42u64)), || {} ); - } - }; - assert_eq!(format!("{}", compare), format!("{}", stream)); - } #[test] fn test_stripped_attributes() { let _ = env_logger::builder().is_test(true).try_init(); @@ -585,7 +484,7 @@ mod tests { #[test] #[something_else] fn foo () { - serial_test::local_serial_core("", :: std :: option :: Option :: None, || {} ); + serial_test::local_serial_core("", || {} ); } }; assert_eq!(format!("{}", compare), format!("{}", stream)); @@ -601,7 +500,7 @@ mod tests { let stream = local_serial_core(attrs.into(), input); let compare = quote! { async fn foo () { - serial_test::local_async_serial_core("", :: std :: option :: Option :: None, || async {} ).await; + serial_test::local_async_serial_core("", || async {} ).await; } }; assert_eq!(format!("{}", compare), format!("{}", stream)); @@ -646,7 +545,7 @@ mod tests { let compare = quote! { #[test] fn foo () { - serial_test::fs_serial_core("foo", :: std :: option :: Option :: None, None, || {} ); + serial_test::fs_serial_core("foo", None, || {} ); } }; assert_eq!(format!("{}", compare), format!("{}", stream)); @@ -670,7 +569,7 @@ mod tests { let compare = quote! { #[test] fn foo () { - serial_test::fs_serial_core("foo", :: std :: option :: Option :: None, ::std::option::Option::Some("bar_path"), || {} ); + serial_test::fs_serial_core("foo", ::std::option::Option::Some("bar_path"), || {} ); } }; assert_eq!(format!("{}", compare), format!("{}", stream)); diff --git a/serial_test_test/src/lib.rs b/serial_test_test/src/lib.rs index edf84d5..0150b90 100644 --- a/serial_test_test/src/lib.rs +++ b/serial_test_test/src/lib.rs @@ -106,14 +106,6 @@ mod tests { #[cfg(feature = "file_locks")] use serial_test::{file_parallel, file_serial}; - #[test] - #[serial(timeout_key, timeout_ms = 60000)] - fn demo_timeout_with_key() {} - - #[test] - #[serial(timeout_ms = 60000)] - fn demo_timeout() {} - #[test] #[serial] fn test_serial_no_arg() { From 34529be55b496a6887b4b250d9462d256649d2d4 Mon Sep 17 00:00:00 2001 From: Tom Parker-Shemilt Date: Mon, 12 Dec 2022 15:17:39 +0000 Subject: [PATCH 2/2] Fix use of start only in logging --- serial_test/src/code_lock.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/serial_test/src/code_lock.rs b/serial_test/src/code_lock.rs index 4afb01d..cf333e1 100644 --- a/serial_test/src/code_lock.rs +++ b/serial_test/src/code_lock.rs @@ -1,12 +1,11 @@ use crate::rwlock::{Locks, MutexGuardWrapper}; use dashmap::{try_result::TryResult, DashMap}; use lazy_static::lazy_static; -#[cfg(all(feature = "logging"))] +#[cfg(feature = "logging")] use log::debug; -use std::{ - sync::{atomic::AtomicU32, Arc}, - time::Instant, -}; +use std::sync::{atomic::AtomicU32, Arc}; +#[cfg(feature = "logging")] +use std::time::Instant; pub(crate) struct UniqueReentrantMutex { locks: Locks, @@ -56,9 +55,10 @@ impl Default for UniqueReentrantMutex { } pub(crate) fn check_new_key(name: &str) { + #[cfg(feature = "logging")] let start = Instant::now(); loop { - #[cfg(all(feature = "logging"))] + #[cfg(feature = "logging")] { let duration = start.elapsed(); debug!("Waiting for '{}' {:?}", name, duration);