Bug #64

String#modulo specs mocking goes too far

Added by Federico Builes 85 days ago. Updated 85 days ago.

Status :Feedback Start :08/26/2008
Priority :Normal Due date :
Assigned to :- % Done :

0%
Category :-
Target version :-

Description

One of the sample specs from String#modulo:



  it "calls #to_ary on argument for %c formats" do
    obj = mock('65')
    obj.should_receive(:to_ary).and_return([65])
    ("%c" % obj).should == ("%c" % [65])
  end

This smells like implementation details to me and it's all over the modulo specs. We shouldn't be worrying if it calls #to_ary or if it doesn't (as an example, Rubinius does not). I think that we should be checking that the final return values (with multiple inputs if necessary), checking the "what" it does, not "how".

What are your thoughts on this?

History

08/26/2008 11:21 PM - Vladimir Sizikov

I don't think that this is an "implementation detail".
This is a part of public behavior, it might be wrong, but definitely not impl. detail :)
Actually, I think that this is a natural behavior.
In most cases, #% expects the array of values, so calling to_ary seems logical.

08/27/2008 09:13 AM - Eero Saynatkari

Generally I would say a #to_ary call is never an implementation detail, because code may expect the conversion to take place.

Whether it is correct is a different matter. Arguably the user should always provide an Array but then again, if the object responds to #to_ary, it arguably is Array enough :) I would lean toward Vladimir's view in this particular case but overall it may be wise to keep an eye out for dubious conversions and escalate those cases as necessary.

08/27/2008 10:33 AM - Federico Builes

  • Status changed from New to Resolved

It's still not totally clear to me why we should spec this case. Consider this:


    obj = 65
    ("%c" % obj).should ==  "A"

Right now this works as expected on both MRI and Rubinius, but if we run the current spec:


   obj = mock('65')
    obj.should_receive(:to_ary).and_return([65])
    ("%c" % obj).should == "A"

It'll fail on Rubinius since #to_ary was never called (it was not needed). We'd be forcing every implementation to cast that to an Array . The solution for Rubinius would be to cast everything anytime something's passed in but somehow it feels like a bit of cargo culting that we don't need (the only tests that fail are the mock ones, the final result for each case is the same).

Sorry to keep hanging to this specific thing but I somehow think it's wrong.

08/27/2008 10:39 AM - Wilson Bilkovich

How is to_ary not needed in the example case?
It seems like Rubinius is at fault here, not the spec.

08/27/2008 12:04 PM - Eero Saynatkari

  • Status changed from Resolved to Feedback

Actually, let me revise after having actually read and considered this particular case. I maintain that #to_ary is never an implementation detail.

The description seems to be the clearest: if the format string contains more than one substitution, #to_ary should be called (unless the argument is already an Array, maybe, it should be just as fast to do the no-op conversion on an Array than to check first. That is an implementation detail.) Otherwise it should not be called. I do not believe any of the format specifiers expects an Array argument.

So I think `"%c" % obj` calling #to_ary would actually be a bug. `"%c %c" % obj` on the other hand should.

Trying this out in IRB, it is not actually calling #to_ary even when it is supposed to. %c at least seems to attempt an Integer conversion and failing there. Perhaps I am just confused.

08/27/2008 02:35 PM - Federico Builes

Waiting on clarification from ruby-core: http://redmine.ruby-lang.org/issues/show/512

Also available in: Atom PDF