asomers / mockall

A powerful mock object library for Rust
Apache License 2.0
1.5k stars 62 forks source link

References to mock objects that live for `static break expectation evaluation #456

Closed Will-Low closed 1 year ago

Will-Low commented 1 year ago

Description

From my understanding, expectations are checked on object drop. In cases where objects live for `static and are never dropped, this leads to expectations passing that shouldn't.

This fails, as expected, since my_method() is not being called 200 times:

use mockall::automock;

#[automock]
trait MyTrait {
    fn my_method(&self);
}

fn function_that_does_not_call_my_method<M>(mock: &MockMyTrait)
where M: MyTrait {
    println!("hello");
}

#[test]
fn my_method_is_called() {
    let mut mock = MockMyTrait::default();
    mock.expect_my_method().times(200);
    function_that_does_not_call_my_method::<MockMyTrait>(&mock);
}

However this passes, even though my_method() is not called 200 times:

use mockall::automock;

#[automock]
trait MyTrait {
    fn my_method(&self);
}

fn function_that_does_not_call_my_method<M>(mock: &MockMyTrait)
where M: MyTrait {
    println!("hello");
}

#[test]
fn my_method_is_called() {
    let mock = Box::leak(Box::new(MockMyTrait::default()));
    mock.expect_my_method().times(200);
    function_that_does_not_call_my_method::<MockMyTrait>(mock);
}

Proposals

  1. Is there a technical solution that would allow this to be handled by mockall?
  2. If not, can an error be raised when trying to do this?
  3. Would it make sense to add this exception to the documentation?
asomers commented 1 year ago

Your test fails for me, as expected, like this:

---- my_method_is_called stdout ----
hello
thread 'my_method_is_called' panicked at 'MockMyTrait::my_method: Expectation(<anything>) called 0 time(s) which is fewer than expected 200', mockall/tests/issue_456.rs:3:1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

How are you running it?

Will-Low commented 1 year ago

I'm just using cargo test.

Here's my output from the first version that I've confirmed is failing:

% cargo test
   Compiling test_doubles_with_mockall v0.1.0 (/Users/<REDACTED>/repos/test_doubles_with_mockall)
warning: unused variable: `mock`
 --> src/lib.rs:8:45
  |
8 | fn function_that_does_not_call_my_method<M>(mock: &MockMyTrait)
  |                                             ^^^^ help: if this is intentional, prefix it with an underscore: `_mock`
  |
  = note: `#[warn(unused_variables)]` on by default

warning: function `function_that_does_not_call_my_method` is never used
 --> src/lib.rs:8:4
  |
8 | fn function_that_does_not_call_my_method<M>(mock: &MockMyTrait)
  |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(dead_code)]` on by default

warning: `test_doubles_with_mockall` (lib) generated 2 warnings
warning: `test_doubles_with_mockall` (lib test) generated 1 warning (1 duplicate)
    Finished test [unoptimized + debuginfo] target(s) in 0.20s
     Running unittests src/lib.rs (/Users/<REDACTED>/repos/test_doubles_with_mockall/target/debug/deps/test_doubles_with_mockall-af9d419b6703c590)

running 1 test
test my_method_is_called ... FAILED

failures:

---- my_method_is_called stdout ----
hello
thread 'my_method_is_called' panicked at 'MockMyTrait::my_method: Expectation(<anything>) called 0 time(s) which is fewer than expected 200', src/lib.rs:3:1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    my_method_is_called

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

error: test failed, to rerun pass `--lib`

And here is the output from the second version that is not failing:

% cargo test
   Compiling test_doubles_with_mockall v0.1.0 (/Users/<REDACTED>/repos/test_doubles_with_mockall)
warning: unused variable: `mock`
 --> src/lib.rs:8:45
  |
8 | fn function_that_does_not_call_my_method<M>(mock: &MockMyTrait)
  |                                             ^^^^ help: if this is intentional, prefix it with an underscore: `_mock`
  |
  = note: `#[warn(unused_variables)]` on by default

warning: function `function_that_does_not_call_my_method` is never used
 --> src/lib.rs:8:4
  |
8 | fn function_that_does_not_call_my_method<M>(mock: &MockMyTrait)
  |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(dead_code)]` on by default

warning: `test_doubles_with_mockall` (lib) generated 2 warnings
warning: `test_doubles_with_mockall` (lib test) generated 1 warning (1 duplicate)
    Finished test [unoptimized + debuginfo] target(s) in 0.20s
     Running unittests src/lib.rs (/Users/<REDACTED>/repos/test_doubles_with_mockall/target/debug/deps/test_doubles_with_mockall-af9d419b6703c590)

running 1 test
test my_method_is_called ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

   Doc-tests test_doubles_with_mockall

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

I am using:

asomers commented 1 year ago

Oh, I missed the Box::leak part. In that case, the mock never gets dropped, as you observed. What you should do is add a mock.checkpoint() line at the end of your test.

Will-Low commented 1 year ago

I tried that workaround in my code (not the example here), but it continued to be a problem in my case, since the reference is passed to a new thread, which holds it for `static. At that point, it is no longer possible to call mock.checkpoint(), since .checkpoint() requires &mut self, but the thread still has an active reference to the mock.

asomers commented 1 year ago

It sounds like you need some interior mutability then. Perhaps pass a Arc<Mutex<MyMock>> to the other thread.

asomers commented 1 year ago

Closing this stale issue on the assumption that it's been adequately answered. No code change in Mockall is required.