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

Issue #512 send_command("monitoring") is doomed to fail #749

Merged
merged 1 commit into from
Aug 27, 2015

Conversation

danielbprice
Copy link
Contributor

I tried to work out how to add a test case for this condition but it was challenging since the current
tests don't really deal with disconnects/reconnects/etc. I have run some of my own tests with
this change in place and it seems to work but additional validation would be welcomed.

@blainsmith
Copy link
Contributor

Since we have updated the testing suite would you mind adding tests for this now? Then we can get this merged.

@danielbprice
Copy link
Contributor Author

I'll try to find some time to work on it soon.

@danielbprice
Copy link
Contributor Author

I added a test case, and confirmed that, without the fix in place, the bug is triggered by the test case. With the fix in place, the test is passing.

@blainsmith
Copy link
Contributor

Thanks for adding tests. @bcoe @erinspice care to take a quick look?

@bcoe
Copy link
Contributor

bcoe commented Aug 27, 2015

@danielbprice awesome contribution thank you!

bcoe added a commit that referenced this pull request Aug 27, 2015
Issue #512 send_command("monitoring") is doomed to fail
@bcoe bcoe merged commit 6b1e789 into redis:master Aug 27, 2015
@erinishimoticha
Copy link
Contributor

Great! Thanks, @danielbprice!

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.

4 participants