asomers / mockall

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

Check mock objects expectations in test main thread #461

Closed vikz95 closed 1 year ago

vikz95 commented 1 year ago

Hello, I already opened a StackOverflow question about my issue: How to make tokio test fail if thread panicked? But I wanted to ask also here for suggestions.

If an expectation, like the number of times a function is called, is not met the code panics and the test should fail. But if the mocked object is passed to another task the panic happens in a separate thread, it is not propagated to the main thread and the test succeeds.

Is there a way to handle these situations with mockall? If not, wouldn't be useful to add a verify method like those present in the Java library mockito? Something like: verify(mockedObject, times(1)).calledMethod(); So the check would be placed at the end of the test logic and would be executed in the main test thread, instead of an automatic check in the thread where the function is called.

asomers commented 1 year ago

I read your stack overflow post, and I think you're leaving something out. Panic that happen within a function marked #[tokio::test] should definitely cause a test failure. I suspect that something else in your code is starting a new thread, or spawning a new tokio task. Could you post a complete working example? Also, you can use mockedObject.checkpoint() to verify that all of its expectations have been satisfied prior to dropping the object.

vikz95 commented 1 year ago

You are right, I didn't mention that another tokio task is spawned and that the mocked object is called in this other task. However by using the suggested checkpoint() function I was able to make the test fail. Thank you for the support.

P.S.: I'll update my StackOverflow question and provide an answer.

asomers commented 1 year ago

Glad to help.

hpenne commented 1 year ago

I'm having a similar problem. The mock is called upon by code running in a spawned task. The application is such that there is no need to ever join this task. The task running the tokio test completes while the background task is still running, so the test completes before the mock is dropped.

My problem is that I cannot call the checkpoint function, because the mock object is moved into the spawned task.

The function parameters doing the move are typed as "impl MyTrait". Wrapping the mock object in an Arc<Mutex<>> would make it possible to call checkpoint() from the test, but that will require manually writing boilerplate to implement MyTrait for a wrapper type, and manually writing such boilerplate code is just what a mocking framwork is supposed to help us avoid. It would be possible to change the moved type to an Arc instead but that is not necessary in production, so it should be avoided.

Another solution is to add (otherwise unnecessary) logic to the production code to signal the background task to stop and then join it. A call for this must be made explicitly in the unit tests, as Rust cannot call async code in Drop implementations. The fact that one must always remember to add this extra call in the unit tests is inconvenient and error prone. Support for Async Drop may solve this some time in the future.

This issue is perhaps not the mocking framework's responsibility, but it certainly would be great if mockall could help solve it somehow. It's probably going to be a recurring problem in tokio-based applications.

asomers commented 1 year ago

It sounds like you have a much larger problem than just checking that your mock object's expectations are satisfied. If your background task is important to this test, yet it never gets joined or observed in any other way, then that's a significant test escape. It could even panic and your test would still pass. And fixing that test escape would catch any expectation failure, but observing any mock's panic during Drop. There isn't anything that Mockall can do to ensure that your expectations got satisfied after the test's main thread returns, because at that point the test's status is fixed and there's no way to change it.

hpenne commented 1 year ago

It sounds like you have a much larger problem than just checking that your mock object's expectations are satisfied. If your background task is important to this test, yet it never gets joined or observed in any other way, then that's a significant test escape. It could even panic and your test would still pass. And fixing that test escape would catch any expectation failure, but observing any mock's panic during Drop. There isn't anything that Mockall can do to ensure that your expectations got satisfied after the test's main thread returns, because at that point the test's status is fixed and there's no way to change it.

Yes and no. The task very much gets "observed in any other way", and observable effects of the task are tested thoroughly. Injecting panics in it does in fact cause the tests to fail (I have tested extensively). I think tokio tests run on the Current-Tread Scheduler, but this is only run until the test function completes. Any panics in background tasks up until that point are caught, but other tasks do not get polled after. The special problem with the use of mockall with tokio in this way is that the background task that owns the mock gets dropped after the test function has completed. The panic probably arises when the scheduler is dropped, and tokio does not fail the test on panics at that point.

I do agree that this is not strictly your problem. Ideally, tokio should fail tests on panics that happen when dropping the Current-Tread Scheduler, to better support mockall and other mock framworks working in similar ways.

I can fix it in our design by having the background task use channels instead of calling on a Trait. That makes these events waitable, which solves the problem. It will be a little more code, but will eliminate mockall from our dependencies.

nastynaz commented 11 months ago

I have this exact same problem. The mock is moved to another struct, which is then tokio::spawned in another thread which runs loops indefinitely. It eats messages and passes them to an actor (Actor model).

However none of the mock expectations are getting checked and the test passes when it fails. I cannot call mock.checkpoint() at the end because it's already been moved.

Has anyone found a workaround for this? Would greatly appreciate any suggestions.

asomers commented 11 months ago

I suggest the same solution: join that thread at the end of your test. If you don't, then other types of panics may go undetected too, not just from Mockall.

nastynaz commented 11 months ago

@asomers I have a thread that loops like this and never returns. If I join it, the test never finishes. How can I test it? Alternatively, if you're up for pair programming to help me iron out and fix the issue I'm happy to offer $100/hr.

async fn run_mid_price_policy<M: MidPricePolicyTrait>(
    mut actor: M,
    mut base_quote_update_receiver: BaseQuoteUpdateReceiver,
) {
    while let Some(msg) = base_quote_update_receiver.recv().await {
        actor.handle_base_quote_updated(msg).await
    }
}
asomers commented 11 months ago

Your offer is tempting @nastynaz , but let me offer some easy suggestions first:

Let me also share an anecdote: At a previous job, we had a program written in Java. This program spawned a number of background threads, for various purposes, that were intended to run for the lifetime of the process. And since the program wasn't designed for testability, there was no way to kill and join those threads. Meanwhile, an extensive test suite started the body of the application for many different test cases. Each of those test cases therefore spawned and orphaned several new threads. One day, CI began to fail because the test process reached the limit of threads per process (1500). There was much arguing between the Java developer ("This is obviously an OS defect, because Windows has a higher default limit") and the OS team ("Fix your damn architecture"). In the end, we raised the limit and the Java developer hired a contractor to write a ThreadLeakHunter , which would run throughout the test suite and attempt to detect which threads had been orphaned and kill them. Later, the Java developer quit when he was passed over for promotion. I never did find out if they fixed the thread leak.

And that's why you need to design for testability.

nastynaz commented 11 months ago

@asomers I appreciate your suggestion. In the end I managed to fix it by 'designing for testability' as you said. The fix here wasn't that hard in the end: convert the white let to a loop which breaks when None is received (sender is dropped). Then spawn the task in a new tokio thread, send my inputs via the sender, drop the sender (to return from the method), then .await the spawned task. Because the task completes and was joined the mock object runs all of its assertions.

Thanks for making such a useful library!

asomers commented 11 months ago

Glad you got it to work!