Adding tests requested in #295 #298
Conversation
|
This'll need to be rebased anyway as 361e3ed is in main under a different SHA, I will sort that out. |
|
Thanks, I will review this week. |
|
Thank you for adding the tests! Few nits from my side, and looks like commits are missing sign-off. |
src/memcached/cache_impl.go
Outdated
| @@ -205,9 +205,9 @@ func refreshServers(serverList memcache.ServerList, srv_ string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func newMemcachedFromSrv(srv_ string, d time.Duration) Client { | |||
| func newMemcachedFromSrv(srv_ string, d time.Duration, resolver srv.SrvResolver) Client { | |||
There was a problem hiding this comment.
nit: srv_ -> srv (is not common in Go to use underscores for variable names)
There was a problem hiding this comment.
and the same nit for all other occurrences in the code above and below
src/srv/srv.go
Outdated
| @@ -21,7 +27,7 @@ func ParseSrv(srv string) (string, string, string, error) { | |||
| return matches[1], matches[2], matches[3], nil | |||
| } | |||
|
|
|||
| func ServerStringsFromSrv(srv string) ([]string, error) { | |||
| func (s DnsSrvResolver) ServerStringsFromSrv(srv string) ([]string, error) { | |||
There was a problem hiding this comment.
nit: use more descriptive var name instead of s
src/memcached/cache_impl_test.go
Outdated
| assert.Equal([]string{mockMemcacheHostPort}, actualMemcacheHosts) | ||
| } | ||
|
|
||
| func TestRefreshServerSetsServersOnNonEmptyServerList(t *testing.T) { |
There was a problem hiding this comment.
nit: i think more descriptive name this test case would be ...OverridesServersOnNonEmptyServerList
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
cec7fa2 to
53c19da
Compare
|
@nezdolik I have updated everything as per your comments |
|
@petedmarsh looks like one more nit left and then you can mark this PR as ready for review (instead of Draft) |
Signed-off-by: Peter Marsh <pete.d.marsh@gmail.com>
53c19da to
c886572
Compare
Signed-off-by: Peter Marsh <pete.d.marsh@gmail.com>
I recent raised #295 to fix a bug, this is a bug that could have been caught with tests but no tests had been written (I wrote the original code, so it is my fault).
I've had a stab here at writing some tests for this case, they required a small amount of refactoring. I fully expect that I can improve upon them and I am happy to do so. For one thing the test file is in the main
srctree rather thantestas it seemed silly to me to make these functions public, but it also seems silly to have a test here.Anyway, I am happy to improve and add more, I am raising early in the hope I can get some feedback (though I appreciate that that isn't your job to do so!).