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

Reorganize test #10

Closed
joelcox opened this issue May 3, 2012 · 11 comments
Closed

Reorganize test #10

joelcox opened this issue May 3, 2012 · 11 comments
Assignees

Comments

@joelcox
Copy link
Owner

joelcox commented May 3, 2012

I just migrated the unit test controller to PHPUnit, but the quality of these tests is seriously lacking. These tests are rather integration tests (black box approach) rather than proper unit tests.

@joelcox
Copy link
Owner Author

joelcox commented May 4, 2012

I wrote some thorough tests for _encode_request in 0c52347, but _read_request() is currently untestable on its own. Some smoke tests are still in place, which I fleshed out a bit.

@ghost ghost assigned joelcox May 4, 2012
@tinkertim
Copy link
Collaborator

I just threw a DEBUG SEGFAULT, we need to handle the socket closing unexpectedly. Ouch.

@joelcox
Copy link
Owner Author

joelcox commented May 5, 2012

How can this be reproduced? Shutting down redis-server in the middle of a request?

@tinkertim
Copy link
Collaborator

By closing the socket randomly if a variable is passed to the constructor (which would only happen in test cases). Otherwise, it's just a check against NULL, perhaps if some constant is defined.

@tinkertim
Copy link
Collaborator

Redis not working is recoverable in some instances. I need to have a think about how to handle it, and the 'how' could be tested :)

@joelcox
Copy link
Owner Author

joelcox commented May 5, 2012

So by creating multiple instances of the Redis class? Sorry, I'm not following you.

@tinkertim
Copy link
Collaborator

Sorry, this would be more work than worth it. What I was hoping to be able to do is handle (gracefully) instances where Redis just dies. You can see it's a bit icky by adding a temporary method to close the socket midway through, so I thought of adding a user defined callback (also handy for testing) that could tell an app to stop using Redis.

More overhead and work than it's really worth at present especially for a library that implements the entire command set without any actual concrete methods :)

@tinkertim
Copy link
Collaborator

I'll add proper tests for transactions shortly (though those are mostly integration tests), I need to tickle WATCH during transactions as well. I'm quite happy at this point, everything 'just works' :)

@joelcox
Copy link
Owner Author

joelcox commented May 6, 2012

Haha. Great job.

@tinkertim
Copy link
Collaborator

Well, I went through and tried various commands in various scenarios differently and everything works (or fails) as expected. But really, all we're doing beyond what we have is testing Redis itself, which Redis does quite a good job of doing already :)

I can properize a simple test for transactions if you really want to include them. But honestly, the tests we have seem sufficient. We'll know quickly if a change broke encoding / decoding of the wire protocol .. anything else is really just exercising Redis.

@joelcox
Copy link
Owner Author

joelcox commented May 7, 2012

Agreed. I think we covered most (if not all) the encoding cases. Everything else is beyond our responsibility. I'll close this for now.

@joelcox joelcox closed this as completed May 7, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants