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@rubyforge.org
http://rubyforge.org/mailman/listinfo/rspec-users
