Nosy controller specs

Testing RSpec
1

Looking at generated controller specs we tend to get something like

describe PostsController do
describe “handling GET /posts” do

before(:each) do
  @post = mock_model(Post)
  Post.stub!(:find).and_return([@post])
end

def do_get
  get :index
end
...

it "should find all posts" do
  Post.should_receive(:find).with(:all).and_return([@post])
  do_get
end

Now this last spec “should find all posts” is nosy is far as I’m
concerned
in that its specifying how the model should get all the posts (i.e.
white
box testing) rather than checking the result i.e. that its got @post
back.
So now if I change the model so that “all posts” is for example “all
posts
in last year” because there is a new business rule that we don’t show
posts
over a year old then my controller spec fails. Now it seems to me that I
should only have to change my model specs when I make this sort of
change,
this implementation details is none of the controllers business

So a couple of things

  1. Am I right about this?
  2. If so is there a better way to still use the mock for speed but not
    be
    nosy
  3. Should the default controller generators be re-written

TIA

2

yeah, instead of using Post.find(:all) in your controller, use a method
like
Post.all_posts, or some names like that. Then in the controller spec,
you
go:
Post.should_receive(:all_posts) and leave the implementation detail of
this
method to the Post model. Technically speaking, this all_posts more
belongs
to Service layer though.

Yi

3

Looking at this a bit further, it seems in Rails controllers decide how
things are got from Models i.e. by calling a class Method. I sort of
forgot
this for a bit. So with this pattern the testing is correct in
reflecting
the design of Rails. Still, with my OO purist hat on, it feels like this
responsibility should be in the model not in the controller, and that
Rails
has got it wrong here (fat controllers). Do other ruby frameworks take a
different approach?

2008/12/11 Andrew P. [email protected]

4

“Mark W.” [email protected] writes:

    then my controller spec fails.

exist in the controller and be tested there.
[email protected]
http://rubyforge.org/mailman/listinfo/rspec-users

I agree with this. You generally don’t want to screw with the meaning
of AR::Base.find. That’s really only for situations where you want some
uniform, transparent behavior (acts_as_versioned, acts_as_paranoid,
etc).

The view shouldn’t really care about this stuff either…it should just
take a list of posts and display them.

It’s up to the controller to ask the model for what it wants to push to
the view. So your finder can be something like Post.since(1.year.ago)
or whatever.

Andrew, I’ve noticed a theme in your posts that suggests you feel you
needn’t ever change controller specs. That’s just not true. Models
are a distilled representation of the domain, and controllers give
meaning to it in the context of the application you’re working on.
Models are what your app is and controllers are what your app does.
When you change what your app does, you’re going to have to change some
specs and production code along with it.

Keep in mind that if you went with a state-based test, you would still
have to modify the controller spec. It wouldn’t be good enough to
expect a post to be displayed. You’d have to create a post with a
created_at of over a year ago, and one within the last year, and expect
that the former is not shown while the latter is.

Also, I’d like to point out that if you really wanted Post.find(:all)
to only return posts in the last year, you wouldn’t actually have to
change the controller spec :slight_smile: You would just have to write a model spec
for that behavior.

Pat

5

Mark,

Thanks for your reply. As you say controllers should not be responsible
for
defining things, but in Rails this is exactly what they do, they
formulate
queries using class methods like find_by_xxx. Personally I think Rails
is
somewhat confused or perhaps lax in defining Controller responsibilities
compared to many MVC frameworks and particularly to the underlying
concept
of MVC.

As for this years posts being a presentation issue that depends. If the
request came from a view i.e. I’d like to see post for 2001 then the
controller should pass parameters to the model so it can return the
correct
things. On the other hand if last years posts represent a specific
business
concept something perphaps like AuditablePosts, then the model
could/should
represent this as a seperate resource. In Rails all business rules don’t
generally reside in the model, but maybe they should!

IMO of course :slight_smile:

2008/12/11 Mark W. [email protected]

6

On Thu, Dec 11, 2008 at 4:33 AM, Andrew P. [email protected]
wrote:

in last year" because there is a new business rule that we don’t show posts
over a year old then my controller spec fails.

I think this is probably correct as is.

When specing a controller, it’s correct BDD to specify how it interacts
with
other objects. If the controller itself wants all posts in the last
year,
it’s OK to test that. The fact that only this year’s posts are of
interest
is a presentation issue - it’s not an essential characteristic of the
data.
It’s not the model’s job to decide that all clients should get back
posts in
the last year, and hence that’s what its find method should return.

All business rules don’t reside in the model. As soon as you say “we
don’t
SHOW posts over a year,” you’re talking about a presentation rule, not a
model rule. Controllers mediate between data and screen - they’re
responsible for getting the data to be shown. That code should exist in
the
controller and be tested there.

On the other hand, the controller should not necessarily be responsible
for
defining what constitutes “all posts in the last year.” That should very
likely be a named scope.

This is all just my humble opinion, of course, and might be utter
rubbish in
any particular real world situation.

///ark

7

On Thu, Dec 11, 2008 at 11:28 AM, Andrew P. [email protected]
wrote:

As you say controllers should not be responsible for defining things, but
in Rails this is exactly what they do, they formulate queries using class
methods like find_by_xxx. Personally I think Rails is somewhat confused or
perhaps lax in defining Controller responsibilities compared to many MVC
frameworks and particularly to the underlying concept of MVC.

When I was talking about defining things, I meant that the controller
should
request “this year’s posts,” but shouldn’t define what that means
(calendar
year? fiscal year? which calendar? which timezone?). Put it another way

  • if
    there was a button on the view labeled “View this year’s posts”, it
    would be
    appropriate for the controller to call the this_years_posts method on
    the
    model, right? I was just extending that thinking to what the controller
    displays by default. But it’s not cut and dried. Pat mentioned things
    like
    acts_as_paranoid which controllers certainly should not be concerned
    with.

As for this years posts being a presentation issue that depends. If the
request came from a view i.e. I’d like to see post for 2001 then the
controller should pass parameters to the model so it can return the correct
things.

Don’t most requests come from views? I mean, you got to this page via a
user
gesture of some kind. (Parenthetically, I consider views the most
important
part of a web app - if the right thing is displayed to the right user at
the
right time, it doesn’t matter if it’s generated by a herd of monkeys.)

On the other hand if last years posts represent a specific business concept
something perphaps like AuditablePosts, then the model could/should
represent this as a seperate resource.

That sounds good. But the controller would still have to decide to
display
AuditablePosts on that page (and might display UnauditablePosts on
another
page). Again, it would know what result set - in domain terms - it wants
to
display. It shouldn’t care about how the model defines that result set.

I appreciate the opportunity to grope my way through my opinions on this
subject. :slight_smile:

///ark

8

We’re actually getting pretty close to saying the same thing here,
though my
use of language is causing some confusion. The two sorts of requests I
was
talking about where a view request and a request by the business for a
specific rule to be implemented.

If there were AuditablePosts the controller wouldn’t decide what display
it
could decide where to route (well actually the Dispatcher but you get my
drift) because this new resource would have its own controller as its a
seperate business entity.

A controller really shouldn’t be choosing between model entities, you
wouldn’t do this would you?

def index
if params[:auditable]
@posts = AuditablePosts.find(:all)
else
@posts = Posts.find(:all)
end
end

but we often end up with

def index
if params[:auditable]
@posts = Posts.find_auditable
else
@posts = Posts.find(:all)
end
end

which really isn’t much different. You mentioned something like

def index
if params[:auditable]
@posts = Posts.get_auditable_posts
else
@posts = Posts.find(:all)
end
end

which I think is better. However I kind of think we should have

def index
@posts = Posts.get_collection(params)

or have two resources and two controllers, and not have anything
inbetween.

Andrew

2008/12/11 Mark W. [email protected]

9

“Andrew P.” [email protected] writes:

@posts = AuditablePosts.find(:all)

else
@posts = Posts.find(:all)
end
end

I don’t see why not.

In fact, something I like to do is

def index
@posts = repository.find :all
end

def repository
params[:auditable] ? Post.auditable : Post
end

:open_mouth:

Makes a lot of sense to me. The controller’s basic job is to get some
stuff from the model and give it to the view. If the “get some stuff”
requires parameterization, fine by me.

Of course, there are plenty of ways you can structure your code. You
can use query params (and take advantage of routes to make it look like
a clean URL…hey, you’re still RESTful!) or a new action (again, still
RESTful), or create a whole new controller for it. That’s another
reason why REST is sweet from web app development perspective - stick to
a consistent URL scheme, and you can refactor the app beneath it however
you want.

Pat

10

Pat,

Thanks for reply. The one year ago thing was a really bad example. Using
your terminilogy it looks like a rule that changes what my app does.
But I
was talking about a change in what the app is; i.e. a business rule.

I definitely don’t want to have business rules in controllers. From my
viewpoint in MVC the controller is a conduit that allows the business
domain
encapsulated by the model to be presented in views and in reverse allows
interaction with views to communicate with the model. In Rails though
its
very easy for the business domain to creep into the controller. I’m
beggining to think that Rails got this wrong. Perhaps it should have
specified a restrictive api between the model and the controller, I’ve
been
thinking of something like

get_collection
get_object
get_new_object

By giving completely open access to anything in the model including all
classes its real easy for controllers to get real messy. I think the
emergence of Presenters as well as skinny controllers is symptomatic of
that
problem

With a resource based approach the controller really should only be
requesting resources or more accurately representations of resources. It
shouldn’t be creating/specifying representations of resources. That is
outside the responsibility of a controller. Again using your terminology
REST restricts what an application does by using a standard set of verbs
to
do things. Now in Rails we can pollute this by putting all sorts of
stuff in
our restful methods, but really we shouldn’t. Instead we should use the
expressivness of what ‘is’ to get our functionality. If we do this then
our
controllers can be ultra skinny and our controller specs should be much
less
brittle. To change what our app ‘does’ we then add new routes,
controllers
and views to show a different representation from our model but still
using
our standard verbs.

If controllers only use the restrictive api I’ve suggested and we
combine
this with a ‘presenter’ layer (when required) on top of our model. then
controller specs really can be very straightforward and focus on their
key
responsibilities which are getting something and controlling i.e.
routes.

I’ll try and give a better example. In my business domain I have
products
and some of these products have refills which are also products.
Generally
when I want to look at all products I don’t want to see the refills.
This is
because the refills are bought and specified through their product. So
using
tags when I want to find ‘all’ products I’ll actually do the following
bizzarre thing

Product.find_tagged_with(‘refill’, :exclude => ‘refill’)

Now generally in Rails this will be done in my controllers index method,
and
with rspecs scaffold code we’d mock Product and test that this method
was
called the correct way. So as my call has changed from
Product.find(:all) to
this new call I have to change my controller spec. Now changing what all
products means has nothing to do with controllers and everything to do
with
models. What my controller should have done is called
Product.get_collection. Then when I change my definition of what
get_collection does then yes I have to update my model spec, but that
makes
sense, and my controller and its specs remain the same.

I don’t see controller specs as being fixed, but I do think they should
only
need to change when something a controller is responsible for changes.
Generally this is routing - controlling where we go - which is
fundamentally
how web applications do things.

Anyhow hope that makes sense and once again thanks for the input much
appreciated

Andrew

2008/12/11 Pat M. [email protected]

11

“Andrew P.” [email protected] writes:

reverse allows interaction with views to communicate with the
model. In Rails though its very easy for the business domain to creep
into the controller. I’m beggining to think that Rails got this
wrong. Perhaps it should have specified a restrictive api between the
model and the controller, I’ve been thinking of something like

get_collection
get_object
get_new_object

You do… find(:all), find(id) and new(attributes)

By giving completely open access to anything in the model including
all classes its real easy for controllers to get real messy. I think
the emergence of Presenters as well as skinny controllers is
symptomatic of that problem

Oh. I’m a software libertarian. Arguments for locking stuff down are
NEVER going to fly with me.

Look, Ruby is a very powerful language. As Spiderman teaches us, with
great power comes great responsibility. Just because stuff can get
messy doesn’t mean we need to lock down the language or framework. It
means we need to be good developers, inventing useful conventions and
abstractions.

Do you know the secret to having a clean controller? Don’t write one!
Use something like make_resourceful.

method, and with rspecs scaffold code we’d mock Product and test that
this method was called the correct way. So as my call has changed from
Product.find(:all) to this new call I have to change my controller
spec.

Well, yeah. You’re asking for a different thing now. You need to
verify that somehow. Either you use mock objects to expect a different
method call, or you write an expectation that you should see one product
and not another.

Now changing what all products means has nothing to do with
controllers and everything to do with models. What my controller
should have done is called Product.get_collection. Then when I change
my definition of what get_collection does then yes I have to update my
model spec, but that makes sense, and my controller and its specs
remain the same.

If you really, really want that, why not just change Product.find(:all)
to return something different? Same thing. I don’t like it, but same
thing :slight_smile:

I think you’re overlooking the fact that all of this stuff must occur in
context. As in “show me all the products” is a straightforward
statement, yes, but is missing some info. As you yourself have stated,
“all” can mean something different than “every single record which
exists in our system.” The clearest way to represent these contextual
differences, in my experience, is to create an intention-revealing
method and use that.

Pat

12

2008/12/11 Pat M. [email protected] writes

viewpoint in MVC the controller is a conduit that allows the business

Look, Ruby is a very powerful language. As Spiderman teaches us, with
great power comes great responsibility. Just because stuff can get
messy doesn’t mean we need to lock down the language or framework. It
means we need to be good developers, inventing useful conventions and
abstractions.

I’m looking to find better conventions (for me) to use the power of
rails
responsibly. But probably I’m betraying my Java/C++ heritage by using
the
word rules instead of convention

In the design of any web framework and in Rails lots of
rules/conventions
exist about what you can do and where you can do things. For example
Rails
is quite happy to restrict what you can access in views by default.

Do you know the secret to having a clean controller? Don’t write one!
Use something like make_resourceful.

Thats exactly what I’m looking at. The trouble with these is that the how
of object retrieval ends up buried in ‘magic’ code somewhere, so when
you
need to change things you end up having to overload something somewhat
obscure in the controller, and then having to test this overloading
occurs
in a controller spec. For example using resource_controller and the
example
below I’d end up with something like

class ProductsController < ResourceController::Base
private
def collection
@collection ||=
end_of_association_chain.find_tagged_with(‘refill’, :exclude =>
‘refill’)
end
end
end

This sort of code isn’t very intuitive and (for me) it feels as though
its
in the wrong place

Now generally in Rails this will be done in my controllers index

Now changing what all products means has nothing to do with
controllers and everything to do with models. What my controller
should have done is called Product.get_collection. Then when I change
my definition of what get_collection does then yes I have to update my
model spec, but that makes sense, and my controller and its specs
remain the same.

If you really, really want that, why not just change Product.find(:all)
to return something different? Same thing. I don’t like it, but same
thing :slight_smile:

Because I don’t like it either its really ugly, and its far to clever
for me

I think you’re overlooking the fact that all of this stuff must occur in
context. As in “show me all the products” is a straightforward
statement, yes, but is missing some info. As you yourself have stated,
“all” can mean something different than “every single record which
exists in our system.” The clearest way to represent these contextual
differences, in my experience, is to create an intention-revealing
method and use that.

The point I’m making is that this intention should be expressed clearly
in
the model, whereas currently its expressed in the controller.

Pat

rspec-users mailing list
[email protected]
http://rubyforge.org/mailman/listinfo/rspec-users

Thanks for providing lots of input, its been most interesting

Andrew

13

I was agreeing! I wasn’t saying that what you were saying there wasn’t
expressing the intent in the model, I was saying that my long ramble was
saying the same thing roughly, and that this differs from the standard
approach in rails of using ActiveRecord:Base class methods directly.

Apologies for not making that clear - I find email such a challenging
form
of communication at times :slight_smile:

Andrew

2008/12/12 Pat M. [email protected]

14

“Andrew P.” [email protected] writes:

2008/12/11 Pat M. [email protected] writes

The clearest way to represent these contextual differences, in my
experience, is to create an intention-revealing method and use that.

The point I’m making is that this intention should be expressed
clearly in the model, whereas currently its expressed in the
controller.

What? No. I said create methods with intention-revealing names. That
means stuff like Post.since or Account.audited or
Product.non_refillable. That absolutely is expressing the intent in the
model, and calling it from the controller, which is what you want.

Pat