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