From b4b28d8dc0467084964f0da8c8bf35161e8b4598 Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Sat, 9 Dec 2023 17:28:12 -0700 Subject: [PATCH] Change the recommendation for static methods Using a static method in two or more test cases requires synchronization, such as with a Mutex. Previously we recommended adding a `let _m = SOME_MTX.lock().unwrap()` to the top of each such test case. But if one test case should fail, then the others will fail too due to a poisoned mutex. It turns out that the `.unwrap()` is completely unnecessary; a poisoned Mutex still provides the necessary synchronization. So simply remove all of the unwraps. --- mockall/examples/synchronization.rs | 22 +++++---------- mockall/tests/automock_foreign_c.rs | 16 +++++------ mockall/tests/automock_module.rs | 16 +++++++---- ...neric_struct_with_generic_static_method.rs | 26 +++++++++--------- .../tests/mock_struct_with_static_method.rs | 27 ++++++++++--------- 5 files changed, 51 insertions(+), 56 deletions(-) diff --git a/mockall/examples/synchronization.rs b/mockall/examples/synchronization.rs index b9febe41..b12f844c 100644 --- a/mockall/examples/synchronization.rs +++ b/mockall/examples/synchronization.rs @@ -35,27 +35,19 @@ fn main() { mod test { use crate::my_mock::MockThing; use lazy_static::lazy_static; - use std::sync::{Mutex, MutexGuard}; + use std::sync::Mutex; lazy_static! { static ref MTX: Mutex<()> = Mutex::new(()); } - // When a test panics, it will poison the Mutex. Since we don't actually - // care about the state of the data we ignore that it is poisoned and grab - // the lock regardless. If you just do `let _m = &MTX.lock().unwrap()`, one - // test panicking will cause all other tests that try and acquire a lock on - // that Mutex to also panic. - fn get_lock(m: &'static Mutex<()>) -> MutexGuard<'static, ()> { - match m.lock() { - Ok(guard) => guard, - Err(poisoned) => poisoned.into_inner(), - } - } - #[test] fn test_1() { - let _m = get_lock(&MTX); + // The mutex might be poisoned if another test fails. But we don't + // care, because it doesn't hold any data. So don't unwrap the Result + // object; whether it's poisoned or not, we'll still hold the + // MutexGuard. + let _m = MTX.lock(); let ctx = MockThing::one_context(); ctx.expect().returning(|| 1); @@ -65,7 +57,7 @@ mod test { #[test] fn test_2() { - let _m = get_lock(&MTX); + let _m = MTX.lock(); let ctx = MockThing::one_context(); ctx.expect().returning(|| 2); diff --git a/mockall/tests/automock_foreign_c.rs b/mockall/tests/automock_foreign_c.rs index de0a2d2c..4eaebf07 100644 --- a/mockall/tests/automock_foreign_c.rs +++ b/mockall/tests/automock_foreign_c.rs @@ -8,10 +8,6 @@ use std::sync::Mutex; mod ffi { extern "C" { pub(super) fn foo(x: u32) -> i64; - // Every should_panic method needs to operate on a separate method so it - // doesn't poison other tests - #[allow(dead_code)] - pub(super) fn foo1(x: u32) -> i64; } } @@ -21,24 +17,26 @@ lazy_static! { // Ensure we can still use the original mocked function pub fn normal_usage() { + let _m = FOO_MTX.lock(); unsafe { ffi::foo(42); } } #[test] -#[should_panic(expected = "mock_ffi::foo1(5): No matching expectation found")] +#[should_panic(expected = "mock_ffi::foo(5): No matching expectation found")] fn with_no_matches() { - let ctx = mock_ffi::foo1_context(); + let _m = FOO_MTX.lock(); + let ctx = mock_ffi::foo_context(); ctx.expect() .with(predicate::eq(4)) .returning(i64::from); - unsafe{ mock_ffi::foo1(5) }; + unsafe{ mock_ffi::foo(5) }; } #[test] fn returning() { - let _m = FOO_MTX.lock().unwrap(); + let _m = FOO_MTX.lock(); let ctx = mock_ffi::foo_context(); ctx.expect().returning(i64::from); assert_eq!(42, unsafe{mock_ffi::foo(42)}); @@ -48,7 +46,7 @@ fn returning() { /// function pointer. #[test] fn c_abi() { - let _m = FOO_MTX.lock().unwrap(); + let _m = FOO_MTX.lock(); let ctx = mock_ffi::foo_context(); ctx.expect().returning(i64::from); let p: unsafe extern "C" fn(u32) -> i64 = mock_ffi::foo; diff --git a/mockall/tests/automock_module.rs b/mockall/tests/automock_module.rs index 8f362b3f..8f6f6c39 100644 --- a/mockall/tests/automock_module.rs +++ b/mockall/tests/automock_module.rs @@ -2,16 +2,20 @@ //! Mocking an entire module of functions #![deny(warnings)] + pub mod m { + use std::sync::Mutex; use mockall::*; type T = u32; + lazy_static! { + static ref BAR_MTX: Mutex<()> = Mutex::new(()); + } + #[automock] pub mod foo { use super::T; pub fn bar(_x: T) -> i64 {unimplemented!()} - // We must have a separate method for every should_panic test - pub fn bar1(_x: T) -> i64 {unimplemented!()} // Module functions should be able to use impl Trait, too pub fn baz() -> impl std::fmt::Debug + Send { unimplemented!()} // Module functions can use mutable arguments @@ -19,17 +23,19 @@ pub mod m { } #[test] - #[should_panic(expected = "mock_foo::bar1(5): No matching expectation found")] + #[should_panic(expected = "mock_foo::bar(5): No matching expectation found")] fn with_no_matches() { - let ctx = mock_foo::bar1_context(); + let _m = BAR_MTX.lock(); + let ctx = mock_foo::bar_context(); ctx.expect() .with(predicate::eq(4)) .return_const(0); - mock_foo::bar1(5); + mock_foo::bar(5); } #[test] fn returning() { + let _m = BAR_MTX.lock(); let ctx = mock_foo::bar_context(); ctx.expect() .returning(|x| i64::from(x) + 1); diff --git a/mockall/tests/mock_generic_struct_with_generic_static_method.rs b/mockall/tests/mock_generic_struct_with_generic_static_method.rs index bf6095ae..b01d38b2 100644 --- a/mockall/tests/mock_generic_struct_with_generic_static_method.rs +++ b/mockall/tests/mock_generic_struct_with_generic_static_method.rs @@ -8,10 +8,6 @@ use std::sync::Mutex; mock! { Foo { fn foo(t: T, q: Q) -> u64; - // We must use a different method for every should_panic test, so the - // shared mutex doesn't get poisoned. - fn foo2(t: T, q: Q) -> u64; - fn foo3(t: T, q: Q) -> u64; } } @@ -22,7 +18,7 @@ lazy_static! { // Checkpointing the mock object should not checkpoint static methods too #[test] fn checkpoint() { - let _m = FOO_MTX.lock().unwrap(); + let _m = FOO_MTX.lock(); let mut mock = MockFoo::::new(); let ctx = MockFoo::::foo_context(); @@ -36,9 +32,10 @@ fn checkpoint() { // It should also be possible to checkpoint just the context object #[test] #[should_panic(expected = - "MockFoo::foo2: Expectation() called 0 time(s) which is fewer than expected 1")] + "MockFoo::foo: Expectation() called 0 time(s) which is fewer than expected 1")] fn ctx_checkpoint() { - let ctx = MockFoo::::foo2_context(); + let _m = FOO_MTX.lock(); + let ctx = MockFoo::::foo_context(); ctx.expect::() .returning(|_, _| 0) .times(1..3); @@ -48,21 +45,22 @@ fn ctx_checkpoint() { // Expectations should be cleared when a context object drops #[test] -#[should_panic(expected = "MockFoo::foo3(42, 69): No matching expectation found")] +#[should_panic(expected = "MockFoo::foo(42, 69): No matching expectation found")] fn ctx_hygiene() { + let _m = FOO_MTX.lock(); { - let ctx0 = MockFoo::::foo3_context(); + let ctx0 = MockFoo::::foo_context(); ctx0.expect::() .returning(|_, _| 0); } - MockFoo::foo3(42, 69); + MockFoo::foo(42, 69); } #[cfg_attr(not(feature = "nightly"), ignore)] #[cfg_attr(not(feature = "nightly"), allow(unused_must_use))] #[test] fn return_default() { - let _m = FOO_MTX.lock().unwrap(); + let _m = FOO_MTX.lock(); let ctx = MockFoo::::foo_context(); ctx.expect::(); @@ -71,7 +69,7 @@ fn return_default() { #[test] fn returning() { - let _m = FOO_MTX.lock().unwrap(); + let _m = FOO_MTX.lock(); let ctx = MockFoo::::foo_context(); ctx.expect::() @@ -81,7 +79,7 @@ fn returning() { #[test] fn two_matches() { - let _m = FOO_MTX.lock().unwrap(); + let _m = FOO_MTX.lock(); let ctx = MockFoo::::foo_context(); ctx.expect::() @@ -96,7 +94,7 @@ fn two_matches() { #[test] fn with() { - let _m = FOO_MTX.lock().unwrap(); + let _m = FOO_MTX.lock(); let ctx = MockFoo::::foo_context(); ctx.expect::() diff --git a/mockall/tests/mock_struct_with_static_method.rs b/mockall/tests/mock_struct_with_static_method.rs index 7a507468..98598964 100644 --- a/mockall/tests/mock_struct_with_static_method.rs +++ b/mockall/tests/mock_struct_with_static_method.rs @@ -7,9 +7,6 @@ use std::sync::Mutex; mock!{ Foo { fn bar(x: u32) -> u64; - // We must have a separate method for every should_panic test - fn bar2(x: u32) -> u64; - fn bar3(x: u32) -> u64; } } @@ -20,7 +17,7 @@ lazy_static! { // Checkpointing the mock object should not checkpoint static methods #[test] fn checkpoint() { - let _m = BAR_MTX.lock().unwrap(); + let _m = BAR_MTX.lock(); let mut mock = MockFoo::new(); let ctx = MockFoo::bar_context(); @@ -34,9 +31,11 @@ fn checkpoint() { // It should also be possible to checkpoint just the context object #[test] #[should_panic(expected = - "MockFoo::bar2: Expectation() called 0 time(s) which is fewer than expected 1")] + "MockFoo::bar: Expectation() called 0 time(s) which is fewer than expected 1")] fn ctx_checkpoint() { - let ctx = MockFoo::bar2_context(); + let _m = BAR_MTX.lock(); + + let ctx = MockFoo::bar_context(); ctx.expect() .returning(|_| 32) .times(1..3); @@ -46,19 +45,21 @@ fn ctx_checkpoint() { // Expectations should be cleared when a context object drops #[test] -#[should_panic(expected = "MockFoo::bar3(42): No matching expectation found")] +#[should_panic(expected = "MockFoo::bar(42): No matching expectation found")] fn ctx_hygiene() { + let _m = BAR_MTX.lock(); + { - let ctx0 = MockFoo::bar3_context(); + let ctx0 = MockFoo::bar_context(); ctx0.expect() .returning(|x| u64::from(x + 1)); } - MockFoo::bar3(42); + MockFoo::bar(42); } #[test] fn return_const() { - let _m = BAR_MTX.lock().unwrap(); + let _m = BAR_MTX.lock(); let ctx = MockFoo::bar_context(); ctx.expect() @@ -70,7 +71,7 @@ fn return_const() { #[cfg_attr(not(feature = "nightly"), allow(unused_must_use))] #[test] fn return_default() { - let _m = BAR_MTX.lock().unwrap(); + let _m = BAR_MTX.lock(); let ctx = MockFoo::bar_context(); ctx.expect(); @@ -80,7 +81,7 @@ fn return_default() { #[test] fn returning() { - let _m = BAR_MTX.lock().unwrap(); + let _m = BAR_MTX.lock(); let ctx = MockFoo::bar_context(); ctx.expect() @@ -90,7 +91,7 @@ fn returning() { #[test] fn two_matches() { - let _m = BAR_MTX.lock().unwrap(); + let _m = BAR_MTX.lock(); let ctx = MockFoo::bar_context(); ctx.expect()