testing: `MockConnection` helper for testing clients without a server #465

tdyas · 2021-03-16T07:26:53Z

This PR adds MockConnection which implements crate::connection::ConnectionLike and crate::aio::ConnectionLike to appear like a client without the need to make any network requests. It is initialized with a list of MockCmd instances which wrap the expected commands and responses. This allows users to test their clients without much overhead.

The testing module is behind the testing feature which is non-default. Users should activate it in the dev-dependencies of their crate's Cargo.toml.

tdyas · 2021-03-16T07:41:19Z

~~Open question: What would be the suggested way to test Pipeline with MockConnection?~~

tdyas · 2021-05-19T09:36:24Z

I cleaned up the PR, added module docs with example, and made sure to test by enabling the aio feature. This should be ready for review. (I am using a variant of this code locally.)

tdyas · 2021-06-08T19:00:00Z

cc @Marwes -- It was unclear to me how to flag a PR for attention so decided to cc you here. Is there a better way to go about getting attention for a PR? Are you the right person?

tdyas · 2022-05-18T04:48:33Z

Rebased on latest main and force pushed.

djc · 2022-05-23T12:23:01Z

Cargo.toml

@@ -68,6 +68,7 @@ script = ["sha1_smol"]
 tls = ["native-tls"]
 async-std-comp = ["aio", "async-std"]
 async-std-tls-comp = ["async-std-comp", "async-native-tls", "tls"]
+testing = ["futures"]


Is this intended to be part of the public API? What's the use case for doing this?

Yes, it is intended to be part of the public API so that users (like me) can incorporate these test helpers into the unit tests in our codebases. I have made it be a feature here (which would be enabled as a dev dependency), but it could also be put into a separate crate, much like Tokio has done with the tokio-test crate. Thoughts?

A separate crate would allow evolving the API separately from the rest of the crate's API. I suppose we could keep the feature unstable for now (as in, we don't guarantee the public API to be compatible across semver-compatible versions of the crate).

I will restructure this PR to create a redis-test crate. Is that acceptable?

@jaymell what do you think? I'm just worried the API might be unstable, but maybe it's not so bad?

Hmm, I would say that a separate crate certainly does sound like an ideal solution, though the additional maintenance costs there do temper my enthusiasm somewhat 😅

I'll switch to separate crate then (but the code will still live in this repo).

djc · 2022-05-23T12:23:51Z

src/testing.rs

+#[cfg(feature = "aio")]
+use crate::aio::ConnectionLike as AioConnectionLike;
+
+/// Helper trait for converting test values into a `redis::Value` returned from a


How/why is ToRedisArgs/FromRedisValue different from what we need here?

Neither ToRedisArgs nor FromRedisValue produces redis::Value instances from non-Redis values as required by the test helper:

ToRedisArgs cannot be used because it is designed to convert a value into an array of bytes and not convert to redis::Value. (ToRedisArgs.to_redis_args returns Vec<Vec<u8>> and ToRedisArgs.write_redis_args wants a RedisWrite impl which has methods like write_arg which takes a &[u8]).

FromRedisValue is irrelevant because it converts in the opposite direction than what is needed by the test helper. The test helper needs to convert from a non-Redis type to redis::Value. FromRedisValue converts from redis::Value to a non-Redis type.

djc · 2022-05-24T08:35:45Z

Cargo.toml

@@ -10,6 +10,9 @@ documentation = "https://docs.rs/redis"
 license = "BSD-3-Clause"
 edition = "2018"

+[workspace]


I tend to find nested crate workspaces confusing to navigate. Would you mind changing this so that the top-level Cargo.toml only contains the workspace, and move the redis crate into a redis directory?

Should that be done in a separate PR?

(to avoid confusing commit history)

In my mind the ideal way to submit it would be a single PR with clean commits: one commit that moves the code around and one commit that adds the new crate. Each commit should ideally pass CI, though I usually don't test that explicitly (but it sucks if formatting changes for one commit make it into another).

So it sounds like that PRs are not squashed before merge then. Will do as separate commits in this PR then. (Context: My company merges via squash-and-merge generally for PRs so a separate PR would be the only way to accomplish having separate commit history in that case.)

I'm also okay with separate PRs if you prefer. For my own changes I usually submit clean commits "manually" (squashing with interactive rebase). If a PR only contains one logical change, I'll also squash via merge in the Git UI, but I don't like doing that for changes that contain multiple logical changes.

djc · 2022-05-24T08:36:25Z

redis-test/Cargo.toml

@@ -0,0 +1,18 @@
+[package]
+name = "redis-test"
+version = "0.21.5"


Let's just call this 0.1.0.

djc · 2022-05-24T08:36:41Z

redis-test/Cargo.toml

+license = "BSD-3-Clause"
+
+[dependencies]
+redis = { path = ".." }


If we're going to publish this it should also have a specific version.

I added a version, but kept the path so it continues to work with in-repo builds. Tokio does something similar.

redis-test/Cargo.toml

tdyas · 2022-05-30T22:21:59Z

I rebased the PR and turned it into two commits: the first moves the existing redis crate sources into a redis subdir, then second commit introduces the redis-test crate.

tdyas · 2022-05-30T22:44:40Z

The CI tests with Rust 1.51.0 fails due to async-std dependency async-global-executor using 2021 Edition.

djc

If you rebase this, I'd be okay with merging this.

Style nit: I think we could just git mv mock_conn.rs lib.rs, it doesn't make much sense to me to have the indirection in this case since everything lives in one module anyway.

tdyas · 2022-07-04T15:53:44Z

Rebased and also moved the contents of redis_test::mock_conn into just redis_test.

tdyas · 2022-07-04T16:00:58Z

Repushed after making sure subsequent updates to Cargo.toml were taken into account.

tdyas · 2022-07-04T16:19:04Z

Note: The lint job in CI is failing on existing code, and not the new redis-test crate. https://github.com/redis-rs/redis-rs/runs/7183833699?check_suite_focus=true#step:6:40

djc · 2022-07-05T08:28:11Z

@jaymell any thoughts before we merge this?

I'll take care of the new clippy issues separately.

tdyas mentioned this pull request Mar 16, 2021

Provide fake connection for testing? #455

Closed

tdyas force-pushed the mock_connection branch from 62cf818 to d93a70f Compare May 19, 2021 09:34

tdyas force-pushed the mock_connection branch from a372475 to 726d6df Compare June 8, 2021 18:21

tdyas force-pushed the mock_connection branch from 726d6df to 11306ef Compare May 18, 2022 04:41

djc reviewed May 23, 2022

View reviewed changes

tdyas force-pushed the mock_connection branch 2 times, most recently from 873fa5b to d108f46 Compare May 24, 2022 05:19

djc reviewed May 24, 2022

View reviewed changes

tdyas force-pushed the mock_connection branch from d108f46 to 86689d7 Compare May 30, 2022 22:21

tdyas force-pushed the mock_connection branch 2 times, most recently from aa0e40f to 340debd Compare May 30, 2022 22:33

tdyas force-pushed the mock_connection branch from 340debd to 339952e Compare May 31, 2022 01:11

djc approved these changes Jul 4, 2022

View reviewed changes

tdyas force-pushed the mock_connection branch from 657bdc2 to 6ef6d2f Compare July 4, 2022 15:53

tdyas force-pushed the mock_connection branch from 6ef6d2f to 97b5530 Compare July 4, 2022 16:00

Tom Dyas added 2 commits July 13, 2022 21:32

existing crate to redis subdir

1602199

testing module with mock redis connection and command

5b840c1

tdyas force-pushed the mock_connection branch from 97b5530 to 5b840c1 Compare July 14, 2022 01:33

jaymell merged commit f7409d0 into redis-rs:main Jul 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testing: `MockConnection` helper for testing clients without a server #465

testing: `MockConnection` helper for testing clients without a server #465

tdyas commented Mar 16, 2021

tdyas commented Mar 16, 2021 •

edited

tdyas commented May 19, 2021

tdyas commented Jun 8, 2021 •

edited

tdyas commented May 18, 2022

djc May 23, 2022 •

edited

tdyas May 23, 2022

djc May 23, 2022

tdyas May 23, 2022

djc May 23, 2022

jaymell May 23, 2022

tdyas May 23, 2022

djc May 23, 2022

tdyas May 23, 2022 •

edited

djc May 24, 2022

tdyas May 24, 2022

tdyas May 24, 2022

djc May 24, 2022

tdyas May 24, 2022

djc May 24, 2022 •

edited

djc May 24, 2022

tdyas May 24, 2022

djc May 24, 2022

tdyas May 30, 2022

tdyas commented May 30, 2022

tdyas commented May 30, 2022

djc left a comment

tdyas commented Jul 4, 2022

tdyas commented Jul 4, 2022

tdyas commented Jul 4, 2022

djc commented Jul 5, 2022

testing: MockConnection helper for testing clients without a server #465

testing: MockConnection helper for testing clients without a server #465

Conversation

tdyas commented Mar 16, 2021

tdyas commented Mar 16, 2021 • edited

tdyas commented May 19, 2021

tdyas commented Jun 8, 2021 • edited

tdyas commented May 18, 2022

djc May 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdyas May 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djc May 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdyas commented May 30, 2022

tdyas commented May 30, 2022

djc left a comment

Choose a reason for hiding this comment

tdyas commented Jul 4, 2022

tdyas commented Jul 4, 2022

tdyas commented Jul 4, 2022

djc commented Jul 5, 2022

testing: `MockConnection` helper for testing clients without a server #465

testing: `MockConnection` helper for testing clients without a server #465

tdyas commented Mar 16, 2021 •

edited

tdyas commented Jun 8, 2021 •

edited

djc May 23, 2022 •

edited

tdyas May 23, 2022 •

edited

djc May 24, 2022 •

edited