An failing test to demonstrate #732

[pierredavidbelanger]

No description provided.

[pierredavidbelanger]

My last commit should fix the problem.

I do not really know what I am doing here (I am not really good with Guice), I just had a feeling that the Provider returned Jedis instance should not be a singleton/reused, we should let the Provider create an instance each time, and the application will .close() them.

I may be wrong, in which case I really do not know why the test case is working now :)

[pierredavidbelanger]

In fac, the provider will not create an instance each time, it will pool.getResource() when asked for Jedis.class, this instance will not be reused (since not a singleton anymore), so when one call .close() on it (or using it in a try-with-resources) it will be correctly returned to the pool.

I am pretty sure this is why it works now.

The travis build is still failing though, I did not check why, but I guess I was wrong to assume you had a redis instance up during the test :)

[pierredavidbelanger]

The build is failing because in 3 tests, RedisTest is expecting AnnotatedBindingBuilder.asEagerSingleton() to be called.

I will fix the tests.

[jknack]

Same here :)

Thank you.

[jknack]

You're welcome to join the team :)

[pierredavidbelanger]

I am sorry, I do not know how to properly fix this test (without being an integration test).

I just commented some lines so not actual connection will be made, but this will render the test useless.

If you have some idea to properly fix this test, tel me and I will try to push again to this branch, otherwise, I will leave the code like this and it will be up to you to delete the class or make it work properly.

The build is failing because to redis instance can be reached on localhost:6379:

Caused by: java.net.ConnectException: Connection refused (Connection refused)
	at java.net.PlainSocketImpl.socketConnect(Native Method)
	at java.net.AbstractPlainSocketImpl.doConnect(AbstractPlainSocketImpl.java:350)
	at java.net.AbstractPlainSocketImpl.connectToAddress(AbstractPlainSocketImpl.java:206)
	at java.net.AbstractPlainSocketImpl.connect(AbstractPlainSocketImpl.java:188)
	at java.net.SocksSocketImpl.connect(SocksSocketImpl.java:392)
	at java.net.Socket.connect(Socket.java:589)
	at redis.clients.jedis.Connection.connect(Connection.java:158)
	... 34 common frames omitted

[jknack]

What you did is OK and should work. I'm going to review and fix it.

I'm not going to merge this pull because it is a simply change but you made 5 commits ;). In general, like to keep git history clean and easy to follow, this is the reason of why I'm not going to merge your changes (but will copy and paste).

In general if you need to fix something in an opened pull request use: git commit --amend

Thanks.

[pierredavidbelanger]

What some project owners are doing, is leave PR commits as is, because it is easier to follow the thread when commits are in the right sequence relative to the comments.

When the owner feels it is time to merge, they ask the contributor to rebase.

This way: no copy/paste mess, clean history and the burden of the clean up is on the contributor not the project owner ;)

[jknack]

can you rebase? let's do it (asked before for rebase but never got an answer)

please rebase and will happily merge it!

[pierredavidbelanger]

Haha, yeah, some people are afraid to rebase.
I must argue it is not the simplest thing to do with GIT.

Rebase is in progress.
Thank you.

[pierredavidbelanger]

Here you go, rebased (amend and squash) into two commits.

The test is still commented though, as I do not know how to test this without any actual redis connection.

[jknack]

We don't have integration tests for jedis, so don't worry. I will merge, test and if everything is good, the unit test is enough.

Thank you.