Allow opt-in of warning on deprecating static attributes

What?
=====

Static attributes can introduce odd or unexpected behavior, largely
because the value is evaluated once, when the factory definition is
loaded. Most commonly, issues can come up with dates:

```ruby
factory :post do
  published_on Date.yesterday
end
```

This may cause problems based on the time of day the suite is running,
especially if there's already a mystery guest introduced (e.g. the test
callsite assumes `published_on` was always one day ago).

To address this, the first step is letting developers know where their
test suites refer to static attributes. This commit introduces a
configuration option developers can opt into where we trigger a
deprecation warning with the factory name and static attributes.

To opt into this setting:

```ruby
\# spec/support/factory_girl.rb
FactoryGirl.configuration.warn_on_static_attributes = true
```

GETTING_STARTED.md

@@ -170,6 +170,17 @@ user.first_name
 # => "Joe"
 ```
 
+Static Attributes
+-----------------
+
+Static attributes will be deprecated in an upcoming version of factory\_girl.
+To opt into warnings now, you can configure the setting as such:
+
+```ruby
+# spec/support/factory_girl.rb
+FactoryGirl.configuration.warn_on_static_attributes = true
+```
+
 Dynamic Attributes
 ------------------
 

Gemfile.lock

@@ -7,13 +7,13 @@ PATH
 GEM
   remote: https://rubygems.org/
   specs:
-    activemodel (5.1.4)
+    activemodel (5.1.5)
-      activesupport (= 5.1.4)
+      activesupport (= 5.1.5)
-    activerecord (5.1.4)
+    activerecord (5.1.5)
-      activemodel (= 5.1.4)
+      activemodel (= 5.1.5)
-      activesupport (= 5.1.4)
+      activesupport (= 5.1.5)
       arel (~> 8.0)
-    activesupport (5.1.4)
+    activesupport (5.1.5)
       concurrent-ruby (~> 1.0, >= 1.0.2)
       i18n (~> 0.7)
       minitest (~> 5.1)
@@ -23,15 +23,15 @@ GEM
       rake
       thor (>= 0.14.0)
     arel (8.0.0)
-    aruba (0.14.2)
+    aruba (0.14.3)
-      childprocess (~> 0.5.6)
+      childprocess (~> 0.8.0)
       contracts (~> 0.9)
       cucumber (>= 1.3.19)
       ffi (~> 1.9.10)
       rspec-expectations (>= 2.99)
       thor (~> 0.19)
     builder (3.2.3)
-    childprocess (0.5.9)
+    childprocess (0.8.0)
       ffi (~> 1.0, >= 1.0.11)
     concurrent-ruby (1.0.5)
     contracts (0.16.0)
@@ -43,31 +43,32 @@ GEM
       multi_test (>= 0.1.2)
     diff-lcs (1.3)
     docile (1.1.5)
-    ffi (1.9.18)
+    ffi (1.9.21)
     gherkin (2.12.2)
       multi_json (~> 1.3)
-    i18n (0.8.6)
+    i18n (0.9.5)
+      concurrent-ruby (~> 1.0)
     json (2.1.0)
-    minitest (5.10.3)
+    minitest (5.11.3)
-    multi_json (1.12.2)
+    multi_json (1.13.1)
     multi_test (0.1.2)
-    rake (12.1.0)
+    rake (12.3.0)
-    rspec (3.6.0)
+    rspec (3.7.0)
-      rspec-core (~> 3.6.0)
+      rspec-core (~> 3.7.0)
-      rspec-expectations (~> 3.6.0)
+      rspec-expectations (~> 3.7.0)
-      rspec-mocks (~> 3.6.0)
+      rspec-mocks (~> 3.7.0)
-    rspec-core (3.6.0)
+    rspec-core (3.7.1)
-      rspec-support (~> 3.6.0)
+      rspec-support (~> 3.7.0)
-    rspec-expectations (3.6.0)
+    rspec-expectations (3.7.0)
       diff-lcs (>= 1.2.0, < 2.0)
-      rspec-support (~> 3.6.0)
+      rspec-support (~> 3.7.0)
     rspec-its (1.2.0)
       rspec-core (>= 3.0.0)
       rspec-expectations (>= 3.0.0)
-    rspec-mocks (3.6.0)
+    rspec-mocks (3.7.0)
       diff-lcs (>= 1.2.0, < 2.0)
-      rspec-support (~> 3.6.0)
+      rspec-support (~> 3.7.0)
-    rspec-support (3.6.0)
+    rspec-support (3.7.1)
     simplecov (0.15.1)
       docile (~> 1.1.0)
       json (>= 1.8, < 3)
@@ -77,7 +78,7 @@ GEM
     thor (0.20.0)
     thread_safe (0.3.6)
     timecop (0.9.1)
-    tzinfo (1.2.3)
+    tzinfo (1.2.5)
       thread_safe (~> 0.1)
     yard (0.9.12)
 
@@ -100,4 +101,4 @@ DEPENDENCIES
   yard
 
 BUNDLED WITH
-   1.16.0
+   1.16.0.pre.3

lib/factory_bot/configuration.rb

@@ -3,7 +3,7 @@ module FactoryBot
   class Configuration
     attr_reader :factories, :sequences, :traits, :strategies, :callback_names
 
-    attr_accessor :allow_class_lookup, :use_parent_strategy
+    attr_accessor :allow_class_lookup, :use_parent_strategy, :warn_on_static_attributes
 
     def initialize
       @factories      = Decorator::DisallowsDuplicatesRegistry.new(Registry.new('Factory'))
@@ -14,6 +14,7 @@ module FactoryBot
       @definition     = Definition.new
 
       @allow_class_lookup = true
+      @warn_on_static_attributes = false
 
       to_create { |instance| instance.save! }
       initialize_with { new }

lib/factory_bot/definition_proxy.rb

@@ -8,12 +8,13 @@ module FactoryBot
 
     delegate :before, :after, :callback, to: :@definition
 
-    attr_reader :child_factories
+    attr_reader :child_factories, :static_attributes
 
     def initialize(definition, ignore = false)
       @definition      = definition
       @ignore          = ignore
       @child_factories = []
+      @static_attributes = []
     end
 
     def singleton_method_added(name)
@@ -45,6 +46,7 @@ module FactoryBot
       declaration = if block_given?
         Declaration::Dynamic.new(name, @ignore, block)
       else
+        @static_attributes << name unless @ignore
         Declaration::Static.new(name, value, @ignore)
       end
 

lib/factory_bot/syntax/default.rb

@@ -18,6 +18,12 @@ module FactoryBot
           proxy.instance_eval(&block) if block_given?
 
           FactoryBot.register_factory(factory)
+          if FactoryBot.configuration.warn_on_static_attributes && proxy.static_attributes.any?
+            attribute_list = proxy.static_attributes.map {|a| "* #{a}"}.join("\n")
+            ActiveSupport::Deprecation.warn(
+              "Factory '#{name}' has the following static attributes:\n#{attribute_list}"
+            )
+          end
 
           proxy.child_factories.each do |(child_name, child_options, child_block)|
             parent_factory = child_options.delete(:parent) || name

spec/acceptance/deprecate_static_attributes_spec.rb

@@ -0,0 +1,37 @@
+require "spec_helper"
+
+describe "Deprecate static attributes" do
+  around do |example|
+    old_warn = FactoryGirl.configuration.warn_on_static_attributes
+    FactoryGirl.configuration.warn_on_static_attributes = true
+    example.run
+    FactoryGirl.configuration.warn_on_static_attributes = old_warn
+  end
+
+  it "warns that static usage is deprecated" do
+    define_model("Comment", published_on: :date, user_id: :integer)
+    define_model("User", name: :string, email: :string)
+
+    ActiveSupport::Deprecation.expects(:warn).with(
+      "Factory 'user' has the following static attributes:\n* name\n* email"
+    ).once
+
+    ActiveSupport::Deprecation.expects(:warn).with(
+      "Factory 'comment' has the following static attributes:\n* published_on"
+    ).once
+
+    FactoryGirl.define do
+      factory :comment do
+        published_on Date.current
+        user
+      end
+
+      factory :post
+
+      factory :user do
+        name "John Doe"
+        email "john@example.com"
+      end
+    end
+  end
+end

spec/factory_bot/definition_proxy_spec.rb

@@ -15,6 +15,11 @@ describe FactoryBot::DefinitionProxy, "#add_attribute" do
     expect(subject).to have_static_declaration(:attribute_name).with_value("attribute value")
   end
 
+  it "tracks statically-defined attributes" do
+    proxy.add_attribute(:attribute_name, "attribute value")
+    expect(proxy.static_attributes).to_not be_empty
+  end
+
   it "declares a dynamic attribute on the factory" do
     attribute_value = -> { "dynamic attribute" }
     proxy.add_attribute(:attribute_name, &attribute_value)
@@ -37,6 +42,11 @@ describe FactoryBot::DefinitionProxy, "#add_attribute when the proxy ignores att
     expect(subject).to have_static_declaration(:attribute_name).ignored.with_value("attribute value")
   end
 
+  it "does not track statically-defined ignored attributes" do
+    proxy.add_attribute(:attribute_name, "attribute value")
+    expect(proxy.static_attributes).to be_empty
+  end
+
   it "declares a dynamic attribute on the factory" do
     attribute_value = -> { "dynamic attribute" }
     proxy.add_attribute(:attribute_name, &attribute_value)