Conflict between RSpec::Matchers#change, ActiveRecord::AttributeMethods::TimeZoneConversion, and ActiveSupport::TimeWithZone#method_missing #208

Closed
halostatue opened this Issue Feb 13, 2013 · 2 comments

Comments

Projects
None yet
2 participants
Contributor

halostatue commented Feb 13, 2013

I have a model in Rails that includes a timestamp field. I have a FactoryGirl factory for said model. I have a test that creates an instance from the factory. (Shown below, more or less; this is not a minimum reproducible, but what I have deduced is happening.)

class User < ActiveRecord::Base
end

FactoryGirl.define do
  factory :user
    registration_date { Time.now }
  end
end

describe User do
  it "should set the registration date" do
    user = FactoryGirl.create(:user)
  end
end

Because of a change in Rails 3.2.9, ActiveRecord::AttributeMethods::TimeZoneConversion includes a private method (#round_usec) that looks just like this:

def round_usec(value)
  return unless value
  value.change(:usec => 0)
end

Note that this change has been reverted in an unreleased version of Rails 3.2 (rails/rails@97a4db9), so it won't exist for long as such.

When I run my test, I get TypeError: nil is not a symbol. After much debugging and modification of gems in situ to understand this better (the initial error was on a different factory that depends on the user factory), I determined that this is because the value comes from the database as a ::Time object, is immediately wrapped with ActiveSupport::TimeWithZone, and then this incorrect code in Rails calls #round_usec which calls value#change. This is added to ::Time in active_support/core_ext/time/calculations.rb. However, ActiveSupport::TimeWithZone implements naïve delegation (through #method_missing) to its contained #time object (something that #acts_like_time?). It does not implement #change directly, it implements it through #method_missing.

Here's where RSpec::Matchers#change comes in: it gets inserted into the ancestors list and provides a #change method to ActiveSupport::TimeWithZone (see below).

[
  ActiveSupport::TimeWithZone, Comparable, Object,
  RSpec::Matchers, PP::ObjectMixin, Delayed::MessageSending,
  MongoMapper::Extensions::Object, JSON::Ext::Generator::GeneratorMethods::Object,
  ActiveSupport::Dependencies::Loadable, Kernel, BasicObject,
  RSpec::Mocks::Methods
]

Note that I haven't called any matchers myself, but this seemingly innocuous change causes rspec to do something unexpected and break almost all of my tests. I'm not sure if restricting my specs to the new syntax would help or hurt in this case, but it's not feasible as we've got about 1500 tests that would need to be converted (fortunately, a number of those test runs are through shared examples, but that only covers 1/3 of the tests).

This may not be fixable because ActiveSupport::TimeWithZone just uses #method_missing to do its delegation, but this is a problem…and I'm honestly not sure how no one has reported it until now.

I have been able to work around this problem with the following patch for tests:

if ActiveRecord::AttributeMethods::TimeZoneConversion.private_instance_methods.include?(:round_usec)
  module ActiveRecord
    module AttributeMethods
      module TimeZoneConversion
        def round_usec(value)
          return unless value
          return unless value.respond_to? :time

          result = value.time.__send__(:change, :usec => 0)
          result.acts_like?(:time) ? value.class.new(nil, value.time_zone, result) : result
        end
      end
    end
  end
else
  warn "Support file #{__FILE__} is no longer necessary."
end
Owner

myronmarston commented Feb 13, 2013

RSpec does not include RSpec::Matchers in every object -- rspec-core just mixes it into RSpec::Core::ExampleGroup so the methods are available from within your examples. There must be something else that is including RSpec::Matchers -- potentially into the top-level main object (which is essentially the same as including it in Object). RSpec::Matchers is not intended to be mixed into every object, and is likely to cause other problems for you down the road (even after a new rails version has been released), so I recommend putting effort into figuring out what is including RSpec::Matchers into everything and fixing that.

One simple way to track this down is to edit the RSpec::Matchers module in your local gem or git copy of the project. Add an included hook, and have that print the including class and caller (the backtrace). This should allow you to pinpoint where RSpec::Matchers is wrongly being included somewhere.

For reference, I just tried this in a project of mine and I'm only seeing 2 places where it gets included:


including in: RSpec::Matchers::DSL::Matcher
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-expectations-a46e7aa5d939/lib/rspec/matchers/matcher.rb:11:in `include'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-expectations-a46e7aa5d939/lib/rspec/matchers/matcher.rb:11:in `<class:Matcher>'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-expectations-a46e7aa5d939/lib/rspec/matchers/matcher.rb:8:in `<module:DSL>'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-expectations-a46e7aa5d939/lib/rspec/matchers/matcher.rb:5:in `<module:Matchers>'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-expectations-a46e7aa5d939/lib/rspec/matchers/matcher.rb:4:in `<module:RSpec>'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-expectations-a46e7aa5d939/lib/rspec/matchers/matcher.rb:3:in `<top (required)>'
/Users/myron/.rvm/rubies/ruby-1.9.3-p327/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
/Users/myron/.rvm/rubies/ruby-1.9.3-p327/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-expectations-a46e7aa5d939/lib/rspec/matchers.rb:186:in `<top (required)>'
/Users/myron/.rvm/rubies/ruby-1.9.3-p327/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
/Users/myron/.rvm/rubies/ruby-1.9.3-p327/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-expectations-a46e7aa5d939/lib/rspec/expectations.rb:2:in `<top (required)>'
/Users/myron/.rvm/rubies/ruby-1.9.3-p327/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
/Users/myron/.rvm/rubies/ruby-1.9.3-p327/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-fire-3b4480ad1bf1/lib/rspec/fire.rb:2:in `<top (required)>'
/Users/myron/.rvm/rubies/ruby-1.9.3-p327/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
/Users/myron/.rvm/rubies/ruby-1.9.3-p327/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
/Users/myron/code/vanguard/spec/spec_helpers/base.rb:9:in `<top (required)>'
/Users/myron/.rvm/rubies/ruby-1.9.3-p327/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
/Users/myron/.rvm/rubies/ruby-1.9.3-p327/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
/Users/myron/code/vanguard/spec/unit/util/db_helper_spec.rb:1:in `<top (required)>'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/configuration.rb:819:in `load'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/configuration.rb:819:in `block in load_spec_files'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/configuration.rb:819:in `each'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/configuration.rb:819:in `load_spec_files'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/command_line.rb:22:in `run'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/runner.rb:80:in `run'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/runner.rb:17:in `block in autorun'


including in: RSpec::Core::ExampleGroup
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/configuration.rb:813:in `include'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/configuration.rb:813:in `block in configure_expectation_framework'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/configuration.rb:812:in `each'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/configuration.rb:812:in `configure_expectation_framework'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/example_group.rb:270:in `ensure_example_groups_are_configured'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/example_group.rb:284:in `set_it_up'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/example_group.rb:241:in `subclass'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/example_group.rb:228:in `describe'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/dsl.rb:18:in `describe'
/Users/myron/code/vanguard/spec/unit/util/db_helper_spec.rb:4:in `<top (required)>'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/configuration.rb:819:in `load'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/configuration.rb:819:in `block in load_spec_files'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/configuration.rb:819:in `each'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/configuration.rb:819:in `load_spec_files'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/command_line.rb:22:in `run'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/runner.rb:80:in `run'
/Users/myron/code/vanguard/bundle/ruby/1.9.1/bundler/gems/rspec-core-9f8983f2b2e1/lib/rspec/core/runner.rb:17:in `block in autorun'
Contributor

halostatue commented Feb 13, 2013

That's exactly it, thanks. I'm not quite sure why we included RSpec::Matchers where we did—but I'll figure out what breaks by removing it.

halostatue closed this Feb 13, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment