checkpoint() fails to validate if function is called 0 times or never

[vikz95]

Hi, I opened an issue about how to check if mockObject expectations are met directly from the main tokio test task (https://github.com/asomers/mockall/issues/461). The problem was that panics in subtasks are not propagated to the main task and the test succeeds when it should fail.

It was suggested to use mockObject.checkpoint()

My mockObject function is called once. If I change the expected times that it's called to a higher value, like times(2), when I invoke checkpoint() the test fails as it should. But if I change the value to times(0) or if I use never() the test succeeds..

Is this a bug? Or is it an expected behaviour because checkpoint() is intended to be called mid test, so if the number of times the function is actually called is less than the expected, it's fine because the test is not yet complete and the function could still be called?

If it's the second case I think that it could be useful to have a similar method intended to be used at the end of the tests.

[asomers]

If you use never() and call the function anyway, the function call should immediately panic. You shouldn't even reach the point where you call checkpoint. Perhaps it's panicing inside of the spawned tokio test and you aren't catching the panic again?

[vikz95]

Yeah, it's the same test from last time, so the panic happens in a spawned tokio subtask. Regarding the question, the fact that checkpoint() doesn't work in this case is a bug or it's the intended behaviour?

[asomers]

It's unintended, because I never imagined that anybody would use it after a panic. How are you sharing the object between tasks anyway? And how are you verifying that the spawned task does what it's supposed to? I would think that you'd need to ensure the spawned task doesn't panic, somehow.

[vikz95]

The object is wrapped in an Arc so it can be shared between tasks. The spawned tasks should call an external service, for this reason I need to mock the object. To ensure that the spawned task does what it's supposed to do I need to check that the function that calls the external service is invoked only once.

If the test is written properly (expect function is called only once) then the spawned task doesn't panic. But what if someone modifies the code and now the spawned task calls the external service twice. Then our test should fail. If the change was wanted the developer needs to update the test, otherwise the test did it's job and signaled that there is a bug.

For this reason I think that it could be useful to have a verify() method to use check the mocked object at the end of the test.

I never imagined that anybody would use it after a panic.

With multithreading this is no longer true because panics in the subtasks don't make the main task panic. Also we are not talking about a panic of the function that needs to be fixed, the panic is caused by mockall times() function only in the tests.

[asomers]

If the test is written properly (expect function is called only once) then the spawned task doesn't panic. But what if someone modifies the code and now the spawned task calls the external service twice. Then our test should fail.

Yeah, I get that. But what I'm suggesting is that any panic in the spawned task should count as a test failure. The panic needn't even be related to your mock objects. It's probably an oversight in your test design if panics can go unnoticed.

For this reason I think that it could be useful to have a verify() method to use check the mocked object at the end of the test.

What do you want the verify method to do? All expectations are already checked on drop.

[vikz95]

It's probably an oversight in your test design if panics can go unnoticed.

I don't know how the test design can be improved to notice panics in the spawned tasks. The only way that I know is to use tokio unstable features (https://docs.rs/tokio/latest/tokio/runtime/struct.Builder.html#method.unhandled_panic) to shutdown the runtime on panic.

But it's unstable and panics are not propagated to the caller task for a reason: don't shutdown the whole program if a panic occours in a task.

What do you want the verify method to do? All expectations are already checked on drop.

Don't assume that the panic made the test fail, because if it happened in a subtask then it will be ignored by the test and the test will succeed. So the verify method should check if the expectations are met, like checkpoint does, but even when the expected number of calls is less than the actual number of calls.

[asomers]

I think what you really want is for checkpoint to work post-panic, right? That wouldn't require a new method.

[vikz95]

Ok, it's fine to extend the capabilities of checkpoint() instead of creating a new method.

[asomers]

@vikz95 would you be willing to take a stab at this? It should be easy. You won't even have to modify the proc macro. Just change Times::is_satisfied.

[vikz95]

Ok, I downloaded the dependency in my project path, so I'll also be able to test the change directly from my code.

[vikz95]

I don't know exactly what you had in mind with the is_satisfied function. However my approach is to add a Times::maximum() function:

impl Times {
    pub fn call(&self) -> Result<(), String> {
        let count = self.count.fetch_add(1, Ordering::Relaxed) + 1;
        if count >= self.range.0.end {
            if self.range.0.end == 1 {
                Err("should not have been called".to_owned())
            } else {
                Err(format!(
                    "called {} times which is more than the expected {}",
                    count,
                    self.range.0.end - 1
                ))
            }
        } else {
            Ok(())
        }
    }

    ...

    /// Has this expectation already been called the minimum required number of
    /// times?
    pub fn is_satisfied(&self) -> bool {
        self.count.load(Ordering::Relaxed) >= self.range.0.start
    }

+    /// The maximum number of times that this expectation must be called
+   pub fn maximum(&self) -> usize {
+        self.range.0.end - 1
+    }

    /// The minimum number of times that this expectation must be called
    pub fn minimum(&self) -> usize {
        self.range.0.start
    }

And then perform again the check that Times::call() does in mockall_derive/src/mock_function.rs:

impl #ig Drop for Common #tg #wc {
                fn drop(&mut self) {
                    if !::std::thread::panicking()
                    {
                        if (!self.times.is_satisfied())
                        {
                            let desc = std::format!(
                            "{}", self.matcher.lock().unwrap());
                        panic!("{}: Expectation({}) called {} time(s) which is fewer than expected {}",
                               #funcname,
                               desc,
                               self.times.count(),
                               self.times.minimum());
                        }
+                        if (self.times.count() > self.times.maximum())
+                        {
+                            panic("Exceeded number of expected calls panic propagation.")
+                        }
                    }
                }
            }

If the actuals calls are more than the expected, Times::call() will panic and the message will be printed. But when the panic happens inside a sub task, then the additional check in the drop() function will make the test fail, and both panic messages will be printed.

Tomorrow I'll try to create a pull request.

[asomers]

Just change the is_satisfied to make it check both sides of the range. No need for the new method. You'll have to tweak the panic message, though.

[asomers]

Addressed by #472.