[rspec-users] Possible improvements to routing spec API

Randy Harmon rjharmon0316 at yahoo.com
Mon Jul 5 14:18:48 EDT 2010


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


On 7/5/10 9:18 AM, David Chelimsky wrote:
> On Jul 5, 2010, at 9:04 AM, Wincent Colaiuta wrote:
>
>   
>> 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') }
>
>   
>> 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
>>     
> 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.
>
>   
>> 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.
>>     
> 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
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users
>
>   



More information about the rspec-users mailing list