Returning different values in sequence using any_instance doesn't work as expected

[dwilkie]

The following snipit works well:

foo = mock("Foo")
foo.stub(:bar).and_return('first', 'second', 'other')
p foo.bar
p foo.bar
p foo.bar

Output:

# "first"
# "second"
# "other"

However the following case does not:

Array.any_instance.stub(:sample).and_return('first', 'second', 'other')
p [].sample
p [].sample
p [].sample

Output:

# "first"
# "first"
# "first"

[dchelimsky]

@kaiwren one more for you :) I'm happy to start looking at these if you don't have time, but it'd be great if you and/or your team do.

[kaiwren]

@dchelimsky I'll get on the job this weekend. My apologies for the tardiness. /cc @deobald @achamian @srushti @aakashd

[justinko]

Sidu unleashes his minions! :wink:

[kaiwren]

Hah! They're in cc because they're supposed to be nagging me to get this done :D

[aakashd]

The pull request for issue #137 just verifies the current behavior thought specs. Please let me know if this is correct.

I think, the stubbed method call to multiple instances should ideally fail, but it does not. If that's the case, we can close this issue and raise another one for this bug.

[alindeman]

I'm not sure about this one. We definitely have different behavior from mocha:

$ irb -rubygems -rmocha
1.9.3p194 :003 > Foo.any_instance.stubs(:bar).returns(:one, :two, :three)
1.9.3p194 :004 > Foo.new.bar
 => :one 
1.9.3p194 :005 > Foo.new.bar
 => :two 
1.9.3p194 :006 > Foo.new.bar
 => :three 

But look at this with rspec/mocks:

$ irb -Ilib -rrspec/mocks
1.9.3p194 :008 > Foo.any_instance.stub(:bar).and_return(:one, :two, :three)
1.9.3p194 :009 > f = Foo.new
 => #<Foo:0x007f90e4b34828> 
1.9.3p194 :010 > f.bar
 => :one 
1.9.3p194 :011 > f.bar
 => :two 
1.9.3p194 :012 > f.bar
 => :three 

For a particular instance of an object, we return values in the correct order. But it doesn't return the sequential values across instances.

What is the better behavior?

[kaiwren]

I'm not sure either - the current behaviour is less surprising to me simply because I'm used to treating RSpec's any_instance as "every instance."

I'm also trying to think of use cases where I'd want a sequence to play out over multiple instances, but can't think of one - I personally find this behaviour surprising. The advantage of mocha's approach, though, is that mocha's behaviour is a superset of RSpec's in this respect - if I continue to call the stubbed method the same instance in mocha, the behaviour is identical to RSpec's.

[alindeman]

I'm not sure it's a superset. What about this case? Never mind, I see you said "the same instance in mocha." But what do you think about these behaviors? Which one is more surprising? Useful?

$ irb -rubygems -rmocha
1.9.3p194 :003 > Foo.any_instance.stubs(:bar).returns(:one, :two, :three)
1.9.3p194 :004 > f = Foo.new
1.9.3p194 :005 > f.bar
 => :one 
1.9.3p194 :006 > f.bar
 => :two 
1.9.3p194 :007 > f2 = Foo.new
1.9.3p194 :008 > f2.bar
 => :three 

1.9.3p194 :004 > Foo.any_instance.stub(:bar).and_return(:one, :two, :three)
1.9.3p194 :005 > f = Foo.new
1.9.3p194 :006 > f.bar
 => :one 
1.9.3p194 :007 > f.bar
 => :two 
1.9.3p194 :008 > f2 = Foo.new
1.9.3p194 :010 > f2.bar
 => :one 

[kaiwren]

I'd say that if you need the sequence to hold true across different instances (which is what mocha does) then you're in a truly weird situation. I'm trying to think of how this would even happen unless you're explicitly writing code that shares state across different instances magically (maybe a common real world resource like a printer/db?).

[dwilkie]

So why doesn't it work as expected for array#sample isn't that the same instance

On Friday, June 29, 2012, Sidu Ponnappa < reply@reply.github.com> wrote:

I'd say that if you need the sequence to hold true across different instances (which is what mocha does) then you're in a truly weird situation. I'm trying to think of how this would even happen unless you're explicitly writing code that shares state across different instances magically (maybe a common real world resource like a printer/db?).

---

Reply to this email directly or view it on GitHub: https://github.com/rspec/rspec-mocks/issues/137#issuecomment-6659439

[alindeman]

@dwilkie, each time [] is run, it's a new Array.

[alindeman]

I'm inclined to think that this is expected behavior, or at least a reasonable design difference ... but not a bug. Any arguments the other way (with real use cases to back it up)?

[dwilkie]

@alindeman thanks for the explanation RE when [] is run, it's a new array, but surely Array.any_instance.stub(:sample).and_return(:one, :two, :three) is a classic real use case you need the behavior like Mocha

[alindeman]

Is there a real application and real test case where stubbing Array#sample with sequential return values across instances is needed (or even more importantly, is needed and can't be achieved in a cleaner way)?

[dwilkie]

This was my case:

describe "Reply"
  describe "#greet" do
    let(:reply) { create(:reply) }

    before do
      Array.any_instance.stub(:sample).and_return("Hello, how are you today?", "Hi! What are you doing?")
    end

    it "should send a random icebreaker" do
      reply.greet.should include("Hello, how are you today?")
      reply.greet.should include("Hi! What are you doing?")
    end 
  end
end

class Reply < ActiveRecord::Base
  def greet
    I18n.t(:greet, :locale => user.locale)
  end
end

# config/locals/en.rb

:en => {
  :greet => lambda {|key, options|
    ["Hello, how are you today?", "Hi! What are you doing?"].sample
  }
}

After writing this example and from the explanation from @alindeman that each time [] is run, it's a new Array would rewriting this functionality to be something like this work?:

 # config/locals/en.rb
 ICE_BREAKERS = ["Hello, how are you today?", "Hi! What are you doing?"]

:en => {
  :greet => lambda {|key, options|
    ICE_BREAKERS.sample
  }

[alindeman]

What if you were to instead stub I18n.t? That is the direct dependency/collaborator.

I18n.stub(:t).with(:greet, locale: user.locale).and_return("Hello, how are you today?", "Hi! What are you doing?")

[dwilkie]

@alindeman: What if the code is actually:

:en => {
  :greet => lambda {|key, options|
    message = " Want to play sms with me"
    ice_breaker = ["Hello, how are you today?", "Hi! What are you doing?"].sample
    ice_breaker << message
  }
}

Here I just want to assert that the ice breaker is random, so I don't want to stub i18n.t since the logic for making the icebreaker random is inside the locale file

[alindeman]

Hmm, I'm not yet sure how that changes things. Can you explain?

[dwilkie]

I updated the comment above, to explain but in this case I want to assert that the ice breaker part of the generated message is random. This logic that generates the random message is inside the locale file (which is where it needs to be because in some other locales there is no random ice breaker), so I don't want to stub out i18n.t

[alindeman]

I'm thinking your tests are pushing back at you a bit: the design here is weird, as you're trying to assert about something in a unit test that's actually occurring in a different object than is under test.

What if you were to test these pieces in isolation, stubbing I18n.t in the test for Reply and in a separate test, asserting that the locale file generates a random ice breaker when asked (via Array.any_instance.should_receive(:sample) or ICE_BREAKERS.should_receive(:sample))?

[kaiwren]

I agree with @alindeman on your tests exposing a coupled design - any_instance should only be used when you don't easily have a handle on object creation. For example: When you're using a library/framework that goes through a complex process of object construction that you can't trivially stub because it would be too convoluted/brittle.

IMO, having to use any_instance for code you control is a smell.

Furthermore, logic in your locale files is akin to logic in your views. I'd recommend moving that randomization logic into a model meant to handle it and your testing will become easier.

[alindeman]

Furthermore, logic in your locale files is akin to logic in your views. I'd recommend moving that randomization logic into a model meant to handle it and your testing will become easier.

Agreed here too. It feels like having it directly in the locale file is wrong. (Also, how do translators deal with that?)

[dwilkie]

Hmm I'd have to disagree with you both here and say that this logic does belong in the locale file, because as I said here (https://github.com/rspec/rspec-mocks/issues/137#issuecomment-6690425) it differs for different locales, i.e. for some languages there is no random ice breaker but we use other logic to generate the message. However I do agree, and I posted this comment (https://github.com/rspec/rspec-mocks/issues/137#issuecomment-6690181) that this random data should be in a constant. The code that I had has since actually changed so I haven't tested it but having would having it in a constant fix the problem?

[kaiwren]

I prefer to wrap native/framework types with explicit wrappers that express intent and encapsulate custom behaviour.

Something like

class RandomLocalizedMessage
  def initialize(key, options)
     @key = key
     @options = options
  end

  def sample(additional_message = "")
     I18n.t(@key, @options).sample + additional_message
  end
end

:en => {
  :greet => lambda {|key, options|
     ["Hello, how are you today?", "Hi! What are you doing?"]
  }
}

class Reply < ActiveRecord::Base
  def greet
    RandomLocalizedMessage.new(:greet, :locale => user.locale).sample(" Want to play sms with me")
  end
end

Now that it's decoupled, you can easily unit test

• The randomization logic
• The appending logic

And you don't need to stub anything other than I18n.t to do this. Makes sense?

Basically, if you can't test drive some piece of logic, either your design is flawed or Rails is getting in your way. In this case I'm guessing it's the former, but I could be wrong.

[dwilkie]

Hmm ok that sounds reasonable. So if there is no other case where Array#sample would need to be stubbed with different return values I guess there is no need to change the current behavior.

On Tuesday, July 3, 2012, Sidu Ponnappa wrote:

I prefer to wrap native/framework types with explicit wrappers that express intent and encapsulate custom behaviour.

Something like

class RandomLocalizedMessage
  def initialize(key, options)
     @key = key
     @options = options
  end

  def sample(additional_message = "")
     I18n.t(@key, @options).sample + additional_message
  end
end

:en => {
  :greet => lambda {|key, options|
     ["Hello, how are you today?", "Hi! What are you doing?"]
  }
}

class Reply < ActiveRecord::Base
  def greet
    RandomLocalizedMessage.new(:greet, :locale => user.locale).sample("
Want to play sms with me")
  end
end

Now that it's decoupled, you can easily unit test

• The randomization logic
• The appending logic

And you don't need to stub anything other than I18n.t to do this. Makes sense?

Basically, if you can't test drive some piece of logic, either your design is flawed or Rails is getting in your way. In this case I'm guessing it's the former, but I could be wrong.

---

Reply to this email directly or view it on GitHub: https://github.com/rspec/rspec-mocks/issues/137#issuecomment-6710084

[alindeman]

Cool. Closing this as a design difference, but not a bug. I tweeted about it and nobody else has weighed in :)