[rspec-users] Possible improvements to routing spec API

David Chelimsky dchelimsky at gmail.com
Wed Jul 7 07:29:55 EDT 2010


On Jul 7, 2010, at 1:24 AM, Wincent Colaiuta wrote:

> El 07/07/2010, a las 01:16, Lalish-Menagh, Trevor escribió:
> 
>> 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.)

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


More information about the rspec-users mailing list