Need a test for "redis://...

[steveklabnik]

Coveralls shows that we have no test for building something with "redis:// https://coveralls.io/builds/24478/source?filename=lib%2Fresque%2Fconfig.rb

So I tried to write a test:

diff --git a/test/resque/config_test.rb b/test/resque/config_test.rb
index 7585826..cd93eab 100644
--- a/test/resque/config_test.rb
+++ b/test/resque/config_test.rb
@@ -12,6 +12,12 @@ describe Resque::Config do
       assert_equal 'namespace', config.redis.namespace
     end

+    it "works correctly with a URI string" do
+      redis = Redis.new(:url => 'redis://127.0.0.1:6379')
+      config.redis = 'redis://127.0.0.1:6379'
+      assert config.redis
+    end
+
     it "works correctly with a Redis::Namespace param" do
       new_redis = Redis.new(:host => "localhost", :port => 9736)
       new_namespace = Redis::Namespace.new("namespace", :redis => new_redis)
@@ -45,4 +51,5 @@ describe Resque::Config do
       assert_equal config.redis_id, "redis://localhost:6379/0, redis://localhost:6380/0"
     end
   end
-end
\ No newline at end of file
+end
+

This doesn't go well. If we make this line return nil, then the test still passes, because of https://github.com/resque/redis-namespace/blob/master/lib/redis/namespace.rb#L192 in redis-namespace

Tl;DR: I don't know how to write a good test for this.

[ndbroadbent]

Stub Redis::Namespace#initialize?

def initialize(namespace, options = {})
  @namespace, @redis = namespace, options[:redis]
end

[steveklabnik]

That might work, but also feels super hacky. Hm. I guess that'd be okay. Feel like submitting a patch?

[ndbroadbent]

Agree it's a bit hacky... Other thought was setting Redis.current to another known value, and testing that the result is different?

[steveklabnik]

I feel like I tried that and it didn't work for some other reason.

[ndbroadbent]

Slightly less hacky would be to stub Redis.current to return nil.

Heh, just saw your tweet and thought it would be fun to have a look.

[Crunch09]

Slightly less hacky would be to stub Redis.current to return nil.

I also think this is the best solution.

[davidbiehl]

Shooting from the hip here ... Asserting config.redis will always asset true because the method is returning an instance of Redis::Namespace regardless of whether or not the line in question returns nil. So, stubbing Redis.current may not have worked because you would need to change the assertion to config.redis.redis

[steveklabnik]

Right, I think even there were issues. Don't remember.
—
Sent from Mailbox for iPhone

On Sun, Apr 28, 2013 at 8:45 AM, David Biehl notifications@github.com
wrote:

Shooting from the hip here ... Asserting config.redis will always asset true because the method is returning an instance of Redis::Namespace regardless of whether or not the line in question returns nil. So, stubbing Redis.current may not have worked because you would need to change the assertion to config.redis.redis

Reply to this email directly or view it on GitHub:
#971 (comment)

[davidbiehl]

Taking a step back, if the Redis.connect method is stable and tested you could mock it and assert that it receives the expected value. Just another approach....

[chelseakomlo]

There is a test for this method in this commit. Not sure if this is what you had in mind though.

[steveklabnik]

It is!