UnitTest: make SimpleCacheClient mockable

[ymwjbxxq]

Hello Team,

It is impossible to Mock or create a Fake of the SimpleCacheClient (at least for me).

The client is initialised in this way:

    let credential_provider =
        CredentialProviderBuilder::from_environment_variable("MOMENTO_AUTH_TOKEN".to_string())
            .build()?;
    let cache_client =
        SimpleCacheClientBuilder::new(credential_provider, Duration::from_secs(500))?
            .build();

When writing code, you should be able to Mock the client or replace it.

If you create a trait with https://crates.io/crates/async-trait, it is possible to replace it or Mock it with a library like https://crates.io/crates/mockall.

With this, the code is testable if you write integration tests that are pretty expensive to run each time.

Thanks,

[kvcache]

Thanks @ymwjbxxq! We're talking about what would be best to do here. It seems reasonable to have a trait here, but we'll need to weigh the cost of dynamic dispatch here (BoxFuture is still required for trait async methods on stable rust) versus the testing flexibility.

[cprice404]

@kvcache do the async trait features in rust 1.75.0 mean that we can now have a trait for the cache client without dynamic dispatch?

[cprice404]

@ymwjbxxq we started looking into this and I wanted to check back in with you.

We could theoretically make a trait for the cache client, but it would have a ton of functions on it, and the list will keep growing over time. It feels like that might not actually be desirable to "mock" for testing purposes because you'd have to implement so many fns, and any time we added a new one, your test compilation would break.

I was thinking about how I might try to do mock testing around this, and wondering if it might make more sense to make a small wrapper in your own application that abstracts the cache client. Then you could have a trait for that, with only the functions that you wanted to be able to mock during tests, and it might have a much smaller surface area and be easier to maintain.

Very interested in any more thoughts/ideas on this though!

[ymwjbxxq]

Of course, your suggestion is correct, and I can do an integration test, but regardless of your implementation decision, I wonder about this:

It feels like that might not actually be desirable to "mock" for testing purposes because you'd have to implement so many fns, and any time we added a new one, your test compilation would break.

How the AWS SDK Rust does it? When I mock DynamoDB, I replace the client and I do not need to mock all.
This is the unit test

let request = UnitTestHelper::dynamodb_request_builder();
        let request = request
            .header("x-amz-target", "DynamoDB_20120810.GetItem")
            .body(SdkBody::from(
                r##"{
                      "TableName":"some-table",
                      "Key":{"myKey":{"S":"xxx"}},
                      "ProjectionExpression":"field1, field2"
                    }"##,
            ))
            .unwrap();
        let response = Response::builder()
            .status(200)
            .body(SdkBody::from(
                r#"{
                  "Item": {
                    "field1": {"S": "xxx"}, 
                    "field2": {"S": "xxxx"}
                    }
                  }"#,
            ))
            .unwrap();

let conn = UnitTestHelper::buil_test_connnection(request, response);
let dynamo_db_client = UnitTestHelper::dynamo_fake_client(&conn);

let query = MyQuery::builder()
            .table_name("some-table".to_owned())
            .dynamo_db_client(dynamo_db_client)
            .build();

The UnitTestHelper is

pub fn buil_test_connnection(
        http_request: Request<SdkBody>,
        http_response: Response<SdkBody>,
    ) -> StaticReplayClient {
        StaticReplayClient::new(vec![
            // Event that covers the first request/response
            ReplayEvent::new(http_request, http_response), // The next ReplayEvent covers the second request/response pair...
        ])
    }
    
 pub fn dynamodb_request_builder() -> http::request::Builder {
        http::Request::builder()
            .header("content-type", "application/x-amz-json-1.0")
            .uri(http::uri::Uri::from_static(
                "https://dynamodb.eu-central-1.amazonaws.com/",
            ))
    }

 pub fn dynamo_fake_client(conn: &StaticReplayClient) -> aws_sdk_dynamodb::Client {
        let credential = aws_sdk_dynamodb::config::Credentials::new(
            "ATESTCLIENT",
            "astestsecretkey",
            Some("atestsessiontoken".to_string()),
            None,
            "",
        );

        aws_sdk_dynamodb::Client::from_conf(
            aws_sdk_dynamodb::Config::builder()
                .behavior_version(BehaviorVersion::latest())
                .credentials_provider(credential)
                .region(aws_sdk_dynamodb::config::Region::new("eu-central-1"))
                .http_client(conn.clone())
                .build(),
        )
    }

Either way, I am fine if you leave it ununit-testable, and I can always do integration tests.

[cprice404]

Thanks @ymwjbxxq - I hadn't seen the StaticReplayClient before, that is a cool idea. We can look into that.

https://docs.aws.amazon.com/sdk-for-rust/latest/dg/testing.html#testing-replay