Watch out for your test case values: detecting code changes

Having a test case exercising your code path is not enough if it is set up with values that wouldn’t detect code changes.

In a real-world Ruby on Rails app codebase, when refactoring and moving big chunks of legacy code around, an important if-comparison was dropped by accident and went undetected through our test suite. Let’s take a look.

Case

(For this post, snippets are made-up to illustrate the point, and they don’t refer to the actual application code).

Imagine a Sale model has a many-to-many relationship with Product and we have a commission_type ("dollar" or "percent") and a commission_value for each link. To calculate the sale commission amount in cents for a given sale, we had:

def calculate_sale_commission_amount_in_cents(sale)
  sale.product_sales.sum do |ps|
    ps.commision_type == "dollar" ? ps.commision_value * 100 : (ps.commission_value * sale.price_in_cents / 100.0).to_i
  end
end

In the middle of the process, we decided to expand that ternary-if just for ease of reading, and we ended up making a mistake:

# Mistaken code

def calculate_sale_commission_amount_in_cents(sale)
  sale.product_sales.sum do |ps|
    if ps.commision_type
      ps.commision_value * 100
    else
      (ps.commission_value * sale.price_in_cents / 100.0).to_i
    end
  end
end

Notice the == "dollar" from the if-comparison was accidentally dropped, which made the else branch for the "percent" case unreachable! Interestingly enough, tests still passed. Let’s check it out:

# ... some shared setup for the system under test
let(:sale) { create(:sale) }
let(:product) { create(:product) }
# ...

context "when commission type is percent" do
  let(:sale) { create(:sale, price_in_cents: 100_00) }

  it "creates a SaleCommission with the expected amount" do
    create(:product_sales, sale: sale, product: product,
      commission_type: "percent",
      commission_value: 10
    )

    call_service

    sale_commission = SaleCommission.where(sale: sale).last
    expect(sale_commission).not_to be_nil
    expect(sale_commission.amount_in_cents).to eq(10_00)
  end
end

Unfortunately, the values chosen for the test case weren’t good choices.

Sale’s price_in_cents: 100_00 and commission_value: 10 for "when commission value type is percent" coincidentally passes when applied to the mistaken code above. That happens just because of the conversion to cents done with * 100 for the "dollar" case.

Fix

A more carefully chosen value for the sale’s price_in_cents could have caused the test to fail and detect the code change. For example:

context "when commission type is percent" do
  let(:sale) { create(:sale, price_in_cents: 199_00) } # Any value other than 100_00

  it "creates a SaleCommission with the expected amount" do
    create(:product_sales, sale: sale, product: product,
      commission_type: "percent",
      commission_value: 10
    )

    call_service

    sale_commission = SaleCommission.where(sale: sale).last
    expect(sale_commission).not_to be_nil
    expect(sale_commission.amount_in_cents).to eq(19_90)
  end
end

Also, for correctness, calculate_sale_commission_amount_in_cents was rewritten to list the expected commission types and to explicitly decide on what to do when a commission_type didn’t match, with tests adjusted accordingly:

def calculate_sale_commission_amount_in_cents(sale)
  sale.product_sales.sum do |ps|
    case ps.commision_type
    when "dollar"
      ps.commision_value * 100
    when "percent"
      (ps.commission_value * sale.price_in_cents / 100.0).to_i
    else
      0 # Or raise an error if this is considered exceptional
    end
  end
end

Conclusion

With this case, we could experience that accidents happen mainly when moving and refactoring big chunks of legacy code. A test suite gives us the confidence we aren’t breaking existing business rules as long as we carefully choose values for our test cases to detect code changes.