Improve tests stability #17
Conversation
kardapoltsev
force-pushed
the
wip/improve-tests
branch
4 times, most recently
from
9592c1c
to
69338db
Compare
Pull Request Test Coverage Report for Build 99
💛 - Coveralls |
kardapoltsev
force-pushed
the
wip/improve-tests
branch
from
1e80726
to
4f7e7f9
Compare
|
Now it looks that we have an issue with SentinelMutablePoolSpec only. But it might be not a test issue but a bug in the main code. |
|
Whoa! Thansk a lot for all the work! I skimmed through the changes on my phone and this seems fine. I'll do a more detailed review next week 👍 |
|
Two tests are still fail sometimes but now builds seem to be more stable https://travis-ci.org/kardapoltsev/rediscala/builds. |
|
@Ma27 ping |
|
@kardapoltsev yeah sorry, forgot about that. I'll have a look at this next weekend :) |
|
Any updates? |
| scmInfo := Some(ScmInfo(url("https://github.com/Ma27/rediscala"), "scm:git:git@github.com:Ma27/rediscala.git")), | ||
| apiURL := Some(url("http://etaty.github.io/rediscala/latest/api/")), | ||
| pomExtra := | ||
| <developers> |
There was a problem hiding this comment.
Not directly a review comment, but it would be great if you could those minor task in a separate PR in the future, that makes the diff easier to read :)
src/main/scala/redis/RedisPool.scala
Outdated
|
|
||
| def redisServerConnections: scala.collection.Map[RedisServer, RedisConnection] | ||
| protected val log = Logging.getLogger(system, this) | ||
| protected def redisServerConnections: scala.collection.Map[RedisServer, RedisConnection] |
There was a problem hiding this comment.
Unless I'm mistaken, declaring an existing property as protected probably breaks backwards-compatibility. Are there any technical reasons? I'll probably find some more issues, but I'd prefer to get this merged without having to release a new major release.
There was a problem hiding this comment.
I think it must not be public initially. Ok, I'll revert this changes.
src/main/scala/redis/RedisPool.scala
Outdated
| def onConnect(redis: RedisCommands, server: RedisServer): Unit = { | ||
| server.password.foreach(redis.auth(_)) // TODO log on auth failure | ||
| server.db.foreach(redis.select) | ||
| private def onConnect(redis: RedisCommands, server: RedisServer): Unit = { |
|
@kardapoltsev first of all, sorry again for me being so unresponsive recently. Is there anything else I can do to get this ready? @herzrasen in case you have time - would you like to have a look at this as well? |
|
Generally LGTM. I'd like to keep the visibility of the defs as they were before, because we might break backwards compatibility, as you said @Ma27 |
|
Hi @ALL! I'm finally back now. Is there anything except visibility for merging? I think we should move forward with this project :) |
kardapoltsev
force-pushed
the
wip/improve-tests
branch
from
5edb4a9
to
e421b2d
Compare
|
So, just pushed this branch onto this repo and it seems as the build is fine now. Not exactly sure (probably a caching issue on their side as some old code was checked out according to the build logs). I'll remove my commits again from thsi branch, after this I'd consider this mergable. |
Ma27
force-pushed
the
wip/improve-tests
branch
from
7197211
to
e421b2d
Compare
|
@kardapoltsev thanks! |
It's still WIP. Any feedback would be useful :)