How to avoid overtesting of a private method's behavior?

Question

Given that:

Testing private methods is not a good practice.
An interface with many public methods is not a good practice either.

Suppose I have the following code:

module RedisHelper
  private

  def key_for(id)
    [prefix, id].join('-')
  end

  def prefix
    self.class.name.split('::').last.underscore
  end
end

class X
  include RedisHelper

  def perform(id)
    key = key_for(id)
    redis.sadd(key, [:bar])
  end
end

class Y
  include RedisHelper

  def perform(id)
    key = key_for(id)
    redis.set(key, :foo)
  end
end

What would be the best way to test the behavior of key_for/prefix methods without overtesting them? I wouldn't like to repeat its logic when testing classes X and Y.

I don't think that turning RedisHelper's methods public is good, because they would be public in X and Y also.

Why would you think testing private methods is not good practice? — Cjmarkham, Apr 17 '19 at 15:53
@CarlMarkham because doing so means you tests are coupled to a particular implementation, which can make it more difficult to refactor. — Andy Waite, Apr 17 '19 at 16:26
The goal of unit-testing is to find bugs. Different implementations, different bugs. For example, coverage looks at implementation, not at interface. Thus, tests should be designed to (also) test implementation details. Certainly, fragile tests are not desirable, but don't revert the priorities: Primary goal is finding bugs, secondary goal is avoiding unnecessarily fragile tests. — Dirk Herrmann, Apr 21 '19 at 00:09

Kache · Answer 1 · 2019-05-09T20:46:27.233

Direct answer

Test the module directly:

def test_redis_key_namespacing
  dummy_class = Class.new do # this anonymous class will not pollute namespace
    include RedisHelper

    public :key_for

    def self.name
      'Dummy'
    end
  end

  assert_equal 'Dummy-hello', dummy_class.new.key_for('hello')
end

Overall Suggestion

I recommend not using a RedisHelper module. It's good that you're trying to keep "Redis namespacing code" logic together and separated from other code, but what you really have is "Redis namespacing code" all over the place.

key_for is in both X and Y -- they have to handle the namespacing themselves every call to redis. Also, both key_for & prefix can be called in any of those classes (and subclasses and any other included modules) even though they're both internal to "Redis namespacing".

It looks like "Redis key namespacing" is an important concept in your application. It's reused in multiple places by different unrelated areas. Make it "a thing":

class NsRedis
  key_methods = Redis.instance_methods(false).select do |m|
    Redis.instance_method(m).parameters.first == [:req, :key]
  end

  # some metaprogramming, but it's static, much better than method_missing!
  key_methods.each do |m_name|
    define_method(m_name) do |key, *args|
      Redis.current.public_send("#{@namespace}:#{key}", *args)
    end
  end

  def initialize(namespace)
    @namespace = namespace
  end
end

# Now, anywhere you're going to need it, including tests:
class X
  def initialize
    @redis = NsRedis.new(self.class.name)
  end

  def do_something
    @redis.set(:my_key, 'my_val') # "unaware" of the namespacing at usage
  end
end

score 0 · Answer 2 · answered Apr 19 '19 at 11:13

In RSpec you can use shared examples:

RSpec.shared_examples "Redis namespacing helper" do
  # your tests for key_for, prefix and what-have-you
end

RSpec.describe X do
  it_behaves_like "Redis namespacing helper"
end

RSpec.describe Y do
  it_behaves_like "Redis namespacing helper"
end

In minitest this is probably handled by simply including the module with shared tests.

How to avoid overtesting of a private method's behavior?

2 Answers2