Recent `any_instance` Change Regarding Superclasses Causes Spec Failure #180

Closed
jfelchner opened this Issue Aug 25, 2012 · 8 comments

Projects

None yet

3 participants

@jfelchner

I'm not exactly sure what is going on here, and although I don't have time to figure out how to fix it, I think I've done 99% of the forensic analysis for you all. :)

I've tracking the point of failure down to this commit: 1272c8a

I'm specifically stubbing with:

ActiveMerchant::Billing::Integrations::AuthorizeNetSim::Notification.any_instance.stub(:complete?).and_return false

and it is failing with:

Failure/Error: it { ActiveMerchant::Billing::Integrations::AuthorizeNetSim::Notification.new.complete?.should be_true }
     ArgumentError:
       wrong number of arguments (0 for 1)

Prior to this commit, all works as expected.

@alindeman
Contributor

I think it might be operating more correctly IMHO. It appears that the superclass (ActiveMerchant::Billing::Integrations::Notification) expects 1 argument passed to the constructor: https://github.com/Shopify/active_merchant/blob/1bce92d067caac3efb36ada94ae93912f5dde47c/lib/active_merchant/billing/integrations/notification.rb#L11

If you change it to:

it { ActiveMerchant::Billing::Integrations::AuthorizeNetSim::Notification.new("").complete?.should be_true }

does it stop erroring? I'm not totally sure what the argument means, but it looks like it needs to be a string or something that responds to #to_s with a string.

@jfelchner

@alindeman It's not the .new that's causing the problem. It's the #complete?.

In other words:

am = ActiveMerchant::Billing::Integrations::AuthorizeNetSim::Notification.new

Causes no errors.

am = ActiveMerchant::Billing::Integrations::AuthorizeNetSim::Notification.new
am.complete?

Throws the error.

@alindeman
Contributor

Yikes, I'm finally seeing it now: it's because ActiveMerchant::Billing::Integrations::AuthorizeNetSim::Notification defines a method method which overrides Object#method: https://github.com/Shopify/active_merchant/blob/1bce92d067caac3efb36ada94ae93912f5dde47c/lib/active_merchant/billing/integrations/authorize_net_sim/notification.rb#L96

I have at least one crazy idea to make it work, but need a bit more time to flesh it out. It's unfortunate that the class overrides a Object method, but given that it's not a __ method (like __send__), I understand that it's relatively common and think we should probably strive to support it.

Edit: I meant Object#method

@alindeman alindeman closed this in ea5b749 Aug 26, 2012
@alindeman alindeman reopened this Aug 26, 2012
@alindeman
Contributor

Opened #181 with a potential fix. Reviews welcomed.

@alindeman alindeman closed this in 645b158 Aug 26, 2012
@jfelchner

@alindeman I sincerely appreciate the quick response but shouldn't you have let me verify before you closed my issue? :/

@myronmarston
Member

@jfelchner -- we commonly use "Closes #blah" in commit messages to link it to the issue (and indicate that we believe the commit fixes the issue). That's what @alindeman did here. We can re-open if your issue isn't fixed.

Also, it sounds like you do a great job of following up on the issues that you file (thanks!) but lots of people don't...for example, see this issue where I've been waiting for the original poster to respond with more detail for a month. We like keeping fixed issues out of the open list so we can more easily focus on the remaining unfixed issues, so not knowing if you're the sort who will follow up, we tend to close issues when we're reasonably certain they've been fixed. We can always re-open if need be.

@jfelchner

@myronmarston thanks for the explanation and it's understandable. I just checked out the 645b158 and it indeed fixed my issue. @alindeman thanks again for the quick response!

@alindeman
Contributor

Agreed: thanks for the well-written bug report. I felt comfortable closing it because I used your report to write a spec that reproduced the issue. It passed after I made the fix.

If it hadn't fully covered it, I would have definitely reopened it if you had said something here.

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