rspec users - Apr 2011 - stubbing Model.find(id) issues

I''ve started a fresh rails app with Employee belongs_to Company.

Here is the spec:

describe Employee do
   example "stub should work with find(id)" do
     company = mock_model Company
     Company.stub!(:find).with(company.id).and_return company
     employee = Employee.new company_id: company.id
     employee.company.should == company
   end
end

And here is the result:

Failures:

   1) Employee stub should work with find(id)
      Failure/Error: employee.company.should == company
<Company(id: integer, name: string, created_at: datetime, updated_at: 
datetime) (class)> received :find with unexpected arguments
          expected: (1001)
               got: (1001, {:conditions=>nil})

Writing any of the following is not elegant:

   Company.stub!(:find).with(company.id, conditions: nil).and_return company
or
   Company.stub!(:find).with{|id, *args| id == company.id }.and_return 
company # find could be called like Company.find(1, 2, 3) which should 
return an array instead
or
   Company.stub!(:find).with{|id, *args| id == company.id && (args.size 
== 0 || (args.size == 1 && args[0].is_a?(Hash)) }.and_return company

So, it would be great to add some syntax to rspec-rails like:

    Company.stub!(:find).with_id(company.id).and_return company

or

    Company.stub_find_with!(company.id).and_return company

I''m not sure yet what changes would be better, but the current 
implementation makes it very hard to read specs like this that are so 
common when mocking models.

What do you think?

On Apr 24, 2011, at 9:29 AM, Rodrigo Rosenfeld Rosas wrote:
> I''ve started a fresh rails app with Employee belongs_to Company.
> 
> Here is the spec:
> 
> describe Employee do
>  example "stub should work with find(id)" do
>    company = mock_model Company
>    Company.stub!(:find).with(company.id).and_return company
>    employee = Employee.new company_id: company.id
>    employee.company.should == company
>  end
> end
This ^^ is specifying Rails'' behavior. There is a rather large test
suite that does this already, so I''d recommend avoiding this sort of
example in a model spec. Use-case specifics aside ...
> And here is the result:
> 
> Failures:
> 
>  1) Employee stub should work with find(id)
>     Failure/Error: employee.company.should == company
> <Company(id: integer, name: string, created_at: datetime, updated_at:
datetime) (class)> received :find with unexpected arguments
>         expected: (1001)
>              got: (1001, {:conditions=>nil})
> 
> Writing any of the following is not elegant:
> 
>  Company.stub!(:find).with(company.id, conditions: nil).and_return company
> or
>  Company.stub!(:find).with{|id, *args| id == company.id }.and_return
company # find could be called like Company.find(1, 2, 3) which should return an
array instead
> or
>  Company.stub!(:find).with{|id, *args| id == company.id &&
(args.size == 0 || (args.size == 1 && args[0].is_a?(Hash)) }.and_return
company
> 
> So, it would be great to add some syntax to rspec-rails like:
> 
>   Company.stub!(:find).with_id(company.id).and_return company
> 
> or
> 
>   Company.stub_find_with!(company.id).and_return company
> 
> I''m not sure yet what changes would be better, but the current
implementation makes it very hard to read specs like this that are so common
when mocking models.
> 
> What do you think?
Totally agree that this _should_ be easy. The problem with this approach is that
it would add a dependency from rspec-rails on Rails'' internals. That
means that if/when the internals change, rspec-rails would break.

What about a more generalized `with` method that only constrains the first n
arguments?

  Company.stub(:find).with_args_including(company.id).and_return company

This would work with find(company.id) or find(company.id, anything_else), but
not find(anything_else, company.id). This doesn''t guarantee solving the
problem, but it reduces the risk of future breakage because the likelihood of
active record changing that first argument to find is very small.

Thoughts?

Em 24-04-2011 12:09, David Chelimsky escreveu:
> On Apr 24, 2011, at 9:29 AM, Rodrigo Rosenfeld Rosas wrote:
>
>> I''ve started a fresh rails app with Employee belongs_to
Company.
>>
>> Here is the spec:
>>
>> describe Employee do
>>   example "stub should work with find(id)" do
>>     company = mock_model Company
>>     Company.stub!(:find).with(company.id).and_return company
>>     employee = Employee.new company_id: company.id
>>     employee.company.should == company
>>   end
>> end
> This ^^ is specifying Rails'' behavior. There is a rather large
test suite that does this already, so I''d recommend avoiding this sort
of example in a model spec. Use-case specifics aside ...
Yes, I know, I just wanted to make my point. This is not a real test case.
>> And here is the result:
>>
>> Failures:
>>
>>   1) Employee stub should work with find(id)
>>      Failure/Error: employee.company.should == company
>> <Company(id: integer, name: string, created_at: datetime,
updated_at: datetime) (class)>  received :find with unexpected arguments
>>          expected: (1001)
>>               got: (1001, {:conditions=>nil})
>>
>> Writing any of the following is not elegant:
>>
>>   Company.stub!(:find).with(company.id, conditions: nil).and_return
company
>> or
>>   Company.stub!(:find).with{|id, *args| id == company.id }.and_return
company # find could be called like Company.find(1, 2, 3) which should return an
array instead
>> or
>>   Company.stub!(:find).with{|id, *args| id == company.id&& 
(args.size == 0 || (args.size == 1&&  args[0].is_a?(Hash)) }.and_return
company
>>
>> So, it would be great to add some syntax to rspec-rails like:
>>
>>    Company.stub!(:find).with_id(company.id).and_return company
>>
>> or
>>
>>    Company.stub_find_with!(company.id).and_return company
>>
>> I''m not sure yet what changes would be better, but the current
implementation makes it very hard to read specs like this that are so common
when mocking models.
>>
>> What do you think?
> Totally agree that this _should_ be easy. The problem with this approach is
that it would add a dependency from rspec-rails on Rails'' internals.
That means that if/when the internals change, rspec-rails would break.
Not exactly, only if the Rails API changes which I don''t think it will 
happen for find in the near future.
> What about a more generalized `with` method that only constrains the first
n arguments?
>
>    Company.stub(:find).with_args_including(company.id).and_return company
>
> This would work with find(company.id) or find(company.id, anything_else),
but not find(anything_else, company.id). This doesn''t guarantee solving
the problem, but it reduces the risk of future breakage because the likelihood
of active record changing that first argument to find is very small.
>
> Thoughts?
The problem with this approach is that I don''t want Company.find(1, 2) 
to return Company.find(1), since it should return [Company.find(1), 
Company.find(2)]...

Actually, I was thinking about something like this:

company = mock_active_model(Company)
Company.find(company.id).should == company

I mean, whenever a mock for some ActiveModel or ActiveRecord instance is 
created, the class should also be stubbed to return that instance on 
find too. There are basically two find alternatives: find(id[, options]) 
and find(id1, id2...[, options]). These haven''t changed as long as I
can
remember, so I think it would be safe to include something like that on 
rspec-rails.

Do you agree?

Best regards, Rodrigo.

On Apr 24, 2011, at 6:25 PM, Rodrigo Rosenfeld Rosas wrote:
> Em 24-04-2011 12:09, David Chelimsky escreveu:
>> On Apr 24, 2011, at 9:29 AM, Rodrigo Rosenfeld Rosas wrote:
>> 
>>> I''ve started a fresh rails app with Employee belongs_to
Company.
>>> 
>>> Here is the spec:
>>> 
>>> describe Employee do
>>>  example "stub should work with find(id)" do
>>>    company = mock_model Company
>>>    Company.stub!(:find).with(company.id).and_return company
>>>    employee = Employee.new company_id: company.id
>>>    employee.company.should == company
>>>  end
>>> end
>> This ^^ is specifying Rails'' behavior. There is a rather large
test suite that does this already, so I''d recommend avoiding this sort
of example in a model spec. Use-case specifics aside ...
> 
> Yes, I know, I just wanted to make my point. This is not a real test case.
> 
>>> And here is the result:
>>> 
>>> Failures:
>>> 
>>>  1) Employee stub should work with find(id)
>>>     Failure/Error: employee.company.should == company
>>> <Company(id: integer, name: string, created_at: datetime,
updated_at: datetime) (class)>  received :find with unexpected arguments
>>>         expected: (1001)
>>>              got: (1001, {:conditions=>nil})
>>> 
>>> Writing any of the following is not elegant:
>>> 
>>>  Company.stub!(:find).with(company.id, conditions: nil).and_return
company
>>> or
>>>  Company.stub!(:find).with{|id, *args| id == company.id
}.and_return company # find could be called like Company.find(1, 2, 3) which
should return an array instead
>>> or
>>>  Company.stub!(:find).with{|id, *args| id == company.id&& 
(args.size == 0 || (args.size == 1&&  args[0].is_a?(Hash)) }.and_return
company
>>> 
>>> So, it would be great to add some syntax to rspec-rails like:
>>> 
>>>   Company.stub!(:find).with_id(company.id).and_return company
>>> 
>>> or
>>> 
>>>   Company.stub_find_with!(company.id).and_return company
>>> 
>>> I''m not sure yet what changes would be better, but the
current implementation makes it very hard to read specs like this that are so
common when mocking models.
>>> 
>>> What do you think?
>> Totally agree that this _should_ be easy. The problem with this
approach is that it would add a dependency from rspec-rails on Rails''
internals. That means that if/when the internals change, rspec-rails would
break.
> 
> Not exactly, only if the Rails API changes which I don''t think it
will happen for find in the near future.
> 
>> What about a more generalized `with` method that only constrains the
first n arguments?
>> 
>>   Company.stub(:find).with_args_including(company.id).and_return
company
>> 
>> This would work with find(company.id) or find(company.id,
anything_else), but not find(anything_else, company.id). This doesn''t
guarantee solving the problem, but it reduces the risk of future breakage
because the likelihood of active record changing that first argument to find is
very small.
>> 
>> Thoughts?
> 
> The problem with this approach is that I don''t want
Company.find(1, 2) to return Company.find(1), since it should return
[Company.find(1), Company.find(2)]...
> 
> Actually, I was thinking about something like this:
> 
> company = mock_active_model(Company)
> Company.find(company.id).should == company
> 
> I mean, whenever a mock for some ActiveModel or ActiveRecord instance is
created, the class should also be stubbed to return that instance on find too.
There are basically two find alternatives: find(id[, options]) and find(id1,
id2...[, options]). These haven''t changed as long as I can remember, so
I think it would be safe to include something like that on rspec-rails.
> 
> Do you agree?
> 
> Best regards, Rodrigo.
I definitely agree that it would be a nice to have if we could make it work
simply and reliably. That said, rspec-rails-1 has a long history of me having to
drop everything and do an urgent release immediately following a rails-2 release
because internals that we never thought would change did. rspec-rails-2,
however, has survived 7 rails-3 releases unscathed, and I intend to keep it that
way. One major reason for this is a severely reduced dependency on rails
internals.

In terms of the API, how would you expect this to work?:

  company = mock_active_model(Company, :id => 1)
  Company.find(1, 2)

or this:

  company = mock_active_model(Company, :id => 1, :published_on =>
8.days.ago)
  Company.find(1, :conditions => [ "published_on > ?",
7.days.ago ]

Etc.

Another concern is that named scopes defined with ARel do not go through the
find method, so this approach would not survive a refactoring from ActiveRecord
finders to scopes defined with ARel relations. Even if we can find "the
spot" through which all queries go, I would be resistant to depending on
its stability.

There is a similar thread, right now, btw, in an issue raised on rspec-rails:
https://github.com/rspec/rspec-rails/issues/358. Feel free to review my comments
there and add yours. I''m not closed to any of these ideas, but I am
opposed to introducing brittle dependencies, or new APIs that open the door for
a battery of new expectations/requirements.

Cheers,
David

Em 24-04-2011 23:39, David Chelimsky escreveu:
> On Apr 24, 2011, at 6:25 PM, Rodrigo Rosenfeld Rosas wrote:
>
>> Em 24-04-2011 12:09, David Chelimsky escreveu:
>>> On Apr 24, 2011, at 9:29 AM, Rodrigo Rosenfeld Rosas wrote:
>>>
>>>> I''ve started a fresh rails app with Employee
belongs_to Company.
>>>>
>>>> Here is the spec:
>>>>
>>>> describe Employee do
>>>>   example "stub should work with find(id)" do
>>>>     company = mock_model Company
>>>>     Company.stub!(:find).with(company.id).and_return company
>>>>     employee = Employee.new company_id: company.id
>>>>     employee.company.should == company
>>>>   end
>>>> end
>>> This ^^ is specifying Rails'' behavior. There is a rather
large test suite that does this already, so I''d recommend avoiding this
sort of example in a model spec. Use-case specifics aside ...
>> Yes, I know, I just wanted to make my point. This is not a real test
case.
>>
>>>> And here is the result:
>>>>
>>>> Failures:
>>>>
>>>>   1) Employee stub should work with find(id)
>>>>      Failure/Error: employee.company.should == company
>>>> <Company(id: integer, name: string, created_at: datetime,
updated_at: datetime) (class)>   received :find with unexpected arguments
>>>>          expected: (1001)
>>>>               got: (1001, {:conditions=>nil})
>>>>
>>>> Writing any of the following is not elegant:
>>>>
>>>>   Company.stub!(:find).with(company.id, conditions:
nil).and_return company
>>>> or
>>>>   Company.stub!(:find).with{|id, *args| id == company.id
}.and_return company # find could be called like Company.find(1, 2, 3) which
should return an array instead
>>>> or
>>>>   Company.stub!(:find).with{|id, *args| id ==
company.id&&   (args.size == 0 || (args.size == 1&&  
args[0].is_a?(Hash)) }.and_return company
>>>>
>>>> So, it would be great to add some syntax to rspec-rails like:
>>>>
>>>>    Company.stub!(:find).with_id(company.id).and_return company
>>>>
>>>> or
>>>>
>>>>    Company.stub_find_with!(company.id).and_return company
>>>>
>>>> I''m not sure yet what changes would be better, but the
current implementation makes it very hard to read specs like this that are so
common when mocking models.
>>>>
>>>> What do you think?
>>> Totally agree that this _should_ be easy. The problem with this
approach is that it would add a dependency from rspec-rails on Rails''
internals. That means that if/when the internals change, rspec-rails would
break.
>> Not exactly, only if the Rails API changes which I don''t think
it will happen for find in the near future.
>>
>>> What about a more generalized `with` method that only constrains
the first n arguments?
>>>
>>>    Company.stub(:find).with_args_including(company.id).and_return
company
>>>
>>> This would work with find(company.id) or find(company.id,
anything_else), but not find(anything_else, company.id). This doesn''t
guarantee solving the problem, but it reduces the risk of future breakage
because the likelihood of active record changing that first argument to find is
very small.
>>>
>>> Thoughts?
>> The problem with this approach is that I don''t want
Company.find(1, 2) to return Company.find(1), since it should return
[Company.find(1), Company.find(2)]...
>>
>> Actually, I was thinking about something like this:
>>
>> company = mock_active_model(Company)
>> Company.find(company.id).should == company
>>
>> I mean, whenever a mock for some ActiveModel or ActiveRecord instance
is created, the class should also be stubbed to return that instance on find
too. There are basically two find alternatives: find(id[, options]) and
find(id1, id2...[, options]). These haven''t changed as long as I can
remember, so I think it would be safe to include something like that on
rspec-rails.
>>
>> Do you agree?
>>
>> Best regards, Rodrigo.
> I definitely agree that it would be a nice to have if we could make it work
simply and reliably. That said, rspec-rails-1 has a long history of me having to
drop everything and do an urgent release immediately following a rails-2 release
because internals that we never thought would change did. rspec-rails-2,
however, has survived 7 rails-3 releases unscathed, and I intend to keep it that
way. One major reason for this is a severely reduced dependency on rails
internals.
>
> In terms of the API, how would you expect this to work?:
>
>    company = mock_active_model(Company, :id =>  1)
>    Company.find(1, 2)
>
> or this:
>
>    company = mock_active_model(Company, :id =>  1, :published_on => 
8.days.ago)
>    Company.find(1, :conditions =>  [ "published_on>  ?",
7.days.ago ]
>
> Etc.
>
> Another concern is that named scopes defined with ARel do not go through
the find method, so this approach would not survive a refactoring from
ActiveRecord finders to scopes defined with ARel relations. Even if we can find
"the spot" through which all queries go, I would be resistant to
depending on its stability.
>
> There is a similar thread, right now, btw, in an issue raised on
rspec-rails: https://github.com/rspec/rspec-rails/issues/358. Feel free to
review my comments there and add yours. I''m not closed to any of these
ideas, but I am opposed to introducing brittle dependencies, or new APIs that
open the door for a battery of new expectations/requirements.
I agree with you. I do think we should keep it as simple as possible 
relying as little much as possible on Rails itself.

Using your examples:

   company = mock_active_model(Company, :id =>  1)
   Company.find(1, 2)

I would expect this to return [company, nil] unless Company.find(2) 
actually returns something. Behind the scenes, Company.find(1) would use 
the stub, while Company.find(2) would not.

   company = mock_active_model(Company, :id =>  1, :published_on => 
8.days.ago)
   Company.find(1, :conditions =>  [ "published_on>  ?",
7.days.ago ]

I would expect this to return company, regardless on the conditions. 
This should be explicitated on documentation of course. The idea is not 
to replicate Rails here, but just to make it easy for constructing basic 
object creation and simple relationships like belongs_to and has_many. 
This should not work on scopes as it would be too complicated and 
dependent on Rails internals.

I recognize that this last example is not that easy to decide on, since 
the above approach could lead to some unexpected behaviors regarding 
scopes for instance.

Maybe another approach would be defining something like that, although 
this is a bit specific to Rails internals:

def mock_active_model(model, options={})
   mock_model(model, options).tap do |m| # I know, I know, ignore tap 
and the new hash syntax and you get the idea
     model.stub!(:find).with {|id, *options|
       id == m.id && (options.size == 0 || (options.size == 1 &&
options[0] == {conditions: nil}) )
     }.and_return(m)
   end
end

That way, the following call won''t be stubbed:

   Company.find(1, :conditions =>  [ "published_on>  ?",
7.days.ago ]

I understand that this is not much desired because it creates a little 
dependency on Rails internals, but I do see more benefits... On the 
other side, supporting scopes doesn''t pay off in my opinion.

Here is why I think that this is not a big issue. Rails find API hasn''t
change as far as I remember. It just happens that Rails used to call 
Company.find(1) instead of Company.find(1, conditions: nil). But this 
was already possible before Rails 3. And the results should remain the same.

On the other side, if Rails decides to change this in some next release 
to use another useless option, the changes in RSpec would be trivial, I 
guess, although it might clutter the code with lots of "if 
Rails.version==...". But the good news is that I don''t think that
things
like that changes very often in small releases, and bigger releases use 
to happen in a long time. Also, the Rails core team decided to publish 
some release candidate before each final release. This provides us the 
chance to request them to rollback some change that would break RSpec if 
it is simple for them to fix the issue...

I''m only asking for supporting the commonest case, because it is too 
common and too hard to understand what the mock is doing as it is 
implemented now...

class A; end
class B
   belongs_to :a
end

When someone needs some scope or is avoiding the conventions, then I 
don''t think he or she would mind to write a bit more in mocks too...

I wish I could have some definite answer to your questions, but 
unfortunately I don''t and I understand your concerns and I know you
also
did understand mine. Maybe someone else could enlight us with another 
approach :)

Best regards,
Rodrigo.