Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reuse created factories (caching) #389

Closed
sobrinho opened this issue Jun 8, 2012 · 4 comments
Closed

Reuse created factories (caching) #389

sobrinho opened this issue Jun 8, 2012 · 4 comments

Comments

@sobrinho
Copy link

sobrinho commented Jun 8, 2012

Hi,

To simplify the issue, take a look on that example using machinist:

brasil = Country.make!(:brasil)

minas_gerais = State.make!(:minas_gerais, country: brasil)
rio_grande_do_sul = State.make!(:rio_grande_do_sul, country: brasil)

City.make!(:belo_horizonte, state: minas_gerais)
City.make!(:porto_alegre, state: rio_grande_do_sul)

Using machinist-caching, you can just do that:

City.make!(:belo_horizonte)
City.make!(:porto_alegre)

It is possible because machinist-caching uses an "identity map" on blueprints to cache subsequent calls to make:

Country.count
#=> 0
Country.make!(:brasil).object_id
#=> 70130314890540
Country.make!(:brasil).object_id
#=> 70130314890540
Country.count
#=> 1

It is a didactic scenario because on real world, the problem is much harder to workaround.

Take a look at my production code:

FactoryGirl.define do
  factory :property_transfer_revenue, :class => Revenue do
    name 'Imposto Sobre Transmissão de Bens e Imóveis'
    association :incidence_revenue, :factory => :tribute_incidence
    association :currency, :factory => :real
    association :type_revenue, :factory => :tax_revenue
  end

  factory :property_transfer_subrevenue, :class => Revenue do
    association :parent, :factory => :property_transfer_revenue
    name 'Imposto Sobre Transmissão de Bens e Imóveis'
    association :incidence_revenue, :factory => :tribute_incidence
    association :currency, :factory => :real
    association :type_revenue, :factory => :tax_revenue
  end
end

Note I need the tribute incidence to make property transfer sub revenue and I need him again to make property transfer revenue which is needed to make property transfer sub revenue.

It happens again on currency and type revenue.

Later, I will need another factory like that:

FactoryGirl.define do
  factory :property_transfer, :class => PropertyTransfer do
    association :revenue, :factory => :property_transfer_subrevenue
  end
end

And more later, I will need another factory like that:

FactoryGirl.define do
  factory :property_transfer_payment, :class => Payment do
    association :property_transfer, :factory => :property_transfer
  end
end

So, using the manual workaround of specifying attributes is not easy to apply and many times is impossible.

I'm not familiar with factory girl code to make the same solution here and since machinist is dead, I need to replace it with factory girl.

What you guys think?

Should it be a feature of factory girl or should it be a plugin just like machinist-caching?

I really think it should be a feature of factory girl because this problem happens on every project I know that uses machinist, factory girl or fabrication since ever :-)

@joshuaclayton
Copy link
Contributor

Hey @sobrinho!

As for the identity map, that's come up a number of times and we've chosen not to support it, for various reasons. The biggest one is that we've been bitten on multiple occasions with data that we expected (or didn't expect). We've also found that it leads to poorer testing practices.

As for the second half of your request regarding reusing associations, that's actually pretty easy!

You can use parent/child relationships to define common attributes (in this case, associations):

FactoryGirl.define do
  factory :property_transfer_revenue, :class => Revenue do
    name 'Imposto Sobre Transmissão de Bens e Imóveis'

    association :incidence_revenue, :factory => :tribute_incidence
    association :currency, :factory => :real
    association :type_revenue, :factory => :tax_revenue

    factory :property_transfer_subrevenue, :class => Revenue do
      association :parent, :factory => :property_transfer_revenue
    end
  end
end

This won't reuse the same values that would've come from your identity map; to carry the attributes from the parent to the child, you'd actually need to do something like this:

  factory :property_transfer_subrevenue, :class => Revenue do
    association :parent, :factory => :property_transfer_revenue
    incidence_revenue { parent.incidence_revenue }
    currency { parent.incidence_revenue }
    type_revenue { parent.type_revenue }
  end

Let me know if one or the other works out!

@sobrinho
Copy link
Author

sobrinho commented Jun 9, 2012

Hey @joshuaclayton!

I don't agree this lead to poor testing practices and let me explain why.

In my current application, I have a thing called active debt, it's a debt of tax payer in city hall.

To have a active debt, I need to have a payment (payment not necessarily means it's paid) and to have a payment, I need to have a revenue.

Making blueprints:

FactoryGirl.define do
  factory :tax_revenue, :class => RevenueType do
    name 'Tax'
  end

  factory :iptu, :class => Revenue do
    association :revenue_type, :factory => :tax_revenue
  end

  factory :itbi, :class => Revenue do
    association :revenue_type, :factory => :tax_revenue
  end

  factory :john_doe_payment, :class => Payment do
    association :revenue, :factory => :iptu
  end

  factory :john_roe_payment, :class => Payment do
    association :revenue, :factory => :itbi
  end

  factory :john_doe_active_debt, :class => Payment do
    association :payment, :factory => :john_doe_payment
  end

  factory :john_roe_active_debt, :class => Payment do
    association :payment, :factory => :john_roe_payment
  end
end

scenario 'filter active debts report by revenue' do
  create :john_doe_active_debt
  create :john_roe_active_debt

  ...
end

Some people said to me to handle this in scenario just like that:

scenario 'filter active debts report by revenue' do
  tax_revenue = create :tax_revenue

  iptu = create :iptu, :revenue_type => tax_revenue
  itbi = create :itbi, :revenue_type => tax_revenue

  john_doe_payment = create :john_doe_payment, :revenue => iptu
  john_roe_payment = create :john_roe_payment, :revenue => itbi

  john_doe_active_debt = create :john_doe_active_debt, :payment => john_doe_payment
  john_roe_active_debt = create :john_roe_active_debt, :payment => john_roe_payment

  ...
end

This make scenario complex to maintain, think if we do that in 50 scenarios and the database changes the structure.

Also, this do not have all objects my application needs because revenue have 3 associated objects which repeats for iptu and itbi revenues which also happens on payment for others objects like person and taxable objects.

It happens again for active debt which have others shared objects.

Ok, it's a symptom of coupled objects in a system but that happens for simple cases like debt (revenue) > active debt.

I'm working in a solution using factory girl strategy and I'm close to get the same functionality of machinist-caching, take a look: https://gist.github.com/2901172

How you deal in this situations? Handle manually like the second code example?

@joshuaclayton
Copy link
Contributor

@sobrinho Yep, we'd handle it from a FG standpoint like your example. That said, what we'd likely do is use the Factory method pattern to roll all of that data creation into a method to use. Different sets of data would have different methods to generate, hopefully named well, and using some sort of abstraction underneath assuming some of the data creation is similar.

You're more than welcome to use whatever you feel comfortable with; if your caching strategy works and you're aware of the issues, then feel free to use it! That's why we gave devs the ability to write their own strategies. Otherwise, I'd suggest looking into a solution similar to mine, where you have some object that creates swaths of data necessary to run your tests.

Best of luck!

@sobrinho
Copy link
Author

@joshuaclayton thanks for explanation! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants