Unit-testing account transaction code

Question

I'm playing with unit-testing in Ruby. There is a situation that I don't know if is good enough or if I am doing the wrong abstraction.

I have two classes: Transaction and Account. An Account could have 0..* Transactions. The only constraint is shall not have duplicated transactions for an Account.

it "add a duplicated transaction will fails" do
 transaction = double(:transaction, id: '124-acd-345')
 expect(transaction).to receive(:==).and_return true
 subject.add_transaction transaction
 subject.add_transaction transaction.clone
 expect(subject.transactions.count).to eq 1
end

Account#add_transaction piece:

def add_transaction(transaction)
 @transactions << transaction if transactions.select { |t| t == transaction }.empty?
end

Transaction#== piece:

def ==(other)
 other.id == self.id
end

The point is: Transaction#== is already unit tested in their test class. I know that transaction == works properly. So, my approach in Account#add_transaction was Test Behavior (with that expect(transaction) ...) and Test Value, checking if there is only one transaction into object.

IMO, I didn't like this solution, but I can't find anything better to do. Tips?

• unholysampler: What don't you like about the solution? What makes you feel like there should be a better way?
• hahaha: @unholysampler mixing behavioral and value test assertions makes me feel like if there are something wrong.

Answer 1

My first thought based on your comment (behavioral and state testing in same test) was that you should just make two different tests.

However, then I took a step back. The specification says that:

add[ing] a duplicated transaction will fail

It doesn't say anything about how a duplicate is determined. Currently, you are using the naive implementation is to check if and of existing transactions equal the one being added. However, that is not the only way satisfy the requirement.

If there are a large number of transaction, it will be slow to iterate over each transaction. Instead, the transactions could be stored in a map that uses the id as a key. Now be can check if the transaction already exists by doing a single key lookup, \$O(1)\$ instead of \$O(n)\$.

The fact that Transaction#== is called is an implementation detail. Based on this, the behavioral assertion should be removed from the test. If there happens to be a different requirement, that was not presented here, that the duplication check must be done throw an equivalence check, then that should be a different test.

• hahaha: Good observations! My only concern is about when add a ORM there, because even DataMapper will try to manage Account#transactions array. To keep O(1), I should have another array who will check transactions_map.include?(new_transaction.id) before do @transactions << new_transaction. Do you agree? Thanks.
• unholysampler: 1) I'm not actually recommending that you change any code in Account. I mentioned the performance issues to prove my point. These type of optimizations are best saved until the code is profiled, unless you are explicitly targeting large number. 2) I've never actually written an Ruby, so I can't exactly follow the follow up question.