Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove TestContext and improve test connection handling #143

Merged
merged 2 commits into from
Aug 23, 2021
Merged

Remove TestContext and improve test connection handling #143

merged 2 commits into from
Aug 23, 2021

Conversation

rukai
Copy link
Member

@rukai rukai commented Aug 20, 2021

Closes #133 (sorry connor 😢 )

I changed the retry logic to just check if we can create a simple TCP socket.
This drastically reduces the complexity and should also allow the check to work for both regular and TLS connections.

I changed the retry timeout from an increasing timeout to a regular 100ms timeout, this makes more sense to me as we want the test to finish as soon as possible and dont need to worry about overwhelming production systems with too many retries.

I opted to move the FLUSHDB into the test cases that need it.
My reasoning is:

  • It means I can just pass around a connection without each sub test case needing to create their own connection.
  • Kuangda's auth changes need to work around the mandatory FLUSHDB and I think it would be cleaner to just call it when needed instead of disabling the call https://github.com/shotover/shotover-proxy/pull/101/files#diff-fc3e305604446c15140a8ab1db003428ad97e4c01fcc7ac7cbfbb4c8ad376595R88
  • Usually clearing the DB is critical between tests and shouldnt be left to the developer, but in this case the DB is actually cleared between tests as a new docker container is spun up for each test. Additionally each sub test case is always run in the same order, so failures here due to left over DB data will be deterministic and easy to identify.

An alternative to adding FLUSHDB's around is to create an fn run_sub_test_case(&[Func], connection: &mut Connection) that runs FLUSHDB before running each test case. But I think it would be simpler not to do that.

This is a prereq to #141 as I will need to add an fn redis_connection_tls to ShotoverManager and I also need to be able to call the test cases with an arbitrary redis::Connection.

@rukai rukai changed the title Refactor out TestContext and improve test connection handling Remove TestContext and improve test connection handling Aug 20, 2021
@rukai rukai requested a review from benbromhead August 23, 2021 00:15
@benbromhead
Copy link
Member

LGTM!

@benbromhead benbromhead merged commit 7f31011 into shotover:main Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor TestContext into ShotoverManager
2 participants