cfcosta / minitest-firemock

Makes your MiniTest mocks more resilient.
31 stars 1 forks source link

Bad constant lookups when inheritance chain is messy/poluted #8

Closed plukevdh closed 9 years ago

plukevdh commented 9 years ago

This may be something caused by Rails messing with the lookup chain or something, because I can't reproduce this standalone test suite, but our constantize method is pulling in root-level constants, even when the constant is found inside of a namespace. So constantize('MyThing::AConstant') will return ::AConstant if AConstant is also defined at the root level. I did this for something like the following:

require 'stripe'

class BillingService::Stripe
  # special handling...

  def charge
    # code...
  end
end

The Stripe library declares a module of Stripe at the root namespace level. Doing the following yields an error:

mock = Minitest::Firemock.new('BillingService::Stripe')
mock.expect(:charge, true)

Digging revealed that the constantize method was returning the root-level Stripe constant. While I can't prove a failure and a fix, I can prove that nothing breaks :) Tests added to confirm. Hopefully that's good enough.

plukevdh commented 9 years ago

Part of me wonders if it's a good idea to maintain our own constantize method... I like not having to deal with ActiveSupport, but I'm not sure what happens when another "special case" constantizer gets introduced into the mix. Might be worth doing a JavaScript library move and checking to see if constantize exists, and if so, use it.

cfcosta commented 9 years ago

I like the idea of using ActiveSupport constantize when defined, but wouldn't that make our test suite test only for our own constantizer? If there's a different behaviour between both, we'll not have enough control about it.

plukevdh commented 9 years ago

Good point. I'm fine with sticking with our own implementation unless we also have a test that includes active_support and tests with it to ensure compatibility. The downside to that is if Yet Another Library With constantize™ gets included and that one differs as well, we still won't catch that. Probably best if should just stick with our implementation.

plukevdh commented 9 years ago

Did some rebasery. Hopefully this will pass now.