Testing public methods vs private methods

The features or logic of private methods should be tested through public methods, but the private methods should not be tested themselves.

A short intro

Before reading this article, please focus on testing rather than code design. You might need to refactor or debate the code's shape, organization, or naming. While your proposals may be good, we are here to discuss testing, not OOP code design. I like to work with code that is not perfect because in day-to-day activities, we work with code like this, and so it pins down the theory and helps us focus on talking about practical advice.

A simple case

Here is a piece of code from Maybe.co: 

module ImportsHelper

  def permitted_import_configuration_path(import)
    if permitted_import_types.include?(import.type.underscore)
      "import/configurations/#{import.type.underscore}"
    else
      raise "Unknown import type: #{import.type}"
    end
  end

  # ... 

  private
    def permitted_import_types
      %w[transaction_import trade_import account_import mint_import]
    end

We want to test permitted_import_configuration_path I think here there should be at least two test cases:

In this case, are we testing or not the permitted_import_types ? Would you do a different test if that method would be like this: 

 def permitted_import_configuration_path(import)
    permitted_import_types = %w[transaction_import trade_import account_import mint_import]
    if permitted_import_types.include?(import.type.underscore)
      "import/configurations/#{import.type.underscore}"
    else
      raise "Unknown import type: #{import.type}"
    end
  end

A more complex case

Now let's go into a more complex code sample from Maybe.co: 

class Import < ApplicationRecord

  def generate_rows_from_csv
    rows.destroy_all

    mapped_rows = csv_rows.map do |row|
      {
        account: row[account_col_label].to_s,
        date: row[date_col_label].to_s,
        qty: sanitize_number(row[qty_col_label]).to_s,
        ticker: row[ticker_col_label].to_s,
        price: sanitize_number(row[price_col_label]).to_s,
        amount: sanitize_number(row[amount_col_label]).to_s,
        currency: (row[currency_col_label] || default_currency).to_s,
        name: (row[name_col_label] || default_row_name).to_s,
        category: row[category_col_label].to_s,
        tags: row[tags_col_label].to_s,
        entity_type: row[entity_type_col_label].to_s,
        notes: row[notes_col_label].to_s
      }
    end

    rows.insert_all!(mapped_rows)
  end

  private 

    def sanitize_number(value)
      return "" if value.nil?
      value.gsub(/[^\d.\-]/, "")
    end

When testing generate_rows_from_csv in a good testing suite, we need to test if the inserted rows are adequately sanitized. I would write tests for generate_rows_from_csv that will include:

Actually I might model the sanitize_number with Equivalence partitioning and get the following input partitions of similar behavior:

Even if sanitize_number is a private method, I would expect when I test generate_rows_from_csv as a feature to make sure it works when the column values are either valid or invalid values.

When testing, I should behave like this method looks actually like this: 

def generate_rows_from_csv
    rows.destroy_all

    sanitize_number = ->(value) do
      if value.nil?
        ""
      else 
        value.gsub(/[^\d.\-]/, "")
      end
    end

    mapped_rows = csv_rows.map do |row|
      {
        account: row[account_col_label].to_s,
        date: row[date_col_label].to_s,
        qty: sanitize_number(row[qty_col_label]).to_s,
        ticker: row[ticker_col_label].to_s,
        price: sanitize_number(row[price_col_label]).to_s,
        amount: sanitize_number(row[amount_col_label]).to_s,
        currency: (row[currency_col_label] || default_currency).to_s,
        name: (row[name_col_label] || default_row_name).to_s,
        category: row[category_col_label].to_s,
        tags: row[tags_col_label].to_s,
        entity_type: row[entity_type_col_label].to_s,
        notes: row[notes_col_label].to_s
      }
    end

    rows.insert_all!(mapped_rows)
  end

If I don't treat this code like this, I will miss important cases of what a row might contain and not properly assess if generate_rows_from_csv is working as expected.

A case with calling a dependency

What about the following code from RailsDevs.com source code

module Developers
  class PaywalledSearchResults
    # ...
    def unauthorized_page?
      !user_authorized? && not_on_first_page?
    end

    def show_paywall?(result_count)
      result_count > Pagy::DEFAULT[:items] && !user_authorized?
    end

    private

    def user_authorized?
      Users::Permission.new(user).authorized?
    end

    def not_on_first_page?
      page > 1
    end
  end
end

Should we test all rules from the Users::Permission#authorized? when we test for example unauthorized_page? ?

Here the answer is no. Users::Permission#authorized? is a public method of that Users::Permission object and the business rules for that method should have been tested in a test for that object.

Thus when I test: unauthorized_page? with respect to the logic for unauthorized_page? I need the following 4 test cases: 

+------------------+-----+-----+-----+-----+
| Condition        | TC1 | TC2 | TC3 | TC4 |
+------------------+-----+-----+-----+-----+
| user_authorized? | F   | F   | T   | T   |
| page > 1         | F   | T   | F   | T   |
|                  |     |     |     |     |
| Result           | F   | T   | F   | F   |
+------------------+-----+-----+-----+-----+

But we are not going into the details of user_authorized? where we test for customer or admin or public access also. We just care to make that method true and false in our tests here.

What to remember

When you are testing the requirements, business logic, or business rules of a piece of code—that is what this piece of code should do—you should test the scenarios that include the private methods as if the private methods would not be private but would have the code copied into them.

But you should not do that if the private method is just a proxy (or dependency injection) for calling the public method or another object. In that case, we only want to test our code's local behavior and not the internal behavior of the other object.

Consider that when we extract a public method, it is a code design decision, but the public method's final behavior (or output or outcome) should be the same regardless of whether we extract the private methods. Thus, we cannot ignore the cases created by the private method, and we should consider them in our logic.

You don't want to test the private method's shape or form and how it gets the state it needs (does it accept an argument or use instance variables). However, we should care if the behavior of that private method is what we need to make our main method work.

PS: All code presented here is taken from open source projects, and I have no affiliation with those particular projects.