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
_______________________________________________
rspec-users mailing list
rspec-users@rubyforge.org
http://rubyforge.org/mailman/listinfo/rspec-users
