mockall icon indicating copy to clipboard operation
mockall copied to clipboard

Feature request: Allow manually verifying expectations without requiring mutability

Open Plebshot opened this issue 6 months ago • 9 comments

Problem

I'm having an issue with the fact that mock assertions are exclusively verified on drop. In particular, if the mock is behind an Arc that is used in a detached async task, a test won't fail as the panic doesn't propagate to the test thread. This is somewhat related to https://github.com/asomers/mockall/issues/461 and https://github.com/asomers/mockall/issues/396.

While this can be circumvented in some use cases by joining all tasks manually or using interior mutability for the mocked object, I fail to find a satisfying solution for my use case.

Proposal

Add a method like verify(&self) that allows checking all expectations without requiring mutability.

This method could be called on mocks behind an Arc, without having to worry about the logistics of where the last reference ends up dropping the mock.

Context

  • I'm working with an Actix HTTP server.
  • I have written a middleware that records metrics about a request
  • I use a mockable trait to test the middleware and other components interacting with the metrics system

Conceptional code:

use actix_web::{http::StatusCode, test::TestRequest, web, App, HttpResponse};
use std::sync::Arc;
use tokio::sync::mpsc;

struct RequestMetricsRecord {}

#[mockall::automock]
trait MetricsSystem: Send + Sync + 'static {
    fn record(&self, record: RequestMetricsRecord);
    // ...
}

#[derive(Clone)]
struct MetricsMiddleware {
    recorder: mpsc::Sender<RequestMetricsRecord>,
}

impl MetricsMiddleware {
    /// Creates a new factory for the actual middleware.
    ///
    /// This spawns a background task to collect the metrics for better response times.
    fn new(metrics: Arc<dyn MetricsSystem>) -> Self {
        let (recorder, mut receiver) = mpsc::channel(100);
        // Note: Task is detached because there is no logical point to join it.
        // It exits by itself once the last sender for the channel is dropped by the Actix server.
        tokio::spawn(async move {
            while let Some(record) = receiver.recv().await {
                metrics.record(record);
            }
        });
        Self { recorder }
    }
}

// Implement the actual middleware logic that records incoming HTTP requests.
// Omitted for brevity - simply sends metrics over the channel to the above background task.
// ...

#[actix_web::test]
async fn test_middleware_records_metrics() {
    // Setup a mock that expects one call to record with a 200 status code.
    let mut mock_metrics = MockMetricsSystem::new();
    mock_metrics
        .expect_record()
        .withf(|record| record.status == StatusCode::OK)
        .once()
        .return_const(());
    let shared_metrics = Arc::new(mock_metrics);

    // Configure a server that should trigger the middleware to call the mock.
    let app = actix_web::test::init_service(
        App::new()
            .wrap(MetricsMiddleware::new(shared_metrics.clone()))
            .route("/ok", web::get().to(HttpResponse::Ok)),
    )
    .await;

    // Make a request to trigger the middleware
    app.call(TestRequest::get().uri("/ok").to_request()).await.unwrap();
}

Note that even adding drop(app) manually, to ensure the actix server shuts down before the mock is dropped doesn't work here because the detached background task lives longer than the drop.

Potential Workarounds

  • Add a sleep to ensure detached tasks are cleaned up after dropping the server. Unreliable and undesirable.
  • Don't use a detached task in the first place. Then recording metrics would add to the response time of each HTTP request, which it doesn't has to.
  • Put MetricsSystem behind a Mutex to allow using checkpoint() on the mock. Not an acceptable performance overhead if it's just needed for tests.
  • Write a SyncMockMetricsSystem that wraps MockMetricsSystem in an Mutex and manually reimplements each method of the trait. Introduces boilerplate and might cause issues due to having a mutex only in tests.
  • Join all detached threads manually. Doesn't seem (easily) possible within the context of Actix, in particular because of async drop.
  • Use #[test] and create/drop the async runtime manually? Not entirely sure if that really solves the issue, but it would also introduce boilerplate for each test again.

If you have any other suggestions to solve this I'm happy to hear them.

Thanks for the great work on this library!

Plebshot avatar Aug 07 '24 14:08 Plebshot