Deprecate static attributes?

[jferris]

Many of the questions on Stackoverflow and the former mailing list are because of confusion as to when you do and don't need to define your attributes using a block.

Using a block to define dynamic attributes will ALWAYS work, whereas static attributes only work when the value never changes.

Have we done recent benchmarks to determine the performance advantages of static attributes in a realistic test suite? If they aren't significant, I think it's worth deprecating static attributes to improve consistency and reduce confusion.

[joshuaclayton]

I'd definitely be interested in benchmarks. In newer versions of factory_girl, both static and dynamic attributes (along with associations and implicit attributes) get converted to a proc and added as methods on an anonymous subclass of FactoryGirl::Evaluator, so I don't think the performance impact would be large.

I don't feel great about doing this, since it makes developers who otherwise understand the difference between static and dynamic update the syntax (which is noisier). Would changes to the documentation help reduce confusion instead?

[jferris]

The dynamic syntax is a little noisier, but it's also less consistent when combined with static attributes. Also, even experienced developers frequently make this mistake. I often come onto projects that have static attribute values for things like Time.now, which results in odd bugs like tests that fail if the test suite takes too long.

[halogenandtoast]

Also, even experienced developers frequently make this mistake. I often come onto projects that have static attribute values for things like Time.now, which results in odd bugs like tests that fail if the test suite takes too long.

experienced developers

I do not think it means what you think it means.

[derekprior]

I suspect this will piss more people off than it will rescue from mistakes. I also fear the factories would get pretty noisy.

The majority of problems I've seen with static attributes are with dates and times. Would it be possible to warn when using static attributes with these columns?

[halogenandtoast]

One thing I suggested in 'code' was that we could load static values for a definition twice and throw a warning if the value changes.

[joshuaclayton]

@halogenandtoast that's an interesting possibility, especially with the introduction of FactoryGirl.lint.

A couple rough thoughts:

• what happens if there's a static attribute that interacts with the database? would this get cleared out before the test suite is run?
• if a factory has both static and dynamic attributes, do we make the distinction of values at a model level, or an evaluator level? The main distinction here is that if the setter on the model does additional work, it may break things. An example here would be a slug on a model that's a static attribute in factory_girl but the model increments an appended integer, so they'd differ.

I might be over-thinking some of these, but I'm definitely interested in providing more value to users so this isn't an issue that comes up often. I think a good first pass would be to focus on Date and Time, since they're the most common offenders.

[halogenandtoast]

if a factory has both static and dynamic attributes, do we make the distinction of values at a model level, or an evaluator level?

Evaluator. Once we're out of the scope of the factory I don't believe it makes sense in the context of the problem. In your example you mention slug, but that's not really the factories concern (and if it was, I'd imagine it implemented as a sequence).

what happens if there's a static attribute that interacts with the database? would this get cleared out before the test suite is run?

I don't think I've even seen this in a factory outside of callbacks. Do you have a specific example in mind? As an addendum to what I previously said I think FactoryGirl.lint might be a great place for this. I'd image the process would like like:

• start test suite
• load factory girl
• validate factories (ignoring callbacks and associations)
• resume execution of test suite

Furthermore I'd probably add some configuration option to disable this from happening so seasoned devs can skip this and reclaim any time that would be lost by executing this process.

[derekprior]

what happens if there's a static attribute that interacts with the database? would this get cleared out before the test suite is run?

I don't think I've even seen this in a factory outside of callbacks. Do you have a specific example in mind?

I've recently seen it where a project uses both factories and fixtures for reference data. A simplified example:

factory :user do
  speciality Specialty.first
end

Where Speciality is loaded with fixtures. That's not something I'd actually do, but that doesn't mean it isn't frequently done.

[jferris]

Another mistake I've seen frequently is defining an attribute like this:

factory :user do
  specialty FactoryGirl.create(:specialty)
end

Preventing ActiveRecord instances in static attributes could also be useful for catching these.

[odlp]

I think in general encouraging dynamic attributes by default is sensible. I wonder if we can evaluate the impact of this on test times with a realistic suite? (Perhaps Hub would be a good candidate).

If dynamic-attributes do become the default perhaps it might make sense to...

• Introduce a #add_static_attribute method (allowing an explicit choice by the dev)
• Change #add_attribute to only accept a block.

[odlp]

PR adding deprecation notice: #1135

[composerinteralia]

I just merged the PR to deprecate static attributes (#1135). To make this as painless as possible, @seanpdoyle and I worked on a rubocop-rspec Cop that can automatically update static to dynamic attributes.

As the next release goes out I will write up a blog post to cover why I think the deprecation was necessary (including much of what was already said in this thread).