[rspec-users] Possible improvements to routing spec API
Wincent Colaiuta
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":
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
_______________________________________________
rspec-users mailing list
rspec...@rubyforge.org
http://rubyforge.org/mailman/listinfo/rspec-users
Matt Wynne
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?
David Chelimsky
> On 5 Jul 2010, at 08:00, Wincent Colaiuta wrote:
>> 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.
This is the source of confusion/trouble that others have brought up here on this list as well. Thanks for identifying the underlying issue.
Naming is hard.
>> In the meantime, here is an example of a spec file that has been converted to use this new "API":
>>
>> http://gist.github.com/464081
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 Colaiuta
> 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: http://gist.github.com/464081), 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
David Chelimsky
> El 05/07/2010, a las 13:56, David Chelimsky 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.
> 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.
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.
> 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.
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') }
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.
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 Colaiuta
> 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?
> 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.
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?
>> 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.
>
> 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
David Chelimsky
On Jul 5, 2010, at 11:58 AM, Wincent Colaiuta wrote:
> El 05/07/2010, a las 18:18, David Chelimsky 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".
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 Colaiuta
> El 05/07/2010, a las 18:18, David Chelimsky 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' }
Wincent Colaiuta
> On Jul 5, 2010, at 11:58 AM, Wincent Colaiuta wrote:
>
>> El 05/07/2010, a las 18:18, David Chelimsky 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
David Chelimsky
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 :)
Randy Harmon
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
Randy Harmon
> 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?
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
Randy Harmon
> ps - I moved your post to the bottom - please bottom/inline post
> instead of top-posting (http://idallen.com/topposting.html).
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 Colaiuta
> David Chelimsky wrote:
>> 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?
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
Lalish-Menagh, Trevor
can of worms. :)
I agree with David that we should stick with wrapping the Rails public
APIs. That is: assert_generates, assert_recognizes, and assert_routing
(http://edgeapi.rubyonrails.org/classes/ActionDispatch/Assertions/RoutingAssertions.html).
OK, here is my thoughts on a format:
http://gist.github.com/464636
I want it to be readable in English, so let's try:
describe "routing" do
specify "days#index" do
route get('/days').should generate 'days#index' # wraps assert_generates
end
end
specify [that the] route [path] get('/days') should generate [the
options] 'days#index'
OK! Next:
describe "routing" do
specify "days#index" do
routing 'days#index'.should recognize get('/days') # wraps assert_recognizes
end
end
specify [that the] route [options] 'days#index' should recognize [the
path] get('/days')
(Might need some work... but it is uniform). Next:
describe "routing" do
specify "days#index" do
route get('/days').should be 'days#index' # wraps assert_routing
end
end
specify [that the] route [path] get('/days') should be [the options]
'days#index'
Finally, this makes nested routes (my original issue) look nice:
describe "routing" do
specify "days#index" do
route 'days#index'.should recognize get('/students/1/days') with
:student_id => '1'
end
end
specify [that the] route [options] 'days#index' should recognize [the
path] get('/students/1/days') with [extras] :student_id => '1'
You could also write out the options longhand to include the extras,
but I think this is too ugly:
describe "routing" do
specify "days#index" do
route {:controller => 'days', :action => 'index', :student_id =>
'1'}.should recognize ('/students/1/days')
end
end
specify [that the] route [options] {:controller => 'days', :action =>
'index', :student_id => '1'} should recognize [the path]
('/students/1/days')
What do you think?
Trevor
Wincent Colaiuta
> 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
David Chelimsky
Hey Trevor,
These are all great DSL ideas, but they strike me as being sufficiently different from anything else in either RSpec or Rails that it would be for confusing for users.
Do you have any thoughts on the other proposals?
Cheers,
David
Lalish-Menagh, Trevor
You make a good point. I was talking with a coworker about this
problem, and he suggested a simpler format, that I think will coincide
some with Wincent's thoughts. Here is my next stab
(http://gist.github.com/466064):
describe "routing" do
it "recognizes and generates #index" do
get("/days").should have_routing('days#index')
end
it "generates #index" do
get("/days").should generate('days#index')
end
it "recognizes #index" do
('days#index').should recognize get("/days")
end
describe "nested in students" do
it "recognizes #index" do
('days#index').with(:student_id => "1").should recognize
get("/students/1/days")
end
end
end
Notes:
- I am using have_routing, recognize, and generate because those are
the verbs used in Rails for the functions we are wrapping.
- I like using the word "with" to represent "extras," in the Rails API
(see http://edgeapi.rubyonrails.org/classes/ActionDispatch/Assertions/RoutingAssertions.html),
since I think it fits here, and we already use with in RSpec in other
places (like stubs, etc.).
- I like using get('path') since it is similar to the routing calls in
the Rails 3 route file, and I think it is easy to intuit.
- We can use the hash notation to conform to Rails 3, with an option
to provide a full hash as well (Rails 2 style).
- The format still reads like English, and using "have_routing"
instead of "routes_to" avoids the "_to" and "_from" problem that we
have been talking about, PLUS it makes it easier to draw a
relationship between the RSpec command and the Test::Unit command
(assert_routing).
- Using these verbs still allow "should_not" to make sense.
Thoughts,
Trevor
On Tue, Jul 6, 2010 at 9:14 AM, David Chelimsky <dchel...@gmail.com> wrote:
> Hey Trevor,
>
> These are all great DSL ideas, but they strike me as being sufficiently different from anything else in either RSpec or Rails that it would be for confusing for users.
>
> Do you have any thoughts on the other proposals?
>
> Cheers,
> David
--
Trevor Lalish-Menagh
484.868.6150 mobile
tr...@trevreport.org
http://www.trevmex.com/
Wincent Colaiuta
> Hi David,
>
> You make a good point. I was talking with a coworker about this
> problem, and he suggested a simpler format, that I think will coincide
> some with Wincent's thoughts. Here is my next stab
> (http://gist.github.com/466064):
> describe "routing" do
> it "recognizes and generates #index" do
> get("/days").should have_routing('days#index')
> end
Very nice. I definitely like this better than the "map()" terminology that I used.
> it "generates #index" do
> get("/days").should generate('days#index')
> end
Reads well, _but_ I think you've got it back-to-front. assert_generates, and therefore the corresponding matcher in RSpec, asserts that a _path_ can be generated from a set of _params_ (controller, action, additional params), but here you're asserting the opposite.
(One thing to note is that assert_generates really only does assert about the _path_, not the method + path, but it's still nice to write it as "get(path)" just for symmetry with the reverse version of the spec.)
> it "recognizes #index" do
> ('days#index').should recognize get("/days")
> end
This one is back-to-front too; assert_recognizes is an assertion that a path (this time method + path) is recognized as a particular route (action, controller, additional params) but here you're asserting the opposite.
If we're really serious about using the same verbs as the Rails assertions, and we want to forward and reverse versions of the spec to read in the same way, we could use something like:
specify { get('/days').should recognize_as('days#index') }
specify { get('/days').should generate_from('days#index') }
If we don't mind swapping the order around
specify { get('/days').should recognize_as('days#index') }
specify { 'days#index'.should generate('/days') }
Note that in the "generate" spec here I drop the "get()" seeing as Rails isn't actually looking at the method, and it is just verbiage IMO to start nesting other method calls inside the generate() matcher.
Weighing up the two orders, I prefer the first pair of specs, I think, because:
- the repetition of the pattern makes it easier to remember and apply.
- if you have to supply additional parameters (as you do in your later example), using the first format you can write ('days#index', :student_id => '1') whereas with the second format you are obliged to involve another method like with() in order to express it as "('days#index').with(:student_id => '1')".
> describe "nested in students" do
> it "recognizes #index" do
> ('days#index').with(:student_id => "1").should recognize
> get("/students/1/days")
> end
> end
> end
This is another back-to-front one, I think. You're not recognizing here, but generating.
> Notes:
> - I am using have_routing, recognize, and generate because those are
> the verbs used in Rails for the functions we are wrapping.
Seems like a good idea that we should definitely try to do, although with one reservation; if we feel that the language is confusing or suboptimal then we should feel free to change it.
I personally have lingering doubts about the Rails language because I think it _is_ confusing in a way. Look at the way you used recognize and generate back-to-front in this message. And I know that every time I want to make a comment about "assert_recognizes" and "assert_generates" I end up double-checking the API docs just to make sure that I'm about to use them in the right way.
> - I like using the word "with" to represent "extras," in the Rails API
> (see http://edgeapi.rubyonrails.org/classes/ActionDispatch/Assertions/RoutingAssertions.html),
> since I think it fits here, and we already use with in RSpec in other
> places (like stubs, etc.).
I don't really like it; I think it adds unnecessary verbiage to the specs.
> - I like using get('path') since it is similar to the routing calls in
> the Rails 3 route file, and I think it is easy to intuit.
> - We can use the hash notation to conform to Rails 3, with an option
> to provide a full hash as well (Rails 2 style).
Yes, the example I posted at http://gist.github.com/464081 does that.
> - The format still reads like English, and using "have_routing"
> instead of "routes_to" avoids the "_to" and "_from" problem that we
> have been talking about, PLUS it makes it easier to draw a
> relationship between the RSpec command and the Test::Unit command
> (assert_routing).
Yeah, I think "have_routing" is a definite improvement over "map".
I am less convinced that "recognize_as" is an improvement over the highly readable "map_to".
I am less attached to "map_from", though, and think "generate_from" might be an improvement.
Like I said above, I think the Rails terminology does have the potential to be confusing, so I don't necessarily feel "wedded" to it.
> - Using these verbs still allow "should_not" to make sense.
Yes, I think that's important.
One thing I wanted to add is that I discovered in my testing that the existing "be_routable" matcher isn't very useful. This is because it relies on "recognize_path", which I am not sure is public API or not, and "recognize_path" sometimes recognizes the unrecognizable... For example:
in config/routes.rb:
resource :issues, :except => :index
visit issues#index (/issues) in your browser:
get a RoutingError (expected)
in your spec:
recognize_path('/issues', :method => :get) # recognizes!
So this means you can't do stuff like get('issues#index').should_not be_routable, because the Rails recognize_path() method will tell you that it _is_ routable. Seems that it is only useful for a subset of unroutable routes (like completely non-existent resources, for example).
If you look in the Gist you'll see that I work around this limitation by adding a "be_recognized" matcher which doesn't use "recognize_path()" under the hood. Instead it uses "assert_recognizes" and decides what to do based on what kind of exception, if any, gets thrown by it. So you can write:
specify { get('/issues').should_not be_recognized }
(Funnily enough, assert_recognizes() _does_ use recognize_path() under the hood, but it's evidently doing some additional set-up and tear-down to make it work. It seems like wrapping assert_recognizes rather than replicating its logic, though, is the right thing to do.)
Cheers,
Wincent
David Chelimsky
Seems as though this format has been abandoned in this conversation:
it { should route(get "/issues/new").to("issues#new") }
it { should generate("/issues/new").from("issues#new") }
I think that verbiage is concise and intention revealing, and keeps the path on the left and the details on the right. Supported variants would be:
it { should route(:get => "/issues/new") ... # explicit hash for method/path
it { should route("/issues/new")... # defaults to get
it { should ...to(:controller => "issues", :action => "new") } # explicit hash for params
The negative version would be:
it { should_not route(get "/issues/new") }
We could alias route() with recognize() to get:
it { should_not recognize(get "/issues/new") }
We still need to consider output. For example:
it { should route("/issues/37").to("issues#show", :id => 37) }
I think in this case we _might_ be able to figure out that 37 is the value of :id and output
should route "/issues/:id" to "issues#show"
Not sure if that'd be asking for trouble, but it seems feasible/reasonable.
In terms of the more verbose format for custom messages, we could add a router() method for sugar:
it "routes /issues/new to issues#new" do
router.should route("/issues/new").to("issues#new") }
end
And we still need the bi-directional version. I'm really at a loss on this one.
Thoughts?
David
Wincent Colaiuta
> Seems as though this format has been abandoned in this conversation:
>
> it { should route(get "/issues/new").to("issues#new") }
> it { should generate("/issues/new").from("issues#new") }
>
> I think that verbiage is concise and intention revealing, and keeps the path on the left and the details on the right.
True, but it also looks like it might be a little painful to implement. Seeing as you propose a little further down that we should also be able to do stuff like:
it { should_not route(get "/issues/new") }
That means that "route" needs to be (or return) a matcher that also, optionally, responds to the "to" message and returns another matcher.
And "generate" needs to return a proxy that responds to "from" and returns the real matcher, but itself raises an exception if the user mistakenly uses it without chaining the "from" onto it.
Seems fairly magical compared to what we have right now, or what I proposed earlier on.
> Supported variants would be:
>
> it { should route(:get => "/issues/new") ... # explicit hash for method/path
> it { should route("/issues/new")... # defaults to get
> it { should ...to(:controller => "issues", :action => "new") } # explicit hash for params
>
> The negative version would be:
>
> it { should_not route(get "/issues/new") }
>
> We could alias route() with recognize() to get:
>
> it { should_not recognize(get "/issues/new") }
Sounds sensible.
> We still need to consider output. For example:
>
> it { should route("/issues/37").to("issues#show", :id => 37) }
>
> I think in this case we _might_ be able to figure out that 37 is the value of :id and output
>
> should route "/issues/:id" to "issues#show"
>
> Not sure if that'd be asking for trouble, but it seems feasible/reasonable.
I don't think it's worth it, to be honest. It would add a lot of complexity just to save some characters in the spec output. (Saving characters when _writing_ the specs seems more important.) And I think it's throwing away information, because the user reading the spec now has to mentally unpack the ":id" parameter, but doesn't actually see it. Multiple examples with different :ids in them are now all going to look the same. And what if the user has complex routes with requirements and regexp formats and stuff on them? Won't that be a little hard to handle?
> In terms of the more verbose format for custom messages, we could add a router() method for sugar:
>
> it "routes /issues/new to issues#new" do
> router.should route("/issues/new").to("issues#new") }
> end
Yeah, adding router() is a good idea.
> And we still need the bi-directional version. I'm really at a loss on this one.
Perhaps something with/like the "have_routing" that Trevor suggested earlier? Possibly:
it { should have_routing('/issues/new').for('issues#new') }
I've pasted a comparison of the two proposals in their current forms here so we can get a side-by-side (well, above/below) look at them:
Still not convinced about the bi-directional language, though (used "have_routing" in both flavors). If anyone can come up with better language I'll update the comparison.
For my taste, the first one reads nicer for most of the specs, but I like the "router.should_not recognize('foo')" syntax, at least for the GET case. I like it less for the non-GET case, though, where we have to nest another method call (ie. "router.should_not recognize(put '/issues/123')".
Cheers,
Wincent
David Chelimsky
On Jul 7, 2010, at 7:39 AM, Wincent Colaiuta wrote:
> El 07/07/2010, a las 13:29, David Chelimsky escribió:
>
>> Seems as though this format has been abandoned in this conversation:
>>
>> it { should route(get "/issues/new").to("issues#new") }
>> it { should generate("/issues/new").from("issues#new") }
>>
>> I think that verbiage is concise and intention revealing, and keeps the path on the left and the details on the right.
>
> True, but it also looks like it might be a little painful to implement. Seeing as you propose a little further down that we should also be able to do stuff like:
>
> it { should_not route(get "/issues/new") }
>
> That means that "route" needs to be (or return) a matcher that also, optionally, responds to the "to" message and returns another matcher.
>
> And "generate" needs to return a proxy that responds to "from" and returns the real matcher, but itself raises an exception if the user mistakenly uses it without chaining the "from" onto it.
>
> Seems fairly magical compared to what we have right now, or what I proposed earlier on.
The chain method in matchers is intended just for this purpose:
matcher :foo do |expected_foo|
match do |actual|
if @expected_bar
# do one thing
else
# do another
end
end
chain :bar do |expected_bar|
@expected_bar = expected_bar
end
end
thing.should foo('blah').bar('blah again')
>> Supported variants would be:
>>
>> it { should route(:get => "/issues/new") ... # explicit hash for method/path
>> it { should route("/issues/new")... # defaults to get
>> it { should ...to(:controller => "issues", :action => "new") } # explicit hash for params
>>
>> The negative version would be:
>>
>> it { should_not route(get "/issues/new") }
>>
>> We could alias route() with recognize() to get:
>>
>> it { should_not recognize(get "/issues/new") }
>
> Sounds sensible.
>
>> We still need to consider output. For example:
>>
>> it { should route("/issues/37").to("issues#show", :id => 37) }
>>
>> I think in this case we _might_ be able to figure out that 37 is the value of :id and output
>>
>> should route "/issues/:id" to "issues#show"
>>
>> Not sure if that'd be asking for trouble, but it seems feasible/reasonable.
>
> I don't think it's worth it, to be honest. It would add a lot of complexity just to save some characters in the spec output. (Saving characters when _writing_ the specs seems more important.) And I think it's throwing away information, because the user reading the spec now has to mentally unpack the ":id" parameter, but doesn't actually see it. Multiple examples with different :ids in them are now all going to look the same. And what if the user has complex routes with requirements and regexp formats and stuff on them? Won't that be a little hard to handle?
Agreed. I was thinking of an earlier suggestion that the output should read in a more generic way, but that can be managed explicitly by the spec-author.
>> In terms of the more verbose format for custom messages, we could add a router() method for sugar:
>>
>> it "routes /issues/new to issues#new" do
>> router.should route("/issues/new").to("issues#new") }
>> end
>
> Yeah, adding router() is a good idea.
>
>> And we still need the bi-directional version. I'm really at a loss on this one.
>
> Perhaps something with/like the "have_routing" that Trevor suggested earlier? Possibly:
>
> it { should have_routing('/issues/new').for('issues#new') }
>
> I've pasted a comparison of the two proposals in their current forms here so we can get a side-by-side (well, above/below) look at them:
>
> http://gist.github.com/466624
>
> Still not convinced about the bi-directional language, though (used "have_routing" in both flavors). If anyone can come up with better language I'll update the comparison.
How about going back to map, with to_and_from:
it { should map(get "/issues/new").to_and_from("issues#new") }
?????
>
> For my taste, the first one reads nicer for most of the specs, but I like the "router.should_not recognize('foo')" syntax, at least for the GET case. I like it less for the non-GET case, though, where we have to nest another method call (ie. "router.should_not recognize(put '/issues/123')".
What don't you like about the nesting?
Wincent Colaiuta
> On Jul 7, 2010, at 7:39 AM, Wincent Colaiuta wrote:
>
>> El 07/07/2010, a las 13:29, David Chelimsky escribió:
>
> How about going back to map, with to_and_from:
>
> it { should map(get "/issues/new").to_and_from("issues#new") }
>
> ?????
"?????" is precisely what I think whenever I try to figure out the question of naming this particular thing...
>> For my taste, the first one reads nicer for most of the specs, but I like the "router.should_not recognize('foo')" syntax, at least for the GET case. I like it less for the non-GET case, though, where we have to nest another method call (ie. "router.should_not recognize(put '/issues/123')".
>
> What don't you like about the nesting?
Well, it's subtle, but it smells bad to me because it feels like unnecessary indirection. Instead of saying:
this_thing.should satisfy(that_thing)
I'm forced to say:
this_thing.should satisfy(transform(that_thing))
ie. the nested method in there is transforming "that_thing" to make it suitable for consumption by the "satisfy" matcher.
Seeing as we provide the "satisfy" matcher and we also define the requirements for its input, it feels somehow wrong to make the user feed the input through an additional, intermediate method. Why not just make the "satisfy" matcher accept the "that_thing" input as-is? Or any needed transformation itself?
So this is why:
it { should map(:get => '/issues/new').to_and_from('issues#new') }
Feels "right", at least to me, compared to:
it { should map(get '/issues/new').to_and_from('issues#new') }
Which smells.
Seeing as we provide the "map" matcher, at least in the common case of the GET request, we can apply the shortcut that you mentioned earlier and assume GET if not explicitly stated; eg.
it { should map('/issues/new').to_and_from('issues#new') }
Like I said, it's subtle. To me it feels ok to have an unnested transformer on the left side of the "should"; eg:
specify { get('/issues/new').should route('issues#new') }
But nesting it inside a matcher just sets off alarm bells for me. (Even worse than the "too much magic" alarm bells that ring for me when I see matcher chaining.)
Lalish-Menagh, Trevor
that make sense. I agree with Wincent that the Rails verbiage for the
routing tests is confusing, but what is NOT confusing is the new
routing format, so why not try out this format
(http://gist.github.com/467563):
describe 'routing another way' do
it { should have_resources(:days) }
it { should get('/days' => 'days#index') }
it { should match('/days' => 'days#index', :via => :get) }
it { should recognize('/days', :via => :get).as('days#index') }
it { should generate('days#index').from('/days', :via => :get) }
it { should recognize('/students/1/days', :via =>
:get).as('days#index', :student_id => '1') }
end
Notes:
it { should have_resources(:days) }
- tests all resource routes (index, show, etc.)
it { should get('/days' => 'days#index') }
- wrapper for assert_routing({:method => :get, :path => '/days'},
{:controller => 'days', :action => 'index'})
it { should match('/days' => 'days#index', :via => :get) }
- same as the previous example
it { should recognize('/days', :via => :get).as('days#index') }
- wrapper for assert_recognizes({:controller => 'days', :action =>
'index'}, {:method => :get, :path => '/days'})
it { should generate('days#index').from('/days', :via => :get) }
- wrapper for assert_generates({:method => :get, :path => '/days'},
{:controller => 'days', :action => 'index'})
it { should recognize('/students/1/days', :via =>
:get).as('days#index', :student_id => '1') }
- wrapper for assert_recognizes({:controller => 'days', :action =>
'index', :student_id => '1'}, {:method => :get, :path =>
'/students/1/days'})
The Rails test read backwards, look at this example from
http://edgeapi.rubyonrails.org/classes/ActionDispatch/Assertions/RoutingAssertions.html:
# assert that POSTing to /items will call the create action on ItemsController
assert_recognizes({:controller => 'items', :action => 'create'},
{:path => 'items', :method => :post})
The comment reads like the inverse of the actual method call. I think
that recognize(path).as(options) reads better (more like the comment
above).
The same goes for generate(options).from(path).
This approach gives us a very Rails-readable test and keeps it readable.
Thoughts?
Trevor
--
Trevor Lalish-Menagh
484.868.6150 mobile
tr...@trevreport.org
http://www.trevmex.com/
Wincent Colaiuta
> OK, here is an idea. I was thinking about how to make routing tests
> that make sense. I agree with Wincent that the Rails verbiage for the
> routing tests is confusing, but what is NOT confusing is the new
> routing format, so why not try out this format
> (http://gist.github.com/467563):
>
> describe 'routing another way' do
> it { should have_resources(:days) }
> it { should get('/days' => 'days#index') }
> it { should match('/days' => 'days#index', :via => :get) }
> it { should recognize('/days', :via => :get).as('days#index') }
> it { should generate('days#index').from('/days', :via => :get) }
> it { should recognize('/students/1/days', :via =>
> :get).as('days#index', :student_id => '1') }
> end
[snipped for brevity]
> it { should generate('days#index').from('/days', :via => :get) }
Hehe. You got it back-to-front again. "assert_generates" asserts that a _path_ is generated from a set of params (action, controller, additional params). Here you are doing the opposite, saying that certain params (action, controller) get generated from a path.
> The Rails test read backwards, look at this example from
> http://edgeapi.rubyonrails.org/classes/ActionDispatch/Assertions/RoutingAssertions.html:
> # assert that POSTing to /items will call the create action on ItemsController
> assert_recognizes({:controller => 'items', :action => 'create'},
> {:path => 'items', :method => :post})
>
> The comment reads like the inverse of the actual method call. I think
> that recognize(path).as(options) reads better (more like the comment
> above).
I think that's the general pattern of pretty much all Test::Unit assertions which compare two parameters. Where RSpec tends to say:
actual.should == expected
Test::Unit tends to say:
assert expected, actual
> Thoughts?
Well, this comes back to the eternal question of specing things like validations. Where the spec ends up being an almost identical re-statement of the implementation:
validates_presence_of :foo
it { should validate_presence_of(:foo) }
Are you testing the Rails validation code itself (which is already tested)? Are you testing that you remembered to include the validation itself? Do you trust your "validate_presence_of" matcher? Are you testing your ability to cut and paste? etc.
Well, same questions can arise with routing when the implementation and spec look like this:
resource :days
it { should have_resource(:days) }
So, when I see this kind of thing, I think _what_ are we trying to achieve in our routing specs and _why_? At least for me, the answers are:
1. Confirm: To confirm that the routes I've defined actually do what I think they do
2. Describe: To serve as a complete, explicit, readable specification of the behavior I expect from my routes
And finally:
3. That the Rails routing code actually works as advertised
This last one is a bit dubious, but the truth is the Rails 3 router is a complete re-write and bugs in it and incompatibilities with the old router are being found and squished constantly. An exhaustive set of routing specs can definitely help to uncover undiscovered edge cases.
So, bearing in mind those goals, what I actually need from RSpec in order to achieve them is:
- most importantly, a way of asserting that a user action (eg. HTTP verb + path + params) gets routed to a given controller + action with certain additional parameters (ie. a wrapper for assert_recognizes)
- less importantly, a way of asserting that a set of params (action, controller, additional params) generates a particular path (ie. a wrapper for assert_generates); this is less important for me because in practice close to all (98%) of my URL generation is done with named URL helpers, and I can test those explicitly if I want
- as syntactic sugar, a way of combining the above two assertions into one for those cases where the mapping is perfectly bidirectional (ie. a wrapper for assert_routing).
With these tools I can achieve pretty much everything I need: not only test that user actions end up hitting the right spots in my application, but also specify clearly and explicitly what I expect those mappings to be.
So for me, anything else doesn't really help me achieve my goals. The "have_resource(s)" matcher, for instance, doesn't help me and in fact actually undermines my goal of providing a complete and explicit specification of how my routes work.
The "recognize" and "generate" matchers you suggest obviously are "on target" to help me fulfill my goals. Of these, the "recognize" one is the most important one though.
Of the "match" and "get" matchers you suggest, seeing as they both wrap the same thing (assert_routing), one of them would have to go. I'd probably ditch "match" because it is just a repetition of the router DSL method of the same name, and my goal here isn't just to repeat that DSL in my specs.
In fact, if you look at my most important goal -- asserting that a user action (HTTP verb, path, params) hits a specific end point (controller + action + additional params) -- you'll understand why, in my proposal, all of my specifications start with "get"/"post" etc followed by path, params and then controller/action. It's not just for resonance with other parts of RSpec like where we use "get" etc in controller specs.
But if the goal is just to wrap the 3 Rails assertions as faithfully as possible, then you wind up with a different proposal. What David has posted is probably the closest to this goal.
And if the goal is make the specs map as closely as possible onto the language of config/routes.rb, then you wind up with a different proposal still... (ie. the one you just made).
So, I guess what I'm saying here is I'd like to hear _what_ people are wanting to achieve in their routing specs and _why_; and then to ask what kind of means RSpec should provide to best achieve those goals.
Michael Kintzer
agree with the goals as set out by Wincent below, and from a
readability standpoint have preferred the API as suggested by
Trevor.
-Michael
On Jul 7, 11:25 pm, Wincent Colaiuta <w...@wincent.com> wrote:
>
> So, when I see this kind of thing, I think _what_ are we trying to achieve in ourroutingspecs and _why_? At least for me, the answers are:
>
> 1. Confirm: To confirm that the routes I've defined actually do what I think they do
>
> 2. Describe: To serve as a complete, explicit, readable specification of the behavior I expect from my routes
>
> And finally:
>
> 3. That the Railsroutingcode actually works as advertised
>
> This last one is a bit dubious, but the truth is the Rails 3 router is a complete re-write and bugs in it and incompatibilities with the old router are being found and squished constantly. An exhaustive set ofroutingspecs can definitely help to uncover undiscovered edge cases.
>
> So, bearing in mind those goals, what I actually need from RSpec in order to achieve them is:
>
> - most importantly, a way of asserting that a user action (eg. HTTP verb + path + params) gets routed to a given controller + action with certain additional parameters (ie. a wrapper for assert_recognizes)
>
> - less importantly, a way of asserting that a set of params (action, controller, additional params) generates a particular path (ie. a wrapper for assert_generates); this is less important for me because in practice close to all (98%) of my URL generation is done with named URL helpers, and I can test those explicitly if I want
>
> - as syntactic sugar, a way of combining the above two assertions into one for those cases where the mapping is perfectly bidirectional (ie. a wrapper for assert_routing).
>
> With these tools I can achieve pretty much everything I need: not only test that user actions end up hitting the right spots in my application, but also specify clearly and explicitly what I expect those mappings to be.
>
> So for me, anything else doesn't really help me achieve my goals. The "have_resource(s)" matcher, for instance, doesn't help me and in fact actually undermines my goal of providing a complete and explicit specification of how my routes work.
>
> The "recognize" and "generate" matchers you suggest obviously are "on target" to help me fulfill my goals. Of these, the "recognize" one is the most important one though.
>
> Of the "match" and "get" matchers you suggest, seeing as they both wrap the same thing (assert_routing), one of them would have to go. I'd probably ditch "match" because it is just a repetition of the router DSL method of the same name, and my goal here isn't just to repeat that DSL in my specs.
>
> In fact, if you look at my most important goal -- asserting that a user action (HTTP verb, path, params) hits a specific end point (controller + action + additional params) -- you'll understand why, in my proposal, all of my specifications start with "get"/"post" etc followed by path, params and then controller/action. It's not just for resonance with other parts of RSpec like where we use "get" etc in controller specs.
>
> But if the goal is just to wrap the 3 Rails assertions as faithfully as possible, then you wind up with a different proposal. What David has posted is probably the closest to this goal.
>
> And if the goal is make the specs map as closely as possible onto the language of config/routes.rb, then you wind up with a different proposal still... (ie. the one you just made).
>
> So, I guess what I'm saying here is I'd like to hear _what_ people are wanting to achieve in theirroutingspecs and _why_; and then to ask what kind of means RSpec should provide to best achieve those goals.
>
> Cheers,
> Wincent
>
> _______________________________________________
> rspec-users mailing list
> rspec-us...@rubyforge.orghttp://rubyforge.org/mailman/listinfo/rspec-users