Possible improvements to routing spec API

[Wincent_C]

Hi folks,

I’ve been unhappy with routing specs for a long time now and last night
when updating some old 1.3 specs for 2.0 I decided to see if I could
come up with something that didn’t make me feel unhappy.

Principal causes of unhappiness:

1. Historically we had “route_for” and “params_from”, which felt awfully
   repetitive because we ended up doing:

route_for(lengthy_hash_of_params).should ==
string_or_hash_describing_destination
params_from(list_describing_destination).should ==
lengthy_hash_of_params

Of course, it was worse than that in practice because those two lines
usually appeared in separate example blocks with the associated
boilerplate. It felt like a lot of work for testing such a simple thing.
It also felt irritating to have to repeat basically the same thing twice
but in a different order.

2. So then RSpec gave us “route_to”, which is a wrapper for Rails’
   “assert_routing”; being a bi-directional test that encompasses the
   function of both “assert_recognizes” and “assert_generates”, this allows
   us to avoid some, or even all, of the repetition:

{ :get => ‘foo’ }.should route_to(:controller => ‘foo’, :action =>
‘index’)

The unhappiness here comes from three causes:

One is that { :get => ‘foo’ } feels inconsistent with other places in
RSpec like controller specs where “get” is a method, so we can do things
like “get ‘thing’”.

The second issue is that the “to” in “route_to” feels misleadingly
uni-directional when in reality it is a bi-directional test.

The third issue is that for routes which don’t actually have that
bi-directional mapping, “route_to” can’t be used and we must instead
drop down to Rails’ assert_recognizes() and/or assert_generates()
methods, or wrap them using our own matchers.

So I thought about what I would rather be writing and in my first cut
came up with this:

describe ArticlesController do
describe ‘routing’ do
example ‘GET /wiki’ do
get(‘/wiki’).should map_to(:controller => ‘articles’, :action
=> ‘index’)
get(‘/wiki’).should map_from(:controller => ‘articles’, :action
=> ‘index’)
articles_path.should == ‘/wiki’
end
end
end

Things to note:

• make the bi-directionality of the mapping explicit by having separate
  “map_to” and “map_from” lines.

• for ease of readability and writability, keep the order as “method →
  path → destination” for both assertions by using “to” and “from”,
  rather than swapping the order around

• “map” here is the right verb because we’ve always used that language
  to talk about how a given URL “maps to” a given controller#action. And
  remember how in the router DSL prior to Rails 3 everything in
  config/routes.rb started with “map”?

• I’ve tacked a test for the “articles_path” URL helper in there,
  because as a user of the Rails router I generally want to know two
  things: firstly, that requests get mapped to the appropriate
  controller#action; and secondly, that when I generate URLs (almost
  exclusively with named helpers; I use “url_for” in only 4 places in my
  entire app) that they take me where I think they take me. In the end,
  however, I moved this into a separate “describe ‘URL helpers’” block.

• conscious use of “example” rather than “it” because I want my specs to
  be identified as “ArticlesController routing GET /wiki” and not
  “ArticlesController routing recognizes and generates #index”.

• the repetition is a conscious choice because I value
  readability/scannability over DRYness-at-all-costs, especially in specs;
  the following is more DRY, for example, but less readable/scannable:

  path = ‘/wiki’
  destination = { :controller => 'articles, :action => ‘index’ }
  get(path).should map_to(destination)
  get(path).should map_from(destination)

So I went ahead and converted a bunch of specs to this syntax and found
that, surprise, surprise, in an application like this one where almost
everything consists of a “standard” RESTful resource, over 90% of routes
were testable in the bi-directional sense and in a typical routing spec
file I needed to use “map_to” with no corresponding “map_from” for only
one or two cases. So I needed a new method that meant “map_to_and_from”.

Funnily, I just can’t decide on a name for this method. As a placeholder
I am just using “map” for now:

get(‘/wiki’).should map(:controller => ‘articles’, :action => ‘index’)

But others I have tried are:

get(‘/wiki’).should map_as(:controller => ‘articles’, :action =>
‘index’)
get(‘/wiki’).should map_via(:controller => ‘articles’, :action =>
‘index’)
get(‘/wiki’).should map_with(:controller => ‘articles’, :action =>
‘index’)
get(‘/wiki’).should map_to_and_from(:controller => ‘articles’, :action
=> ‘index’)
get(‘/wiki’).should map_both(:controller => ‘articles’, :action =>
‘index’)
get(‘/wiki’).should map_both_ways(:controller => ‘articles’, :action
=> ‘index’)
get(‘/wiki’).should have_routing(:controller => ‘articles’, :action =>
‘index’)
get(‘/wiki’).should have_route(:controller => ‘articles’, :action =>
‘index’)
get(‘/wiki’).should be_route(:controller => ‘articles’, :action =>
‘index’)
get(‘/wiki’).should be_routing(:controller => ‘articles’, :action =>
‘index’)
get(‘/wiki’).should route_as(:controller => ‘articles’, :action =>
‘index’)
get(‘/wiki’).should route_via(:controller => ‘articles’, :action =>
‘index’)
get(‘/wiki’).should route(:controller => ‘articles’, :action =>
‘index’)
get(‘/wiki’).should <=> (:controller => ‘articles’, :action =>
‘index’)
get(‘/wiki’).should > (:controller => ‘articles’, :action => ‘index’)

map_to

get(‘/wiki’).should < (:controller => ‘articles’, :action => ‘index’)

map_from

If anybody has a suitable suggestion please let me know.

In the meantime, here is an example of a spec file that has been
converted to use this new “API”:

forums_routing_spec.rb · GitHub

It also includes the supporting code that adds these new “map”,
“map_to”, “map_from” matchers, and the “get”, “post”, “put” and “delete”
methods. All of this for Rails 3/RSpec 2 only.

I’m going to convert more routing specs and see if any more changes are
needed to handle edge cases, but for a first cut I am pretty happy with
the results, apart from my inability to decide on the right name for the
bi-directional “map” matcher.

Cheers,
Wincent

[Wincent_C]

On 5 Jul 2010, at 08:00, Wincent C. wrote:

The second issue is that the “to” in “route_to” feels misleadingly uni-directional when in reality it is a bi-directional test.
articles_path.should == ‘/wiki’

• “map” here is the right verb because we’ve always used that language to talk about how a given URL “maps to” a given controller#action. And remember how in the router DSL prior to Rails 3 everything in config/routes.rb started with “map”?
  get(path).should map_from(destination)
  get(’/wiki’).should map_via(:controller => ‘articles’, :action => ‘index’)
  get(’/wiki’).should route(:controller => ‘articles’, :action => ‘index’)
  It also includes the supporting code that adds these new “map”, “map_to”, “map_from” matchers, and the “get”, “post”, “put” and “delete” methods. All of this for Rails 3/RSpec 2 only.

I’m going to convert more routing specs and see if any more changes are needed to handle edge cases, but for a first cut I am pretty happy with the results, apart from my inability to decide on the right name for the bi-directional “map” matcher.

Cheers,
Wincent

Seems like progress. One thought: Why not use a macro-style syntax to
eliminate the boring boilerplate call to #it / #example and generate
examples instead?

[Wincent_C]

On Jul 5, 2010, at 4:14 AM, Matt W. wrote:

params_from(list_describing_destination).should == lengthy_hash_of_params

The second issue is that the “to” in “route_to” feels misleadingly uni-directional when in reality it is a bi-directional test.

The third issue is that for routes which don’t actually have that bi-directional mapping, “route_to” can’t be used and we must instead drop down to Rails’ assert_recognizes() and/or assert_generates() methods, or wrap them using our own matchers.

This is the source of confusion/trouble that others have brought up here
on this list as well. Thanks for identifying the underlying issue.

end

get(‘/wiki’).should map_both(:controller => ‘articles’, :action => ‘index’)
get(‘/wiki’).should < (:controller => ‘articles’, :action => ‘index’) # map_from

If anybody has a suitable suggestion please let me know.

Naming is hard.

In the meantime, here is an example of a spec file that has been converted to use this new “API”:

forums_routing_spec.rb · GitHub

Nice overall. Much of the code belongs in Rails, though, so I’d like to
try to get a patch in to Rails once we get this worked out. I’d like the
rspec-rails matchers to be simple wrappers for rails’ assertions
wherever possible.

It also includes the supporting code that adds these new “map”, “map_to”, “map_from” matchers, and the “get”, “post”, “put” and “delete” methods. All of this for Rails 3/RSpec 2 only.

I’m going to convert more routing specs and see if any more changes are needed to handle edge cases, but for a first cut I am pretty happy with the results, apart from my inability to decide on the right name for the bi-directional “map” matcher.

I think having three separate matchers would land us in a good spot. The
wrapped rails assertions also accept a hash representing query params,
so let’s see if we can accommodate that as well:

get(‘/something’, :ref_id => ‘1234’).should route_to(…)

Cheers,
Wincent

Seems like progress. One thought: Why not use a macro-style syntax to eliminate the boring boilerplate call to #it / #example and generate examples instead?

Something like:

describe “The router” do
it { should recognize(get(‘/wiki/foo’).as(“wiki#show”, :id => ‘foo’)
}
it { should generate(get(‘/wiki/foo’).from(“wiki#show”, :id =>
‘foo’) }
it { should route(get(‘/wiki/foo’).to(“wiki#show”, :id => ‘foo’) }
end

That’s definitely nice and clean, and it makes it easier to align the
names with the rails assertions, which I think adds some value. The
output could be formatted something like this:

The router
should recognize get(‘/wiki/foo’) as wiki#show with :id => ‘foo’
should generate get(‘/wiki/foo’) from wiki#show with :id => ‘foo’
should route get(‘/wiki/foo’) to wiki#show with :id => ‘foo’

Thoughts?

[Wincent_C]

On Jul 5, 2010, at 9:04 AM, Wincent C. wrote:

El 05/07/2010, a las 13:56, David C. escribió:

Nice overall. Much of the code belongs in Rails, though, so I’d like to try to get a patch in to Rails once we get this worked out. I’d like the rspec-rails matchers to be simple wrappers for rails’ assertions wherever possible.

Well, I’m not sure if there is a problem with the Rails assertions.

Not that there is a problem with them, but all Rails users could benefit
from methods like get(path) in their routing tests. Although, now that I
think of it, those assertions are available in functional tests, and the
http verbs are already defined there.

The thing that concerns me the most is the DestinationParser. Even
though it seems simple, that’s the sort of code that ends up making
rspec-rails a rails-dependent maintenance problem.

Yes, in my initial implementation I left that out seeing as I haven’t yet run into a spec that needed it, but I did have it in mind when I wrote the “get” etc methods (in fact, when I first typed them in I started with ‘def get path, options = {}’, but then thought, YAGNI, better not add that in until I am sure I need it…)

Something like:

describe “The router” do
it { should recognize(get(’/wiki/foo’)).as(“wiki#show”, :id => ‘foo’) }
it { should generate(get(’/wiki/foo’)).from(“wiki#show”, :id => ‘foo’) }
it { should route(get(’/wiki/foo’)).to(“wiki#show”, :id => ‘foo’) }
end

Yes, the “action#controller” notation is a good idea. I actually added that to the gist that I posted already.

That’s where I got it from

Not sure about nesting the get() method inside the matcher though. It seems that in most other places, we use hashes or literal values; eg:

something.should do_something(:when => ‘always’, :how => ‘quickly’)

So, it would be more consistent with existing style to write:

it { should recognize(:get => ‘/wiki/foo’).as(‘wiki#show’, :id => ‘foo’) }

But then we lose some of the conciseness I was looking for in my original proposal.

Keep in mind this is for a one-liner (no string passed to it), so it’s
still far more concise than the previous alternatives.

If so, one misgiving I have is that we still have a unidirectional preposition (“to”) being used in conjunction with the bidirectional assertion.
Agreed. Not sure what the right verbiage is there.

An interesting consequence of the language you propose is that the focus of testing changes ie. in the current generator templates, we have:

describe FooController do
describe ‘routing’ do
it ‘recognizes and generates #index’ do
{ :get => ‘bar’ }.should route_to(:controller => ‘bar’, :action => ‘index’)

So the focus is on controller actions, and the subject of each ‘it’ block is a literal hash representing a request to that controller and action.

The new language puts the router at the center and the implicit subject of each ‘it’ block is really the router itself.

I think that’s where it belongs. It’s the router that does the routing,
not the controller.

That’s definitely nice and clean, and it makes it easier to align the names with the rails assertions, which I think adds some value. The output could be formatted something like this:

The router
should recognize get(’/wiki/foo’) as wiki#show with :id => ‘foo’
should generate get(’/wiki/foo’) from wiki#show with :id => ‘foo’
should route get(’/wiki/foo’) to wiki#show with :id => ‘foo’

Thoughts?

I think it would be nicer to format that as “GET /wiki/foo” (ie. HTTP verb followed by path) rather than embedding the literal text of the “get(’/wiki/foo’)” method call.

You could still get that by adding a message:

it { should recognize(get(’/wiki/foo’)).as(“wiki#show”, :id => ‘foo’),
“recognizes GET /wiki/:id” }

Not saying that’s any better than the other formats, just that it exists
as an option.

Slight tangent - one nice thing about ‘recognize’ as a matcher name is
we get this for free:

it { should_not recognize(:get => ‘/wiki/foo’) }

 end
   put('/issues/123').should map('issues#update', :id => '123')

describe IssuesController do
describe ‘routing’ do
it { post(’/issues’).should map(‘issues#create’) }
it { get(’/issues’).should map(‘issues#index’) }
it { get(’/issues/new’).should map(‘issues#new’) }
it { get(’/issues/123’).should map(‘issues#show’, :id => ‘123’) }
it { get(’/issues/123/edit’).should map(‘issues#edit’, :id => ‘123’) }
it { put(’/issues/123’).should map(‘issues#update’, :id => ‘123’) }
it { delete(’/issues/123’).should map(‘issues#destroy’, :id => ‘123’) }
end
end

The point of "it’ is that it reads as part of a sentence:

describe “something” do
it “does something”

When we introduced the implicit subject, we got this:

describe “something” do
it { should do_something }

In that form it still reads nicely: “it should do something”.

In this case, saying “it get post issues should map issues#create” makes
me want to cry. We’ve still got example and specify. In this case, I
think specify is the most accurate:

describe IssuesController do
describe ‘routing’ do
specify { post(’/issues’).should map(‘issues#create’) }

Out loud this is “specify [that a] post [to] /issues should map [to]
issues#create”. I can live with that tear-free. Except when Brazil gets
knocked out of the cup, but that’s a different matter.

 should map "issues#destroy" and {:id=>"123"}
 should map PUT /issues/123 as issues#update with {:id=>"123"}
 should map DELETE /issues/123 as issues#destroy with {:id=>"123"}

Which IMO looks pretty nice.

It does, though seeing that last output makes me wonder about the
“map/as” verbiage. Seems less clear in this context for some reason.

Anybody else besides me and Wincent and Matt want to weigh in here?

Cheers,
David

[Wincent_C]

El 05/07/2010, a las 18:18, David C.
escribió:

The thing that concerns me the most is the DestinationParser. Even though it seems simple, that’s the sort of code that ends up making rspec-rails a rails-dependent maintenance problem.

But seeing as we’re wrapping Rails assertions we’re always going to be
vulnerable to upstream changes. We’re vulnerable to them right now in
the existing route_to matcher.

As I see it the purpose of the DestinationParser is to just take the
user’s parameters in the spec file (a format which we control, designed
to make specs read nicely) and put them in a format that
assert_(recognizes|generates|routing) expects (a format that the Rails
team controls, designed according to their own criteria). If they ever
change their format, they’ll break thousands of users’ specs, whether or
not we have a “DestinationParser”.

Slight tangent - one nice thing about ‘recognize’ as a matcher name is we get this for free:

it { should_not recognize(:get => ‘/wiki/foo’) }

True, but with “get()” we can already do:

it { get(’/wiki/foo’).should_not be_routable }

Which is effectively the same isn’t it?

In that form it still reads nicely: “it should do something”.

In this case, saying “it get post issues should map issues#create” makes me want to cry.

Yes, I know what you mean.

We’ve still got example and specify.

Knew about example, had forgotten about specify… now that one goes a
loooooong way back doesn’t it?

Which IMO looks pretty nice.

It does, though seeing that last output makes me wonder about the “map/as” verbiage. Seems less clear in this context for some reason.

I think it would read quite nicely as “should map something to
something”, but that puts us back in the old “uni-directional” word for
“bi-directional” relationship problem. I was trying to have:

• “should map X to Y” correspond to map_to/assert_recognizes
• “should map X from Y” correspond to map_from/assert_generates
• “should map X as Y” correspond to map/assert_routing

One possibility that I’ve been toying with is substituting “route” for
“map” here, but “route_to” clashes with the existing “route_to” matcher
in RSpec, and I am not sure that it buys us anything anyway… As you
said earlier on, language is hard…

Anybody else besides me and Wincent and Matt want to weigh in here?

Oops, does that mean I should stop posting?

Cheers,
Wincent

[Wincent_C]

El 05/07/2010, a las 13:56, David C.
escribió:

Nice overall. Much of the code belongs in Rails, though, so I’d like to try to get a patch in to Rails once we get this worked out. I’d like the rspec-rails matchers to be simple wrappers for rails’ assertions wherever possible.

Well, I’m not sure if there is a problem with the Rails assertions. In
my (albeit limited) experience I haven’t yet seen a situation where they
couldn’t express something during testing. So as you point out, the job
of RSpec should just be to expose those APIs within specs in a usable
and effective manner. If we discover some limitation that we can’t get
around, then sure, the upstream assertions will need some work.

I think having three separate matchers would land us in a good spot.

Agreed. That’s why I added a matcher for each of assert_recognizes,
assert_generates, and assert_routing (although so far in practice I’ve
only had to use two of them).

The wrapped rails assertions also accept a hash representing query params, so let’s see if we can accommodate that as well:

get(‘/something’, :ref_id => ‘1234’).should route_to(…)

Yes, in my initial implementation I left that out seeing as I haven’t
yet run into a spec that needed it, but I did have it in mind when I
wrote the “get” etc methods (in fact, when I first typed them in I
started with ‘def get path, options = {}’, but then thought, YAGNI,
better not add that in until I am sure I need it…)

Something like:

describe “The router” do
it { should recognize(get(‘/wiki/foo’).as(“wiki#show”, :id => ‘foo’) }
it { should generate(get(‘/wiki/foo’).from(“wiki#show”, :id => ‘foo’) }
it { should route(get(‘/wiki/foo’).to(“wiki#show”, :id => ‘foo’) }
end

Yes, the “action#controller” notation is a good idea. I actually added
that to the gist that I posted already.

Not sure about nesting the get() method inside the matcher though. It
seems that in most other places, we use hashes or literal values; eg:

something.should do_something(:when => ‘always’, :how => ‘quickly’)

So, it would be more consistent with existing style to write:

it { should recognize(:get => ‘/wiki/foo’).as(‘wiki#show’, :id =>
‘foo’) }

But then we lose some of the conciseness I was looking for in my
original proposal. In any case we also lose the similarity with
controller specs (where “get” is the first thing on the line) which I
was also looking for.

Nevertheless, your choice of verbs and prepositions work well together;
“recognize as”, “generate from” and “route to” all read fairly well.

Just to clarify, are you suggesting that:

• “recognize as” wrap assert_recognizes?
• “generate from” wrap assert_generates?
• “route to” wrap assert_routing?

If so, one misgiving I have is that we still have a unidirectional
preposition (“to”) being used in conjunction with the bidirectional
assertion.

An interesting consequence of the language you propose is that the focus
of testing changes ie. in the current generator templates, we have:

describe FooController do
describe ‘routing’ do
it ‘recognizes and generates #index’ do
{ :get => ‘bar’ }.should route_to(:controller => ‘bar’, :action
=> ‘index’)

So the focus is on controller actions, and the subject of each ‘it’
block is a literal hash representing a request to that controller and
action.

The new language puts the router at the center and the implicit subject
of each ‘it’ block is really the router itself.

That’s definitely nice and clean, and it makes it easier to align the names with the rails assertions, which I think adds some value. The output could be formatted something like this:

The router
should recognize get(‘/wiki/foo’) as wiki#show with :id => ‘foo’
should generate get(‘/wiki/foo’) from wiki#show with :id => ‘foo’
should route get(‘/wiki/foo’) to wiki#show with :id => ‘foo’

Thoughts?

I think it would be nicer to format that as “GET /wiki/foo” (ie. HTTP
verb followed by path) rather than embedding the literal text of the
“get(‘/wiki/foo’)” method call.

In any case, I converted another RESTful resource over to the syntax I
mentioned earlier, now using the “controller#action” syntax:

describe IssuesController do
describe ‘routing’ do
example ‘GET /issues’ do
get(‘/issues’).should map(‘issues#index’)
end

  example 'GET /issues/new' do
    get('/issues/new').should map('issues#new')
  end

  example 'GET /issues/:id' do
    get('/issues/123').should map('issues#show', :id => '123')
  end

  example 'GET /issues/:id/edit' do
    get('/issues/123/edit').should map('issues#edit', :id => '123')
  end

  example 'PUT /issues/:id' do
    put('/issues/123').should map('issues#update', :id => '123')
  end

  example 'DELETE /issues/:id' do
    delete('/issues/123').should map('issues#destroy', :id => '123')
  end
end

end

Which, if you really care about conciseness, can be written as:

describe IssuesController do
describe ‘routing’ do
it { post(‘/issues’).should map(‘issues#create’) }
it { get(‘/issues’).should map(‘issues#index’) }
it { get(‘/issues/new’).should map(‘issues#new’) }
it { get(‘/issues/123’).should map(‘issues#show’, :id => ‘123’) }
it { get(‘/issues/123/edit’).should map(‘issues#edit’, :id =>
‘123’) }
it { put(‘/issues/123’).should map(‘issues#update’, :id => ‘123’)
}
it { delete(‘/issues/123’).should map(‘issues#destroy’, :id =>
‘123’) }
end
end

These get formatted in the spec output as:

IssuesController
routing
GET /issues
GET /issues/new
GET /issues/:id
GET /issues/:id/edit
PUT /issues/:id
DELETE /issues/:id

And:

IssuesController
routing
should map “issues#create”
should map “issues#index”
should map “issues#new”
should map “issues#show” and {:id=>“123”}
should map “issues#edit” and {:id=>“123”}
should map “issues#update” and {:id=>“123”}
should map “issues#destroy” and {:id=>“123”}

Respectively. By adding a description method to the matcher (see updated
Gist: forums_routing_spec.rb · GitHub), we can produce:

IssuesController
routing
should map POST /issues as issues#create
should map GET /issues as issues#index
should map GET /issues/new as issues#new
should map GET /issues/123 as issues#show with {:id=>“123”}
should map GET /issues/123/edit as issues#edit with {:id=>“123”}
should map PUT /issues/123 as issues#update with {:id=>“123”}
should map DELETE /issues/123 as issues#destroy with {:id=>“123”}

Which IMO looks pretty nice.

Cheers,
Wincent

[Wincent_C]

El 05/07/2010, a las 18:58, Wincent C.
escribió:

El 05/07/2010, a las 18:18, David C. escribió:

Slight tangent - one nice thing about ‘recognize’ as a matcher name is we get this for free:

it { should_not recognize(:get => ‘/wiki/foo’) }

True, but with “get()” we can already do:

it { get(’/wiki/foo’).should_not be_routable }

Which is effectively the same isn’t it?

Bah, I take that back. Just looked and I see that the “be_routable”
matcher expects its params to be of the form:

{ :method => ‘path’ }

And get() is giving it params of the form:

{ :method => :get, :path => ‘path’ }

Cheers,
Wincent

[Wincent_C]

On Jul 5, 2010, at 11:58 AM, Wincent C. wrote:

El 05/07/2010, a las 18:18, David C. escribió:

The thing that concerns me the most is the DestinationParser. Even though it seems simple, that’s the sort of code that ends up making rspec-rails a rails-dependent maintenance problem.

But seeing as we’re wrapping Rails assertions we’re always going to be vulnerable to upstream changes. We’re vulnerable to them right now in the existing route_to matcher.

Sort of. What route_to relies on is a published API. If Rails changes
the meaning of assert_routing, that will impact all Rails users, not
just rspec users.

This is a hot-button issue for me because relying on anything but
published APIs has led to frantic day-after-rails-release rspec-rails
releases in the past. Probably not really in an issue in this case,
though. After looking at it some more, DestinationParser isn’t really
relying on any code in Rails, its relying on consistency in the routing
API going forward. i.e. if Rails stops supporting this format,
DestinationParser won’t break, but the concept won’t align correctly
with Rails any longer. In this case, I think we’re OK.

As I see it the purpose of the DestinationParser is to just take the user’s parameters in the spec file (a format which we control, designed to make specs read nicely) and put them in a format that assert_(recognizes|generates|routing) expects (a format that the Rails team controls, designed according to their own criteria). If they ever change their format, they’ll break thousands of users’ specs, whether or not we have a “DestinationParser”.

Right

Slight tangent - one nice thing about ‘recognize’ as a matcher name is we get this for free:

it { should_not recognize(:get => ‘/wiki/foo’) }

True, but with “get()” we can already do:

it { get(’/wiki/foo’).should_not be_routable }

Which is effectively the same isn’t it?

Effectively, yes, but I’m liking “recognize” better than “be_routable”.

In that form it still reads nicely: “it should do something”.

routing
It does, though seeing that last output makes me wonder about the “map/as” verbiage. Seems less clear in this context for some reason.

I think it would read quite nicely as “should map something to something”, but that puts us back in the old “uni-directional” word for “bi-directional” relationship problem. I was trying to have:

• “should map X to Y” correspond to map_to/assert_recognizes
• “should map X from Y” correspond to map_from/assert_generates
• “should map X as Y” correspond to map/assert_routing
  One possibility that I’ve been toying with is substituting “route” for “map” here, but “route_to” clashes with the existing “route_to” matcher in RSpec, and I am not sure that it buys us anything anyway…

I think “map” is working for me in this context. The word “route”
suggests “take a path and route it to a destination”, whereas “map”
suggests “these two things are related, so map one to the other”, at
which point [to|from|as] provide more context as to the nature of the
relationship.

As you said earlier on, language is hard…

Anybody else besides me and Wincent and Matt want to weigh in here?

Oops, does that mean I should stop posting?

Immediately!

Srsly, not a bit - just looking for other voices.

Cheers,
David

[Wincent_C]

El 05/07/2010, a las 19:17, David C.
escribió:

On Jul 5, 2010, at 11:58 AM, Wincent C. wrote:

El 05/07/2010, a las 18:18, David C. escribió:

The thing that concerns me the most is the DestinationParser. Even though it seems simple, that’s the sort of code that ends up making rspec-rails a rails-dependent maintenance problem.

But seeing as we’re wrapping Rails assertions we’re always going to be vulnerable to upstream changes. We’re vulnerable to them right now in the existing route_to matcher.

Sort of. What route_to relies on is a published API. If Rails changes the meaning of assert_routing, that will impact all Rails users, not just rspec users.

This is a hot-button issue for me because relying on anything but published APIs has led to frantic day-after-rails-release rspec-rails releases in the past. Probably not really in an issue in this case, though. After looking at it some more, DestinationParser isn’t really relying on any code in Rails, its relying on consistency in the routing API going forward. i.e. if Rails stops supporting this format, DestinationParser won’t break, but the concept won’t align correctly with Rails any longer. In this case, I think we’re OK.

Yes, it’s not doing any more than “route_to” is already doing (ie.
preparing the parameters to be fed into “assert_routing”). It may have
looked scarier than it was; I just wanted to stick it in a module so as
not to repeat the code in each of the three matchers.

Anybody else besides me and Wincent and Matt want to weigh in here?

Oops, does that mean I should stop posting?

Immediately!

Srsly, not a bit - just looking for other voices.

Yes, would be good. To be honest, with a change like this (involving
interface design), I think slowly is the way to go.

Cheers,
Wincent

[Wincent_C]

On Jul 5, 2010, at 12:38 PM, Wincent C. wrote:

This is a hot-button issue for me because relying on anything but published APIs has led to frantic day-after-rails-release rspec-rails releases in the past. Probably not really in an issue in this case, though. After looking at it some more, DestinationParser isn’t really relying on any code in Rails, its relying on consistency in the routing API going forward. i.e. if Rails stops supporting this format, DestinationParser won’t break, but the concept won’t align correctly with Rails any longer. In this case, I think we’re OK.

Yes, would be good. To be honest, with a change like this (involving interface design), I think slowly is the way to go.

Agreed, but there is a bit of pressure here - we don’t have separate
wrapper matchers assert_recognizes or assert_generates, and I want to
ship 2.0.0 with a solution for that. Given that rails 3 should have a
release candidate some time soon, there’s no time like the present

[Wincent_C]

I’ll chime in, having contributed some of the mess at hand.

Good things I’m seeing between current route helpers and proposals
include:

• The router being at the center of what’s being tested
• Similarity of specs to other conventions
• Ability to specify bi-directional routing behavior (by default, since
  it’s most common)
• Clear indication that a route spec is in fact testing both recognition
  and generation
• Easily specify negative routes (PUT /foo doesn’t route).
• Include testing of URL helpers
• DRY, but not at the expense of clarity

I’m uncertain about the need to easily specify one-directional routes.
While in theory it sounds fine, I don’t understand why you’d want to
specify either a route that isn’t recognized (why bother routing it, in
this case?) or one that doesn’t generate the given path with url_for()
(does it generate some other path instead?). I didn’t see any examples
of its usage in the gist, but I did see a lot of code for it… YAGNI??
or, if I were able to understand the motivation better, maybe this would
make more sense to me.

Randy

[Wincent_C]

El 05/07/2010, a las 20:18, Randy H.
escribió:

I’m uncertain about the need to easily specify one-directional routes.
While in theory it sounds fine, I don’t understand why you’d want to
specify either a route that isn’t recognized (why bother routing it, in
this case?) or one that doesn’t generate the given path with url_for()
(does it generate some other path instead?).

Well, I find that most routes map in both directions (ie. basically
anything that is specified using “resource” in the config/routes.rb
file) but from time to time I find non-resource routes that map only one
way (ie. are routable, but url_for won’t necessarily generate what you
want).

Mostly they seem to be routes declared with “match” like this one:

resource :posts
match ‘/posts/page/:page’ => ‘posts#index’, :as => ‘paginated_posts’

You can see here how the mapping doesn’t work out the same in both
directions:

• recognition: ‘/posts/page/2’ is recognized as ‘posts#index’ with :page
  => ‘2’

• generation: ‘posts#index’ with :page => ‘2’ generates ‘/posts?page=2’

Sometimes they can be reorganized so that they do map in both
directions; eg:

resources :posts do
collection do
get ‘page/:page’ => ‘posts#index’
end
end

With that change, Rails does know how to do the reverse mapping.

But then there are ones which, as far as I know, can’t be reorganized in
that way; here’s one example from my current app:

the resource

resource :links

the shortcut to the links#show action

match ‘l/:id’ => ‘links#show’

According to my experiments, the Rails routing assertions will say that
URLs like ‘l/foo’ are routable, but won’t generate in reverse. So
there’s a case for “assert_recognizes”/“map_to”, for sure.

I agree with you that it doesn’t make much sense to specify an
unrecognizable route, but I guess it is conceivable that you could have
routes that were recognizable in one direction, but only generated if
fed a different set of parameters. So you’d need to test them using a
pair of “map_from”/“map_to” or “assert_generates”/“assert_recognizes”.

Cheers,
Wincent

[Wincent_C]

David C. wrote:

ps - I moved your post to the bottom - please bottom/inline post
instead of top-posting (Top Posting and Bottom Posting).
Sorry, perhaps I should have deleted most of the quotey bits, since I
was replying very generally to the thread.
Check out these two threads:

http://groups.google.com/group/rspec/browse_thread/thread/9ac4ff6f3bac8423
http://groups.google.com/group/rspec/browse_thread/thread/4775eaa9b8b3c25f

Both of them were, coincidentally, started independently and with a day or two of this thread

I don’t think I understand the core of these two problems - nesting
isn’t something I’ve dealt with much. Are they both nested issues,
and/or is there a distinction between the two problems? It seems to me
that Rails’ own assertions aren’t able to test these well because the
route generation itself is suboptimal. Or does that characterization
reveal my lack of understanding?

Randy

[Wincent_C]

On 7/5/10 2:14 AM, Matt W. wrote:

Seems like progress. One thought: Why not use a macro-style syntax to eliminate the boring boilerplate call to #it / #example and generate examples instead?
I thought this was an insightful comment. Riffing on that, I get what
clearly becomes a DSL along the lines of the following:

describe_routes do # method of a controller spec
describe “GET /foo”
recognizes “GET /foo” # redundant?
ignores “PUT /foo”
formats [ :html, :json ], :not => [ :xml, :pdf ]
generated_from “controller#action”, :args => ijef
helper :action_controller_path
end
end

You could possibly mix and match some of these, like with multiple
“recognizes”. Some of these assertive verbs could certainly be
optimized too. And, at risk of being promoted to Captain Obvious, I’ll
point out that the examples generated from these declarations can be
crystal clear in their phrasing because the DSL is… well, more specific
to the domain.

I’m not sure how to honor the one-directional requirement, but if there
are use-cases, I think we could express that case in this DSL and
specify the expected outcomes.

Of course, there’s more than one way to do it.

Randy

[Wincent_C]

El 05/07/2010, a las 21:00, Randy H.
escribió:

and/or is there a distinction between the two problems? It seems to me
that Rails’ own assertions aren’t able to test these well because the
route generation itself is suboptimal. Or does that characterization
reveal my lack of understanding?

I have nothing intelligent to say on this just yet, but I have to do
some nested routing specs in my current work, so I should be able to
spend some time figuring it out tomorrow.

Cheers,
Wincent